decoder: enforce MaxInputLen at the io.Reader boundary by tamirms · Pull Request #31 · stellar/go-xdr

tamirms · 2026-04-11T11:27:13Z

Summary

Fixes a defect in xdr3's MaxInputLen option where an attacker-controlled length prefix could bypass the decoder's byte-budget check and reach an allocation of up to ~2 GB from a payload smaller than the configured limit. Also fixes a related issue where MaxInputLen was silently ignored when the caller passed a reader that already implemented Len().

The bugs

DecodeOptions.MaxInputLen is documented to cap how many bytes the decoder will read from its input. The previous implementation had two defects:

1. readerLenWrapper.Read did not enforce the budget. Its implementation was a pass-through to the underlying reader that incremented a readCount counter without clamping the caller's buffer. A read that crossed the initialLen boundary drove Len() negative (e.g., 4 - 8 = -4), and the downstream uint(dataLen) > uint(maxSize) checks in DecodeString, DecodeOpaque, and decodeArray treated the negative maxSize as ~1.8e19, bypassing the length-prefix guard and reaching DecodeFixedOpaque with an attacker-controlled size. Concretely: with MaxInputLen: 4 and an 8-byte payload containing a 4-byte int followed by a 4-byte length prefix of 0x7ffffffb, the decoder attempts a ~2 GB allocation despite being configured to read only 4 bytes.

2. MaxInputLen was silently ignored for readers that implement lenLeft. NewDecoderWithOptions checked the lenLeft interface before honoring MaxInputLen, so passing an in-memory reader (bytes.Reader, bytes.Buffer, strings.Reader) with a tighter MaxInputLen than the reader's own remaining length silently used the reader's full bound instead of the configured cap.

The fix

Redesign around io.LimitedReader:

readerLenWrapper value-embeds io.LimitedReader and exposes Len() via a trivial method returning int(l.N). io.LimitedReader.Read clamps p to the remaining budget before calling the inner reader and returns io.EOF at N <= 0. Enforcement happens at the io.Reader boundary with no trust relationship to any caller-provided type.
NewDecoderWithOptions always installs the wrapper when MaxInputLen > 0. The initial budget is min(MaxInputLen, inner.Len()) when the inner reader reports a lenLeft, so InputLen() reflects actually-readable remaining bytes rather than just the configured cap.
The wrapper is stored inline in the Decoder struct (rlw field). Construction with MaxInputLen pays a single heap allocation (the Decoder), matching the allocation profile of the previous code for Stellar's generated UnmarshalBinary hot path.

🤖 Generated with Claude Code

Copilot

Pull request overview

This PR fixes a MaxInputLen bypass in the xdr3 decoder by enforcing the input budget at the io.Reader boundary, preventing negative remaining-length underflow from turning size checks into no-ops.

Changes:

Replace the custom readerLenWrapper accounting with an io.LimitedReader-based wrapper and adapt it to the existing lenLeft interface.
Ensure reads past the configured input budget return io.EOF (rather than allowing the read to proceed and only tracking it).
Add regression tests for (1) direct read behavior past budget and (2) an end-to-end DecodeString bypass reproduction.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
xdr3/decode.go	Switch `readerLenWrapper` to embed `io.LimitedReader` and use its remaining-byte counter for `Len()` to prevent underflow/bypass.
xdr3/decode_limits_test.go	Add regression coverage for enforcing `MaxInputLen` at read-time and for preventing the reported `DecodeString` bypass path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

NewDecoderWithOptions checked for the lenLeft interface before honoring MaxInputLen, so passing an in-memory reader (bytes.Reader, bytes.Buffer, strings.Reader) with a tighter MaxInputLen than the reader's own remaining length silently ignored the option. Reorder the checks so MaxInputLen takes priority. Skip the wrapper only when the inner reader already reports a remaining bound that is at most as large as MaxInputLen, preserving the no-allocation fast path for the common case where MaxInputLen matches the full buffer size (Stellar's generated UnmarshalBinary methods). Also reword a stale test comment that referred to the old initialLen field rather than the user-facing MaxInputLen option, and add two regression tests: one for a bytes.Reader with a tighter MaxInputLen (previously not enforced), and one asserting the wrapper is skipped when the inner bound is already tight. Addresses review feedback on #31. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The MaxInputLen-vs-inner-reader skip check trusted l.Len() to return a non-negative value, but lenLeft is an unexported structural interface that any caller can satisfy. A type whose Len() returns a negative int would be treated as "tighter than MaxInputLen" (negative <= positive), the wrapper would be skipped, and the negative Len() would flow through InputLen() into the downstream uint(dataLen) > uint(left) comparisons — reintroducing the exact signed-to-unsigned bypass this PR is fixing, one layer out from readerLenWrapper. Add the left >= 0 guard so any lenLeft implementation returning negative falls back to the LimitedReader wrapper, which maintains a non-negative N invariantly. Add a regression test that passes a deliberately-buggy lenLeft implementation with Len() = -1 and asserts the wrapper is installed instead of used directly. Addresses review feedback on #31. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The xdr3 decoder's MaxInputLen option is documented to cap how many bytes the decoder will read from its input. The previous implementation had three defects: 1. readerLenWrapper's Read method was a pass-through that tracked readCount without clamping the caller's buffer. A read that crossed the initialLen boundary drove Len() negative, and the downstream uint(dataLen) > uint(maxSize) checks in DecodeString, DecodeOpaque, and decodeArray treated the negative maxSize as ~2^64, bypassing the length-prefix guard and reaching DecodeFixedOpaque with an attacker-controlled size. 2. NewDecoderWithOptions checked the lenLeft interface before honoring MaxInputLen, so passing an in-memory reader (bytes.Reader etc.) with a tighter MaxInputLen than the reader's own remaining length silently ignored the option. 3. Any scheme that relied on the caller's lenLeft implementation to enforce the bound was vulnerable to a custom reader whose Len() and Read() were decoupled -- Len() reports 0 but Read() returns bytes. Redesign around io.LimitedReader: - readerLenWrapper value-embeds io.LimitedReader and exposes Len() via a trivial method. io.LimitedReader.Read clamps p and returns EOF at N == 0, so enforcement happens at the io.Reader boundary with no trust relationship to any caller-provided type. - NewDecoderWithOptions always installs the wrapper when MaxInputLen is set. The initial budget is min(MaxInputLen, inner.Len()) when the inner reader reports a non-negative length, so InputLen() reflects actually-readable remaining bytes rather than just the configured cap. - The wrapper is stored inline in the Decoder struct (rlw field) so construction with MaxInputLen pays a single heap allocation, matching the allocation profile of the previous conditional-skip code for Stellar's generated UnmarshalBinary hot path. Regression tests cover the wrapper primitive, end-to-end reproduction of the reported DecodeString PoC, budget selection across MaxInputLen vs inner-reader combinations, enforcement against an adversarial reader with decoupled Len()/Read(), and negative-Len() tolerance in the tightening logic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Picks up stellar/go-xdr#31, which enforces MaxInputLen at the io.Reader boundary via io.LimitedReader. Decode semantics are unchanged for the generated UnmarshalBinary path (bytes.Reader length already bounds reads to len(inp)); microbenchmarks show a ~3% unmarshal overhead from the added wrapping. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings April 11, 2026 11:27

Copilot started reviewing on behalf of tamirms April 11, 2026 11:27 View session