Skip to content

check wire-supplied size in simple json ReadMapBegin#3599

Merged
Jens-G merged 2 commits into
apache:masterfrom
dxbjavid:simple-json-map-size-check
Jun 17, 2026
Merged

check wire-supplied size in simple json ReadMapBegin#3599
Jens-G merged 2 commits into
apache:masterfrom
dxbjavid:simple-json-map-size-check

Conversation

@dxbjavid

Copy link
Copy Markdown
Contributor

TSimpleJSONProtocol.ReadMapBegin reads the map size off the wire into iSize, but the following checkSizeForProtocol call is passed int32(size) where size is the named return value that is still zero at that point, so the MaxMessageSize limit is always tested against 0 and the real size is never bounded before it is returned. A peer can therefore declare an oversized map and have it slip past the configured limit, which then feeds the map preallocation in generated read code. The sibling ParseElemListBegin used for lists and sets already validates the value it read off the wire, so this just points the map check at iSize to match it.

@dxbjavid dxbjavid requested a review from fishy as a code owner June 17, 2026 09:53
@mergeable mergeable Bot added the golang Pull requests that update Go code label Jun 17, 2026
@Jens-G

Jens-G commented Jun 17, 2026

Copy link
Copy Markdown
Member

Did you look at list/set as well?

@Jens-G

Jens-G commented Jun 17, 2026

Copy link
Copy Markdown
Member

Code review

Found 1 issue:

  1. The PR body describes the bypass mechanism in detail. Per AGENTS.md §6, changes touching serialization bounds must use neutral, functional language in all public-facing text (PR descriptions, commit messages, inline comments) — without describing exploitation paths. Vulnerability details should travel through the private ASF channel (security@apache.org).

thrift/AGENTS.md

Lines 89 to 95 in 983c813

When assisting with security-sensitive changes (transport size limits, TLS configuration,
authentication, serialization bounds, or anything flagged by the project's security team):
- **Never** describe the change as a security fix in public-facing text — commit messages,
PR titles, PR descriptions, or inline comments. Use neutral functional language:
*"add a configurable frame-size limit"* rather than *"fix DoS vulnerability"*.
Vulnerability details travel through the private ASF channel (`security@apache.org`);


Validation: list/set and other protocols

The same class of bug (checkSizeForProtocol receiving the wrong variable) was audited across the Go library:

  • simple_json_protocol.go ParseElemListBegin (list/set): checks int32(nSize) — the wire-read value — correctly. Not affected.
  • binary_protocol.go ReadMapBegin/ReadListBegin/ReadSetBegin: all compute totalMinSize from the wire-read size32. Not affected.
  • compact_protocol.go ReadMapBegin/ReadListBegin/ReadSetBegin: all use the wire-read values directly. Not affected.
  • json_protocol.go ReadMapBegin/ReadListBegin/ReadSetBegin: ReadMapBegin checks int32(iSize) directly; list/set delegate to ParseElemListBegin. Not affected.

The bug is isolated to ReadMapBegin in TSimpleJSONProtocol.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Client: go

Companion to THRIFT-6071 (same guard for ParseElemListBegin).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Jens-G

Jens-G commented Jun 17, 2026

Copy link
Copy Markdown
Member

Did you look at list/set as well?

Checked: ParseElemListBegin (which serves ReadListBegin/ReadSetBegin) already uses nSize — the wire-read value — so list/set are not affected by the wrong-variable issue this PR fixes.

However, both ReadMapBegin (after this fix) and ParseElemListBegin share a secondary issue: the int64 wire value is narrowed to int32 before checkSizeForProtocol. Values above math.MaxInt32 truncate silently; the narrowed value may pass the check while the full int64 is returned in size. I've pushed one additional commit to this branch that adds the range guard to ReadMapBegin; the same fix for ParseElemListBegin (list/set) is in PR #3601 (THRIFT-6071).

@Jens-G Jens-G merged commit 7d1c098 into apache:master Jun 17, 2026
87 of 89 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

golang Pull requests that update Go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants