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) + } + }) + } +}