Fix (Re)HLDS exploit (Can't use keys or values with a ")

[IgnacioFDM]

Assume two clients connect with the following string

connect 48 12345678 \prot\2\unique\-1\raw\261578371d95a424925835ca44f82811 \cl_lw\1\cl_lc\1\*hltv\1\rate\10000\cl_updaterate\20\hspecs\0\hslots\0\hdelay\30\name\test"

Name will be parsed as test"

Then in SV_CheckForDuplicateNames, Info_SetValueForKey will fail because of the quotes, and an infinite loop will occur.

I also added a check for \. Altough it's technically impossible to appear, it never hurts to be extra careful with this kind of client input.

TODO (by others, sorry, really busy atm):

• Fix COM_Parse so that you can't inject quote marks. Other exploits may currently exist that also rely on this bug.

[In-line]

Merging for now, because bug was disclosed. Btw I think there are better ways to fix this

[IgnacioFDM]

How do you think it should be fixed?

[WPMGPRoSToTeMa]

Maybe it should be validated inside Host_SetInfo_f or Info_SetValueForKey.

[IgnacioFDM]

@WPMGPRoSToTeMa The existing validation in Info_SetValueForKey (actually Info_SetValueForStarKey) is what causes the infinite loop (since it returns instead of actually setting the new key, and thus causing an infinite loop when trying to remove duplicate names).

IMO the proper way to fix this is in COM_Parse (and related functions). I would still leave the current checks for extra safety.

[WPMGPRoSToTeMa]

@IgnacioFDM validation inside SV_CheckUserInfo is enough, code in SV_ExtractFromUserInfo is redundant. Validation for backslash is also redundant, it is validated here

rehlds/rehlds/engine/sv_main.cpp

Lines 2079 to 2085 in 19e3a5d

	i = Q_strlen(userinfo);
	if (i <= 4 || Q_strstr(userinfo, "\\\\") || userinfo[i - 1] == '\\')
	{
	SV_RejectConnection(adr, "Unknown HLTV client type.\n");
	return 0;
	}

only last value can possibly contain backslash.

[In-line]

@WPMGPRoSToTeMa We can add "" check in SV_ReplaceSpecialCharactersInName

[WPMGPRoSToTeMa]

The best way I think is to fix Info_IsValid function to also check quotes in keys/values.

[In-line]

@WPMGPRoSToTeMa And I think we should return boolean value indicating success or failure for Info_SetValueForKey.

[WPMGPRoSToTeMa]

@In-line how do you want to use it in existing code?

[In-line]

@WPMGPRoSToTeMa It's good for future usage.

[WPMGPRoSToTeMa]

@In-line well maybe for proper error handling.

[WPMGPRoSToTeMa]

Looks like checks for double backslash and for backslash at the end are also redundant because Info_IsValid already checks for empty keys and values. Double backslash is empty key + empty value or empty value + empty key, backslash at the end is empty key.

rehlds/rehlds/engine/sv_main.cpp

Lines 2079 to 2085 in 19e3a5d

	i = Q_strlen(userinfo);
	if (i <= 4 || Q_strstr(userinfo, "\\\\") || userinfo[i - 1] == '\\')
	{
	SV_RejectConnection(adr, "Unknown HLTV client type.\n");
	return 0;
	}

It is also interesting why quotes are prohibited, maybe because you can't input them into your userinfo via setinfo (and also initial userinfo, which is contained in connect packet, is enclosed by quotes).

[IgnacioFDM]

Remember that although I showed the exploit during connection, a user can change name (and other userinfo) while playing. Info_IsValid is not called in such case.

[WPMGPRoSToTeMa]

@IgnacioFDM

a user can change name (and other userinfo) while playing. Info_IsValid is not called in such case.

Info_SetValueForKey behaves well for in-game cases, because userinfo just isn't affected.
Let me explain:
Somebody changes his name via name "some\name", so the client sends to the server setinfo "name" "some\name".
The server receives that command and calls Host_SetInfo_f which calls Info_SetValueForKey and it just returns without changing nothing, like setinfo wasn't received by the server.

You can check it yourself by typing cmd setinfo name nick\name or cmd setinfo name nick"name.

[In-line]

@WPMGPRoSToTeMa Info_IsValid is horrible

[WPMGPRoSToTeMa]

@In-line what do you mean?

[In-line]

@WPMGPRoSToTeMa I just wanted to share my pain with somebody (⌣_⌣”)

[In-line]

@IgnacioFDM @WPMGPRoSToTeMa Please review #596

[WPMGPRoSToTeMa]

COM_Parse is also an interesting thing, because idk where single " is applicable.