Userinfo duplicate key vulnerability

[Chomenor]

The Info_SetValueForKey and Info_RemoveKey functions do not correctly handle input strings containing duplicate keys. Note this example:

char buffer[MAX_INFO_STRING];
Q_strncpyz( buffer, "\\testkey\\oldvalue1\\testkey\\oldvalue2", sizeof( buffer ) );
Info_SetValueForKey( buffer, "testkey", "newvalue" );
Com_Printf( "%s\n", Info_ValueForKey( buffer, "testkey" ) );
Com_Printf( "%s\n", buffer );

Output:

oldvalue2
\testkey\oldvalue2\testkey\newvalue

Info_SetValueForKey calls Info_RemoveKey once, stripping the first value (oldvalue1), then appends the new value to the end of the info string. When Info_ValueForKey is called it gets the next value in the string which is now oldvalue2, instead of the expected new value.

Clients can exploit this to manipulate values normally set by the engine in the userinfo string. For example, a userinfo string containing \ip\localhost\ip\localhost\ip\localhost can bypass both server IP bans and the password check in the standard game module, since the password check is conditional on the IP address not being equal to localhost.

[ensiform]

Quake3e specific or also affects original engines and ioq3?

[ec-]

Fixed in https://github.com/ec-/Quake3e/commit/1b065096d8df140dc93c6ad875ad499d3b7f52cc

[Chomenor]

@ensiform I think it does also affect the original engine and ioq3. Anything that uses a version of Info_RemoveKey that only removes the first key, and calls it on unsanitized userinfo, could be vulnerable to unexpected effects. In most cases it is probably not too serious of an issue though, compared to other possible ways to attack a server.

[Chomenor]

@ensiform Upon further investigation, it appears that this issue likely was Quake3e specific and introduced by 4ac8880ed6a111c535e533d7884e44aa05ed5c41.