BUGFIX: Oversize server commands

[bani]

The hack involves using a bufferoverflow in the sound system. A bind for a vsay is made, this vsay howerever seems to be larger than the buffer will allow and causing an immediate disconnect from all users connected to the server.

this isnt exactly correct. its not a buffer overflow of the buffer (or the server would crash).

it's basically the client engine being retarded.

if you send an oversize trap_sendservercommand(), the server happily sends the data to the client. however the client (engine, not cgame) expects server commands to never be > 1022 characters. so the client engine truncates the received servercommand at 1022, and the client engine interprets the next character in the server command string as a network command byte.

the client then gets totally confused at this point trying to interpret the rest of the string as raw network protocol, and blows up.

here's a true fix. basically it logs oversize commands and drops them on the floor. this will stop the vsay exploit, and any similar exploits in the future.

g_syscalls.c:

Code: Select all

void trap_SendServerCommand( int clientNum, const char *text ) {
        // rain - hack - commands over 1022 chars will crash the
        // client upon receipt, so ignore them
        if( strlen( text ) > 1022 ) {
                G_LogPrintf( "trap_SendServerCommand( %d, ... ) length exceeds 1022.\n", clientNum );
                G_LogPrintf( "text [%s]\n", text );
                return;
        }
        syscall( G_SEND_SERVER_COMMAND, clientNum, text );
}

[Badhabit]

Bani
Is this for the server or the client?
And were does this (code)config need to go.

[bani]

there is only one g_syscalls.c in the SDK and only one void trap_SendServerCommand in g_syscalls.c

[bani]

ok, the limit turned out to be 1022... documentation updated to reflect that.

[spoon]

No, DG. Yer right... you can't stick this into a pk3. Basically, it's a fix for the source code, which means you need to get the wet_source package, find a compiler for Windows and Linux, and then rebuild the qagame so/DLL files.

And that's on the list of stuff bani mentioned they weren't going to cover.

The fix is already in etpro, and this is really only relevant if you're trying to make your own mod.

[DG]

oopsie i deleted and posted on SD forum instead

[spoon]

DG: D'oh.

Any rate, I was just wondering...

After grep'ing through the source this weekend, I ran across SanitizeString() in g_cmds.c Could you just stick a bound check in there with something like

Code: Select all

void SanitizeString( char *in, char* out, qboolean fToLower)
{
      int cnt = 0;
      while&#40; *in && cnt++ < 1022 &#41; &#123;
              /* same stuff */
      &#125;
      *out = 0;
&#125;

Or am I overly optimistic in assuming that they call this func every time the game accepts string input from the user?

[ReyalP]

The fix is already in etpro, and this is really only relevant if you're trying to make your own mod.

Just to clarify, it is not in 2.0.7. It is fixed in the NEXT version of ETPro.

[spoon]

Hrm. What I was thinking of was in the 2.0.7 announcement:

bugfix: fix bug allowing spectators to crash the server - no, we arent going to tell you how to do it

So this was something else besides an overflow?

And the hits just keeeeeeeep on comin'.

[ReyalP]

g_syscalls.c:

Code: Select all

void trap_SendServerCommand( int clientNum, const char *text ) {
        // rain - hack - commands over 1022 chars will crash the
        // client upon receipt, so ignore them
        if( strlen( text ) > 1022 ) {
                G_LogPrintf( "trap_SendServerCommand( %d, ... ) length exceeds 1022.\n", clientNum );
                G_LogPrintf( "text [%s]\n", text );
                return;
        }
        syscall( G_SEND_SERVER_COMMAND, clientNum, text );
}

FWIW, G_LowPrintf will only print the first 1023 chars (includng timestamp) so the above will always be truncated, eating the \n and leaving the next log message on the same line. Not that it really matters...

[bani]

you could always up the buffersize ...

[Chruker]

Wouldn't a length check of the string in G_LogPrintf be better, than just upping the buffer?

[bani]

no. and we dont up the buffer either.

[NYKiller]

I dont get this How do you fix it like what are the steps you have to do?

[WeblionX]

You modify the source in your mod. What's this, you're making a mod and you don't know how to edit source? What, you're not making a mod?! Then what are you doing here?