Fix arbitrary network data sending exploit via malformed userinfo

[Splatt581]

This is a new exploit that was first mentioned in this issue - #1073

By using invalid utf8 chars in userinfo, an attacker can interrupt the reading of the string in svc_updateuserinfo for all connected clients, which will likely cause them to disconnect from the server due to incorrect reading of the rest of the packet. However, in addition to simply disconnecting clients, an attacker can append arbitrary network traffic to his userinfo, which connected clients can process. For example, in the PoC video below, an attacker compromises the svc_stufftext message, which executes some commands/cvars in the console of other clients:

https://www.youtube.com/watch?v=BIG7I859_aI

The thing is that when processing userinfo via the setinfo command, the server already has a check in the code for invalid utf8 chars, it is located in the Info_SetValueForStarKey func:

if (!Q_UnicodeValidate(key) || !Q_UnicodeValidate(value))
{
	Con_Printf("Keys and values must be valid utf8 text\n");
	return FALSE;
}

But there is no such check for userinfo sent via C2S_CONNECTION message, so I suggest adding it to Info_IsValid func.

So this pr fixes this vulnerability.

[dystopm]

gj sir

[ghouls96]

confirm changes please!

[UnrealKaraulov]

Yes

topcolor\0\bottomcolor\0\cl_dlmax\1024\cl_lc\0\cl_lw\1\cl_updaterate\102\rate\100000\1\?cl_updaterateep\        name ~0WN3D~BY~X64CR3W~;spk hgrunt/c2a3_hg_laugh?\1\name\Player\model\urban\*sid\76561199447977504
topcolor\0\bottomcolor\0\cl_dlmax\1024\cl_lc\0\cl_lw\1\cl_updaterate\102\rate\100000\1\?cl_updaterateep\        name ~0WN3D~BY~X64CR3W~;spk hgrunt/c2a3_hg_laugh?\1\name\Player\model\urban\*sid\76561199447977504

[UnrealKaraulov]

Now this exploit detected by https://github.com/UnrealKaraulov/UnrealDemoScanner

[Xelson]

here is the attack traffic recorded on the demo:

There are two notable points here:

1. instead of the next key value pair, 0xFF bytes are written, which stops parsing in MSG_ReadStringLine. A truncated userinfo string literally comes to the validation check.
   

   ReHLDS/rehlds/engine/common.cpp

   Lines 1092 to 1097 in e999171

   	char *MSG_ReadStringLine(void)
   	{
   	int c = 0, l = 0;
   	static char string[2048];
   	while ((c = MSG_ReadChar(), c) && c != '\n' && c != -1 && l < ARRAYSIZE(string) - 1)

   
   called from here
   

   ReHLDS/rehlds/engine/sv_main.cpp

   Lines 3693 to 3700 in e999171

   	void SV_ConnectionlessPacket(void)
   	{
   	char *args;
   	const char *c;
   	MSG_BeginReading();
   	MSG_ReadLong();
   	args = MSG_ReadStringLine();

   
   Technically, the string is valid, but it ends unexpectedly for the validator. At the same time, malicious traffic is recorded in subsequent bytes, which is decoded as SVC_DISCONNECT (0x02) with the corresponding message about the vacban
   
   If such traffic had reached the player's netchan, it would have been perceived by the game client as a message about the player's kick for a vac ban, which actually did not happen.
2. despite the fact that the string itself is validated and truncated from the server's point of view, the players are not sent the string, but directly traffic from attacker's netchan
   

   ReHLDS/rehlds/engine/sv_main.cpp

   Line 4088 in e999171

   SV_FullClientUpdate(client, &host_client->netchan.message);

   
   and the problem here is that the string is read and sent exactly until the null terminator is detected, although MSG_ReadStringLine also reacts to 0xFF
   

   ReHLDS/rehlds/engine/sv_main.cpp

   Lines 4064 to 4067 in e999171

   	MSG_WriteByte(sb, svc_updateuserinfo);
   	MSG_WriteByte(sb, cl - g_psvs.clients);
   	MSG_WriteLong(sb, cl->userid);
   	MSG_WriteString(sb, info);

   
   

   ReHLDS/rehlds/engine/common.cpp

   Lines 293 to 297 in e999171

   	void MSG_WriteString(sizebuf_t *sb, const char *s)
   	{
   	if (s)
   	{
   	SZ_Write(sb, s, Q_strlen(s) + 1);

   
   as a result, it turns out that all players will receive the player's userinfo much longer than the server parsed. The remaining part of the string will be perceived by the client as part of the traffic from the server and will be executed as engine/user messages.