From 562538e0a59916af4d61cda95651d109563c606a Mon Sep 17 00:00:00 2001 From: tamirms Date: Sat, 11 Apr 2026 17:51:46 +0100 Subject: [PATCH] decoder: enforce MaxInputLen at the io.Reader boundary 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) --- xdr3/decode.go | 53 ++++++++++++---------- xdr3/decode_limits_test.go | 91 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 24 deletions(-) diff --git a/xdr3/decode.go b/xdr3/decode.go index 52d8ada..1863440 100644 --- a/xdr3/decode.go +++ b/xdr3/decode.go @@ -142,8 +142,11 @@ type lenLeft interface { // necessary in complex scenarios where automatic reflection-based decoding // won't work. type Decoder struct { - // used to minimize heap allocations during decoding + // scratchBuf minimizes heap allocations during primitive decodes. + // rlw is inline so that a decoder with MaxInputLen > 0 pays a single + // heap allocation (the Decoder itself) instead of two. scratchBuf [8]byte + rlw readerLenWrapper r io.Reader l lenLeft maxDepth uint @@ -151,24 +154,16 @@ type Decoder struct { memoryBytes int64 } -// readerLenWrapper wraps a reader an initial length and provides a Len() method indicating -// how much input is left +// readerLenWrapper adapts an io.LimitedReader to the lenLeft interface. +// io.LimitedReader's Read clamps p and returns io.EOF once N reaches zero, +// so enforcement happens at the io.Reader boundary regardless of how the +// underlying reader behaves. type readerLenWrapper struct { - inner io.Reader - readCount int - initialLen int + io.LimitedReader } func (l *readerLenWrapper) Len() int { - return l.initialLen - l.readCount -} - -func (l *readerLenWrapper) Read(p []byte) (int, error) { - n, err := l.inner.Read(p) - if n > 0 { - l.readCount += n - } - return n, err + return int(l.N) } // NewDecoder returns a Decoder that can be used to manually decode XDR data @@ -184,18 +179,28 @@ func NewDecoderWithOptions(r io.Reader, options DecodeOptions) *Decoder { if maxDepth < 1 { maxDepth = DecodeDefaultMaxDepth } - mob := options.MaxMemoryBytes - if l, ok := r.(lenLeft); ok { - return &Decoder{r: r, l: l, maxDepth: maxDepth, maxMemoryBytes: mob} - } + d := &Decoder{maxDepth: maxDepth, maxMemoryBytes: options.MaxMemoryBytes} if options.MaxInputLen > 0 { - rlw := &readerLenWrapper{ - inner: r, - initialLen: options.MaxInputLen, + // Always wrap: enforcement happens at the io.Reader boundary, not + // via trusting the caller's lenLeft implementation. Tighten the + // budget to the inner reader's reported remaining length when + // smaller, so InputLen() reflects actually-readable bytes. + budget := int64(options.MaxInputLen) + if l, ok := r.(lenLeft); ok { + if left := int64(l.Len()); left < budget { + budget = left + } } - return &Decoder{r: rlw, l: rlw, maxDepth: maxDepth, maxMemoryBytes: mob} + d.rlw.LimitedReader = io.LimitedReader{R: r, N: budget} + d.r = &d.rlw + d.l = &d.rlw + return d + } + d.r = r + if l, ok := r.(lenLeft); ok { + d.l = l } - return &Decoder{r: r, l: nil, maxDepth: maxDepth, maxMemoryBytes: mob} + return d } // DecodeInt treats the next 4 bytes as an XDR encoded integer and returns the diff --git a/xdr3/decode_limits_test.go b/xdr3/decode_limits_test.go index 9fef4c0..1430228 100644 --- a/xdr3/decode_limits_test.go +++ b/xdr3/decode_limits_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/binary" "errors" + "io" "testing" "unsafe" ) @@ -73,3 +74,93 @@ func TestMaxMemoryBytes(t *testing.T) { } }) } + +// passthroughReader is an io.Reader that does not implement lenLeft, so +// NewDecoderWithOptions cannot tighten the MaxInputLen budget from the inner +// reader's remaining length. +type passthroughReader struct { + r *bytes.Reader +} + +func (p *passthroughReader) Read(b []byte) (int, error) { return p.r.Read(b) } + +// TestMaxInputLenEnforced verifies that readerLenWrapper refuses reads past +// the configured MaxInputLen budget even when the underlying reader still has +// bytes available. +func TestMaxInputLenEnforced(t *testing.T) { + // 4 bytes to fill the budget plus 4 extras the wrapper must refuse. + payload := []byte{ + 0x00, 0x00, 0x00, 0x42, + 0xff, 0xff, 0xff, 0xff, + } + reader := &passthroughReader{r: bytes.NewReader(payload)} + + dec := NewDecoderWithOptions(reader, DecodeOptions{MaxInputLen: 4}) + + if _, _, err := dec.DecodeInt(); err != nil { + t.Fatalf("DecodeInt: %v", err) + } + + // Inner reader still holds 4 bytes; the wrapper must refuse them. + buf := make([]byte, 4) + n, err := dec.r.Read(buf) + if n != 0 || err != io.EOF { + t.Errorf("wrapper Read past budget: got (%d, %v), want (0, io.EOF)", n, err) + } + + if left, ok := dec.InputLen(); !ok || left != 0 { + t.Errorf("after budget exhaustion: InputLen=(%d,%v), want (0,true)", left, ok) + } +} + +// TestMaxInputLenDecodeStringBypass reproduces the reporter PoC end-to-end: +// a length prefix decoded past the MaxInputLen boundary must not reach the +// DecodeFixedOpaque allocation phase. +func TestMaxInputLenDecodeStringBypass(t *testing.T) { + payload := []byte{ + 0x00, 0x00, 0x00, 0x42, + 0x7f, 0xff, 0xff, 0xfb, + } + reader := &passthroughReader{r: bytes.NewReader(payload)} + dec := NewDecoderWithOptions(reader, DecodeOptions{MaxInputLen: 4}) + + if _, _, err := dec.DecodeInt(); err != nil { + t.Fatalf("DecodeInt: %v", err) + } + + if _, _, err := dec.DecodeString(0); err == nil { + t.Fatal("DecodeString: expected error, got nil") + } + + // Wrapper must refuse the length-prefix read, so the 4 bytes are still + // in the inner reader. If the wrapper leaked past budget, the decoder + // would have consumed them and proceeded to the allocation phase. + if got := reader.r.Len(); got != 4 { + t.Errorf("inner reader: got %d bytes remaining, want 4", got) + } +} + +// TestMaxInputLenBudget verifies that the wrapper's budget is the minimum of +// MaxInputLen and the inner reader's reported remaining length, regardless of +// which is tighter. InputLen() reports the actual initial budget. +func TestMaxInputLenBudget(t *testing.T) { + cases := []struct { + name string + payloadLen int + maxInputLen int + wantBudget int + }{ + {"MaxInputLen tighter than inner", 16, 4, 4}, + {"inner tighter than MaxInputLen", 4, 1 << 20, 4}, + {"equal", 8, 8, 8}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + reader := bytes.NewReader(make([]byte, tc.payloadLen)) + dec := NewDecoderWithOptions(reader, DecodeOptions{MaxInputLen: tc.maxInputLen}) + if left, ok := dec.InputLen(); !ok || left != tc.wantBudget { + t.Errorf("InputLen=(%d,%v), want (%d,true)", left, ok, tc.wantBudget) + } + }) + } +}