Reject oversized snapshot token_count before allocating (distributed DoS)#546
Open
gigioneggiando wants to merge 1 commit into
Open
Reject oversized snapshot token_count before allocating (distributed DoS)#546gigioneggiando wants to merge 1 commit into
gigioneggiando wants to merge 1 commit into
Conversation
In the SNAPSHOT_LOAD_BEGIN worker path, token_count is only bounded to ~UINT32_MAX/4 by the header check (expected_token_bytes <= UINT32_MAX). The code then malloc()s token_count*4 bytes and runs the full token read loop, and only afterwards does the matching-state check reject token_count > ctx_size. An untrusted peer (the distributed protocol is unauthenticated) can therefore drive a multi-GB allocation and a full-length socket read before the request is rejected. Move the token_count > ctx_size test ahead of the allocation and read loop, discarding the body bytes, so an over-large count is rejected up front.
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
1 participant
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.
Summary
In the
SNAPSHOT_LOAD_BEGINworker path,begin.token_countis only bounded to~UINT32_MAX/4by the header check (expected_token_bytes <= UINT32_MAX). The code thenmalloc()stoken_count * 4bytes and runs the full token read loop, and only afterwards does the matching-state check rejecttoken_count > ctx_size.Because the distributed protocol is unauthenticated (any peer reaching the port is treated as a cluster member), an untrusted peer can drive a multi-GB allocation and a full-length socket read before the request is rejected.
Fix
Move the
token_count > ctx_sizetest ahead of the allocation and the read loop (discarding the body bytes, consistent with the header-validation path), so an over-large count is rejected up front. The existing later check is left in place for the other state fields.Verification
Source-confirmed and compile-checked (verified the patched translation unit builds cleanly under the ASan/UBSan flags). I did not build a runtime PoC for this one — triggering it needs a fully loaded worker plus a socket peer, which I couldn't stand up cheaply. The exact pre-existing line ordering (
malloc+ read loop before thetoken_count > ctx_sizecheck) is straightforward to confirm by eye.Found during a coordinated security review of ds4.