Fix global buffer overflow in SharedParse()#1165
Open
belomaxorka wants to merge 3 commits into
Open
Conversation
SharedParse() copies each parsed token into the fixed-size global buffer s_shared_token[1500] without any bounds checking. A single token (a quoted string or an unquoted word) longer than 1499 bytes overflows the buffer and corrupts adjacent globals, which can crash the server or corrupt state. The buffer is fed from data files parsed via LOAD_FILE_FOR_ME (botprofile.db, BotChatter.db, mapcycle.txt, tutor messages) and from the default-weapon cvars, so a malformed or malicious file/config triggers the overflow. Bound all three write sites to sizeof(s_shared_token) - 1, matching the existing pattern already used in CBasePlayer::ParseAutoBuyString(). Well-formed input is unaffected; only pathologically long tokens are now truncated. Add unit tests covering oversized unquoted/quoted tokens and normal token parsing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SharedParse()
cppunitlite CHECK takes (message, condition); single-argument form broke the Tests build on CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
priorityToken[32] is filled from the client-supplied autobuy string (m_autoBuyString) without any bounds checking. A single autobuy token longer than 31 characters overflows the buffer and corrupts adjacent stack state during career-mode auto-buy. Bound the token copy to sizeof(priorityToken) - 1, matching the pattern in ParseAutoBuyString(), and skip the rest of an oversized token so it is not re-split into a spurious priority token on the next iteration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
SharedParse()copies each parsed token into the fixed-size global buffers_shared_token[1500]without any bounds checking, so a single token longer than 1499 bytes overflows the buffer.Why (Root cause)
The three token-writing sites in
SharedParse()- quoted string, single special character, and regular word - incrementlenand writes_shared_token[len]with no check against the buffer size:Any input that contains one token (quoted or unquoted) longer than the buffer overruns it and corrupts the globals that follow, which can crash the server or corrupt parser state.
The buffer is fed from data files parsed through
LOAD_FILE_FOR_ME-botprofile.db,BotChatter.db,mapcycle.txt, tutor messages - and from thect/t_default_weapons_*cvars, so a malformed or maliciously crafted file/config is enough to trigger it. The sibling parserCBasePlayer::ParseAutoBuyString()already bounds its copy correctly;SharedParse()simply misses the same guard.How it works
Bound each of the three writes to
sizeof(s_shared_token) - 1, mirroring the existing check inParseAutoBuyString(). Oversized tokens are safely truncated and still null-terminated, and parsing resumes from the correct position.Edge cases
'\0'terminator and the returned parse position intact, so the surrounding parse loops behave as before.Tested
unittests/shared_util_tests.cpp): oversized unquoted word, oversized quoted string, and a normal-token regression (primaryWeapon/"hello world"/,).s_shared_tokenbefore the fix (canary clobbered with0x4141414141414141), and is truncated to 1499 bytes after the fix.Edit: also fixed a global buffer overflow of the same class in
CBasePlayer::PrioritizeAutoBuyString().priorityToken[32]is filled from the client-supplied autobuy string without bounds checking, so a single autobuy token longer than 31 characters overflowed it during career-mode auto-buy. Bounded tosizeof(priorityToken) - 1matchingParseAutoBuyString(), and the rest of an oversized token is skipped so it is not re-split into a spurious priority token.