Semicritical issues, memory leaks, undebuggable code, and a request

[sapphonie]

Hello.

Please, either release the engine etc source code for SDK13 and let people fix issues by PRing things, or provide PDBs and fix bugs yourself, per #65. The "security concerns" that are stated in that PR are completely moot - cheaters have already pillaged this engine with reverse engineering and the 2020 code leak - it can not possibly get any worse than it already is. And, on the off chance that it does, I'd be happy to PR plenty of exploit fixes / cheat mitigation to this project, public domain, free of charge.

The lack of support of this abandoned black-box'd project from Valve is very frustrating. I should not have to be a skilled reverse engineer, devops person, AND a skilled coder to get this engine working in a vaguely modern way, e.g. GCC 9, C++17, MSVC 22, https://sentry.io crash reporting, crash dissection, undefined behavior finding, address sanitizing, etc etc etc. I've had to implement significant live (as in on game boot) reverse engineering (e.g. detouring and bytepatching) to fix several bugs, some of them being security issues for users, or otherwise critical, game ruining, server wrecking, etc.

Valve was built on modding, please don't let the official development kit for one of the best engines ever made be complete abandonware.

Here are a few bugs that should probably be fixed or looked at.

Previously reported on HackerOne, never fixed:

• Servers can set host_timescale to arbitrary or non numerical values to "brick" clients until they relaunch the game, as joining a different server will take an excessive amount of time due to it not being reset. Fixed here

Not previously reported;

• sv_tags leaks a spectacular amount of KVs and seems to have arbitrary memory stomping associated with it:
  
  Possibly related:

