From 112f91777520cdc9fe0b14d4ea890d3255593740 Mon Sep 17 00:00:00 2001 From: tamirms Date: Wed, 11 Mar 2026 12:33:08 -0500 Subject: [PATCH 1/2] decoder: add MaxOutputBytes option for cumulative decoded output size limits Add a MaxOutputBytes field to DecodeOptions that tracks cumulative decoded output size across a single decode operation. Before each allocation (array element, union arm, optional field, opaque data), the size is added to a running total; if it exceeds MaxOutputBytes the decode is aborted with ErrOutputBytesExceeded. Key changes: - Add TrackOutputBytes method and TrackOutputBytesOf[T] generic helper - Cap array pre-allocation at 256 elements, growing via append beyond that - Track allocations in DecodeFixedOpaque, decodeArray, decodeUnion, decodeMap, and allocPtrIfNil - Bump minimum Go version to 1.25 for generics support Co-Authored-By: Claude Opus 4.6 --- .github/workflows/build.yml | 5 +- go.mod | 2 +- gotest.sh | 6 +- xdr3/decode.go | 139 +++++++++++++++++++++++++++++++----- xdr3/decode_limits_test.go | 71 ++++++++++++++++++ 5 files changed, 197 insertions(+), 26 deletions(-) create mode 100644 xdr3/decode_limits_test.go diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 2560488..e13c125 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -10,11 +10,10 @@ jobs: name: Build strategy: matrix: - go: ["1.20", "1.21"] + go: ["1.25", "1.26"] runs-on: ubuntu-latest container: golang:${{ matrix.go }}-bookworm steps: - run: go install golang.org/x/tools/cmd/goimports@latest - - run: go install golang.org/x/lint/golint@latest - - uses: actions/checkout@v1 + - uses: actions/checkout@v4 - run: ./gotest.sh diff --git a/go.mod b/go.mod index edd23af..ce9ce29 100644 --- a/go.mod +++ b/go.mod @@ -1,3 +1,3 @@ module github.com/stellar/go-xdr -go 1.12 +go 1.25 diff --git a/gotest.sh b/gotest.sh index 8fb06cc..da98cb8 100755 --- a/gotest.sh +++ b/gotest.sh @@ -1,13 +1,11 @@ #!/bin/bash # The script does automatic checking on a Go package and its sub-packages, including: # 1. goimports (https://golang.org/x/tools/cmd/goimports) -# 2. golint (https://github.com/golang/lint) -# 3. go vet (https://golang.org/cmd/vet) -# 4. test coverage (https://blog.golang.org/cover) +# 2. go vet (https://golang.org/cmd/vet) +# 3. test coverage (https://blog.golang.org/cover) set -ex test -z "$(goimports -l -w .)" -test -z "$(golint ./... )" go vet ./... go test -covermode=atomic -race ./... diff --git a/xdr3/decode.go b/xdr3/decode.go index 1c8f383..cd76842 100644 --- a/xdr3/decode.go +++ b/xdr3/decode.go @@ -17,12 +17,14 @@ package xdr import ( + "errors" "fmt" "io" "math" "reflect" "strconv" "time" + "unsafe" ) const maxInt32 = math.MaxInt32 @@ -47,6 +49,16 @@ type DecodeOptions struct { // the provided io.Reader implements Len() (e.g. strings.Reader, bytes.Reader and bytes.Buffer do). // Otherwise, no sanity checks will be done. MaxInputLen int + + // MaxOutputBytes is an approximate limit on cumulative decoded output size + // (in bytes) across a single decode operation. The decoder tracks the logical + // sizes of decoded values (array elements, union arms, optional fields, opaque + // data) and aborts if the total exceeds MaxOutputBytes. This is a best-effort + // bound: internal copies and over-allocation by the Go runtime may cause + // actual heap usage to be somewhat higher than this limit. + // + // If set to 0, no output size limit is enforced (default). + MaxOutputBytes int64 } // DefaultDecodeOptions are the default decoding options. @@ -125,10 +137,12 @@ type lenLeft interface { // won't work. type Decoder struct { // used to minimize heap allocations during decoding - scratchBuf [8]byte - r io.Reader - l lenLeft - maxDepth uint + scratchBuf [8]byte + r io.Reader + l lenLeft + maxDepth uint + maxOutputBytes int64 + outputBytes int64 } // readerLenWrapper wraps a reader an initial length and provides a Len() method indicating @@ -164,17 +178,18 @@ func NewDecoderWithOptions(r io.Reader, options DecodeOptions) *Decoder { if maxDepth < 1 { maxDepth = DecodeDefaultMaxDepth } + mob := options.MaxOutputBytes if l, ok := r.(lenLeft); ok { - return &Decoder{r: r, l: l, maxDepth: maxDepth} + return &Decoder{r: r, l: l, maxDepth: maxDepth, maxOutputBytes: mob} } if options.MaxInputLen > 0 { rlw := &readerLenWrapper{ inner: r, initialLen: options.MaxInputLen, } - return &Decoder{r: rlw, l: rlw, maxDepth: maxDepth} + return &Decoder{r: rlw, l: rlw, maxDepth: maxDepth, maxOutputBytes: mob} } - return &Decoder{r: r, l: nil, maxDepth: options.MaxDepth} + return &Decoder{r: r, l: nil, maxDepth: maxDepth, maxOutputBytes: mob} } // DecodeInt treats the next 4 bytes as an XDR encoded integer and returns the @@ -387,6 +402,14 @@ func (d *Decoder) DecodeDouble() (float64, int, error) { // RFC Section 4.9 - Fixed-Length Opaque Data // Fixed-length uninterpreted data zero-padded to a multiple of four func (d *Decoder) DecodeFixedOpaque(size int32) ([]byte, int, error) { + if size < 0 { + err := unmarshalError("DecodeFixedOpaque", ErrBadArguments, + "negative size", size, nil) + return nil, 0, err + } + if err := d.TrackOutputBytes(int64(size)); err != nil { + return nil, 0, err + } out := make([]byte, size) n, err := d.DecodeFixedOpaqueInplace(out) if err != nil { @@ -588,15 +611,11 @@ func (d *Decoder) decodeArray(v reflect.Value, ignoreOpaque bool, maxSize int, m return n, err } - // Allocate storage for the slice elements (the underlying array) if - // existing slice does not have enough capacity. sliceLen := int(dataLen) - if v.Cap() < sliceLen { - v.Set(reflect.MakeSlice(v.Type(), sliceLen, sliceLen)) - } - v.SetLen(sliceLen) // Treat []byte (byte is alias for uint8) as opaque data unless ignored. + // DecodeFixedOpaque handles both tracking and allocation, so we skip + // pre-allocation here — SetBytes replaces the backing array directly. if !ignoreOpaque && v.Type().Elem().Kind() == reflect.Uint8 { data, n2, err := d.DecodeFixedOpaque(int32(sliceLen)) n += n2 @@ -607,13 +626,47 @@ func (d *Decoder) decodeArray(v reflect.Value, ignoreOpaque bool, maxSize int, m return n, nil } - // Decode each slice element. - for i := 0; i < sliceLen; i++ { - n2, err := d.decode(v.Index(i), 0, maxDepth) - n += n2 - if err != nil { + // Cap pre-allocation to avoid memory amplification from untrusted inputs. + // The array length check above compares element count against remaining + // input bytes, but each element may be much larger in memory than on the + // wire. For large arrays, capping initial allocation and growing via + // append ensures memory usage is proportional to data actually decoded. + // NOTE: This constant is also used in the xdrgen Go code generator + // (lib/xdrgen/generators/go.rb) for generated DecodeFrom methods. + // Keep them in sync. + const maxPrealloc = 256 + elemSize := int64(v.Type().Elem().Size()) + if sliceLen <= maxPrealloc { + // Small arrays: track total upfront, then pre-allocate and decode. + if err := d.TrackOutputBytes(elemSize * int64(sliceLen)); err != nil { return n, err } + if v.Cap() < sliceLen { + v.Set(reflect.MakeSlice(v.Type(), sliceLen, sliceLen)) + } + v.SetLen(sliceLen) + for i := 0; i < sliceLen; i++ { + n2, err := d.decode(v.Index(i), 0, maxDepth) + n += n2 + if err != nil { + return n, err + } + } + } else { + // Large arrays: cap initial allocation, track and grow per element. + v.Set(reflect.MakeSlice(v.Type(), 0, maxPrealloc)) + zeroElem := reflect.Zero(v.Type().Elem()) + for i := 0; i < sliceLen; i++ { + if err := d.TrackOutputBytes(elemSize); err != nil { + return n, err + } + v.Set(reflect.Append(v, zeroElem)) + n2, err := d.decode(v.Index(i), 0, maxDepth) + n += n2 + if err != nil { + return n, err + } + } } return n, nil } @@ -675,6 +728,10 @@ func (d *Decoder) decodeUnion(v reflect.Value, maxDepth uint) (int, error) { vv := v.FieldByName(arm) vvet := vv.Type().Elem() + // Track the heap allocation for the pointer-typed union arm before allocating. + if err := d.TrackOutputBytes(int64(vvet.Size())); err != nil { + return n, err + } vv.Set(reflect.New(vvet)) field, ok := v.Type().FieldByName(arm) @@ -823,7 +880,12 @@ func (d *Decoder) decodeMap(v reflect.Value, maxDepth uint) (int, error) { // Decode each key and value according to their type. keyType := vt.Key() elemType := vt.Elem() + keySize := int64(keyType.Size()) + elemSize := int64(elemType.Size()) for i := uint32(0); i < dataLen; i++ { + if err := d.TrackOutputBytes(keySize); err != nil { + return n, err + } key := reflect.New(keyType).Elem() n2, err := d.decode(key, 0, maxDepth) n += n2 @@ -831,6 +893,9 @@ func (d *Decoder) decodeMap(v reflect.Value, maxDepth uint) (int, error) { return n, err } + if err := d.TrackOutputBytes(elemSize); err != nil { + return n, err + } val := reflect.New(elemType).Elem() n2, err = d.decode(val, 0, maxDepth) n += n2 @@ -1109,6 +1174,9 @@ func (d *Decoder) allocPtrIfNil(v *reflect.Value) error { } if isNil { vet := v.Type().Elem() + if err := d.TrackOutputBytes(int64(vet.Size())); err != nil { + return err + } v.Set(reflect.New(vet)) } return nil @@ -1182,3 +1250,38 @@ func (d *Decoder) InputLen() (int, bool) { } return d.l.Len(), true } + +// Sentinel errors for TrackOutputBytes — using errors.New instead of +// fmt.Errorf keeps the function small enough for the Go compiler to inline. +// Callers wrap these with fmt.Errorf to add context. +var ( + ErrOutputBytesExceeded = errors.New("output bytes limit exceeded") + ErrNegativeTrackingSize = errors.New("negative tracking size") +) + +// TrackOutputBytes adds size to the cumulative decoded output byte count and +// returns an error if MaxOutputBytes has been exceeded. Generated and +// reflection-based decoders call this before each allocation (array element, +// union arm, optional field, opaque data, string) to bound memory +// amplification from untrusted input. +func (d *Decoder) TrackOutputBytes(size int64) error { + if d.maxOutputBytes <= 0 { + return nil + } + if size < 0 { + return ErrNegativeTrackingSize + } + d.outputBytes += size + if d.outputBytes > d.maxOutputBytes || d.outputBytes < 0 { + return ErrOutputBytesExceeded + } + return nil +} + +// TrackOutputBytesOf is a generic helper that calls TrackOutputBytes with +// unsafe.Sizeof(*new(T)), which the compiler resolves to a constant at +// compile time. This keeps the unsafe import contained in go-xdr — +// generated code calls this function without needing to import unsafe. +func TrackOutputBytesOf[T any](d *Decoder) error { + return d.TrackOutputBytes(int64(unsafe.Sizeof(*new(T)))) +} diff --git a/xdr3/decode_limits_test.go b/xdr3/decode_limits_test.go new file mode 100644 index 0000000..4c9630d --- /dev/null +++ b/xdr3/decode_limits_test.go @@ -0,0 +1,71 @@ +package xdr + +import ( + "bytes" + "encoding/binary" + "testing" + "unsafe" +) + +// wideStruct has a large in-memory footprint (256 bytes) relative to its +// minimal XDR wire representation. +type wideStruct struct { + F0, F1, F2, F3, F4, F5, F6, F7 int64 + F8, F9, F10, F11, F12, F13, F14, F15 int64 + F16, F17, F18, F19, F20, F21, F22, F23 int64 + F24, F25, F26, F27, F28, F29, F30, F31 int64 +} + +// makeArrayPayload creates an XDR-encoded variable-length array header +// followed by zero-filled element data. +func makeArrayPayload(payloadSize int) []byte { + payload := make([]byte, payloadSize) + declaredLen := uint32(payloadSize - 4) + binary.BigEndian.PutUint32(payload[0:4], declaredLen) + return payload +} + +func TestMaxOutputBytes(t *testing.T) { + payloadSize := 100000 + payload := makeArrayPayload(payloadSize) + structSize := int64(unsafe.Sizeof(wideStruct{})) // 256 + + t.Run("unlimited", func(t *testing.T) { + var result []wideStruct + reader := bytes.NewReader(payload) + _, err := UnmarshalWithOptions(reader, &result, DecodeOptions{}) + if len(result) == 0 { + t.Errorf("expected some decoded elements with no limit, err=%v", err) + } + }) + + t.Run("exceeded", func(t *testing.T) { + budget := int64(256) * structSize // 65536 bytes + var result []wideStruct + reader := bytes.NewReader(payload) + _, err := UnmarshalWithOptions(reader, &result, DecodeOptions{ + MaxOutputBytes: budget, + }) + if err == nil { + t.Error("expected error when output byte limit exceeded") + } + maxExpected := int(budget/structSize) + 1 + if len(result) > maxExpected { + t.Errorf("decoded %d elements, expected at most %d", len(result), maxExpected) + } + }) + + t.Run("not_reached", func(t *testing.T) { + // Budget larger than what the payload can produce — decode + // should succeed without hitting the limit. + budget := int64(3000) * structSize + var result []wideStruct + reader := bytes.NewReader(payload) + _, err := UnmarshalWithOptions(reader, &result, DecodeOptions{ + MaxOutputBytes: budget, + }) + if len(result) == 0 { + t.Errorf("expected some decoded elements, err=%v", err) + } + }) +} From 7dd2b08c75136e854d41ba78a2ac3629b5dab070 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Mar 2026 18:46:46 +0000 Subject: [PATCH 2/2] Initial plan