Mismatched free() / delete / delete []
   at 0x4044487: operator delete[](void*) (vg_replace_malloc.c:1103)
   by 0x5D01B4E: KeyValues::RemoveEverything() (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5D01AE9: KeyValues::RemoveEverything() (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5D01AE9: KeyValues::RemoveEverything() (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5D01BE5: KeyValues::deleteThis() (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5BF07EF: CBaseServer::RecalculateTags() (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5BF0978: SetMasterServerKeyValue(ISteamGameServer*, IConVar*) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x4A1C62A: CCvar::CallGlobalChangeCallbacks(ConVar*, char const*, float) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/libvstdlib_srv.so)
   by 0xB5CFE81: ConVar::ChangeStringValue(char const*, float) (convar.cpp:831)
   by 0xB5CFC1F: ConVar::InternalSetValue(char const*) (convar.cpp:785)
   by 0xB5CF4CD: ConVar::SetValue(char const*) (convar.cpp:989)
   by 0x5C06594: CCvarUtilities::SetDirect(ConVar*, char const*) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
 Address 0x121fba00 is 0 bytes inside a block of size 11 alloc'd
   at 0x4040660: malloc (vg_replace_malloc.c:381)
   by 0xB68B646: operator new(unsigned int) (in /home/sappho/sdkdev/tf2cTEST/tf2classic/bin/server.so)
   by 0xB68B687: operator new[](unsigned int) (in /home/sappho/sdkdev/tf2cTEST/tf2classic/bin/server.so)
   by 0xB5E7E78: KeyValues::SetString(char const*, char const*) (KeyValues.cpp:1643)
   by 0xAA81FBB: CMultiplayRules::GetTaggedConVarList(KeyValues*) (multiplay_gamerules.cpp:1887)
   by 0xAB6383C: CTFGameRules::GetTaggedConVarList(KeyValues*) (tf_gamerules.cpp:10463)
   by 0xB037CF6: CServerGameTags::GetTaggedConVarList(KeyValues*) (gameinterface.cpp:3509)
   by 0x5BF0717: CBaseServer::RecalculateTags() (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5BF0978: SetMasterServerKeyValue(ISteamGameServer*, IConVar*) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x4A1C62A: CCvar::CallGlobalChangeCallbacks(ConVar*, char const*, float) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/libvstdlib_srv.so)
   by 0xB5CFE81: ConVar::ChangeStringValue(char const*, float) (convar.cpp:831)
   by 0xB5CFC1F: ConVar::InternalSetValue(char const*) (convar.cpp:785)

• Clients can essentially fuzz CNetChan::ProcessPacket and cause server lag / stutter / crash (hackily fixed here, fixed in TF2 with net_chan_proc_ms or whatever the cvar)
• Clients can send a large amount of stringcmds to the server, causing server lag / stutter / crash, fixed in TF2 with sv_max_stringcmds_per_second or whatever the cvar, trivially implementable in SDK13 code, though this shouldn't be up to each mod to implement
• Clients can lag out etc. servers by sending invalid netchan->RequestFile requests as well as triggering netchan->CreateFragmentsFromFile, causing spam to console, fixed here
• Client KVs to linux dedicated servers are arbitrarily mangled, where the KeyValue name will be null/blank, the name of each string will be set to the value that it should be, and the value will be blank, and also sometimes this just will not occur. This makes ClientKeyValues unusable. Perhaps implement protobufs?
• Event names made with CreateEvent that are longer than 31 characters will get added to the internal engine event list, but will never get cleared due to being truncated later, causing an eventual unplayable server when the events list gets full (e.g. the server will stop sending events to all clients)
• SDK13 uses an ancient version of cURL (7.21? if I'm remembering correctly) that doubtless has its own list of critical security bugs fixed. Hackily fixed here, though I've yet to rip out the arcane fastdl code from engine that makes it take a ridiculous amount of time to download assets to clients.
• Spray code is easily exploitable to crash every client within the same visleaf of a malformed spray, see here, almost certainly abusable to write arbitrary data to client memory
• Countless client crashes in matsystem / vgui / engine that I can't be bothered to reverse engineer because they're completely undebuggable otherwise
• Miscellaneous undefined behavior that Valgrind whines about in SV_PackEntity :

320386639 errors in context 1763 of 2237:
Conditional jump or move depends on uninitialised value(s)
   at 0x5C2A2E0: SendTable_CalcDelta(SendTable const*, void const*, int, void const*, int, int*, int, int) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5CCE26A: SV_PackEntity(int, edict_t*, ServerClass*, CFrameSnapshot*) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5CD0154: CParallelProcessor<PackWork_t, CFuncJobItemProcessor<PackWork_t> >::Run(PackWork_t*, unsigned int, int, IThreadPool*) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5CCF066: PackEntities_Normal(int, CGameClient**, CFrameSnapshot*) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5CCF606: SV_ComputeClientPacks(int, CGameClient**, CFrameSnapshot*) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5CCBD15: CGameServer::SendClientMessages(bool) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5CCC010: SV_SendClientUpdates(bool, bool) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5CCC1D6: SV_Frame(bool) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5C42226: _Host_RunFrame_Server(bool) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5C42F40: _Host_RunFrame(float) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5C43A97: Host_RunFrame(float) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
   by 0x5C509C0: CHostState::State_Run(float) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)
 Uninitialised value was created by a stack allocation
   at 0x5CCE026: SV_PackEntity(int, edict_t*, ServerClass*, CFrameSnapshot*) (in /home/sappho/sdkdev/tf2cTEST/sdk2013/bin/engine_srv.so)

• Clients can twiddle about with their signonstate to bypass a significant number of IsConnected/IsActive checks while still wreaking havoc on servers, this is also a bug in TF2, vaguely fixed here by ignoring usercmds from non-signed on clients, similarly fixable in sdk13
• Clients can purposefully stall out signing onto steam and lie about being unauthenticated while still having an "unauthorized" SteamID, hackily and halfassedly fixed here which was originally stolen from here
• FOVs over 120 break skyboxes, hackily fixed here by someone else who I stole this code from (ficool2, I think?)
• ASLR is not enabled by default
• Several strictstrings violations
• Several hacky downcasts in mathlib and probably other places I forgot about
• SDL not enabled by default
• Particles lib is ancient and doesn't have support for modern compilers, requiring (pre compile) bytepatching

Apologies for posting links to reverse engineered detours and binary patches. I know reversing is against Steam's TOS etc, but I would prefer to not expose my players to malformed / insecure / buggy code, if I can help it.

Hope I can be of some help to Valve, or at the bare minimum illuminate some arcane issues for other SDK13 developers to avoid/fix, god bless their souls.

Thanks
Sappho

[sour-dani]

Please valve. Please help us!

[Rykita]

feels bad seeing all this effort that Valve will never even consider because they basically give 0 fucks about modding nowadays.