From 18dc3e6bb2c061801f689f84a7286fe4e90db202 Mon Sep 17 00:00:00 2001 From: Andrei Smirnov Date: Wed, 20 May 2026 11:37:39 +0300 Subject: [PATCH] core: ssz offset validation (#4529) --- core/proto.go | 22 ++++++- core/proto_internal_test.go | 111 ++++++++++++++++++++++++++++++++++++ core/ssz.go | 4 +- 3 files changed, 132 insertions(+), 5 deletions(-) diff --git a/core/proto.go b/core/proto.go index 97be6c0c4c..d8e3638739 100644 --- a/core/proto.go +++ b/core/proto.go @@ -52,8 +52,7 @@ func ParSignedDataFromProto(typ DutyType, data *pbv1.ParSignedData) (_ ParSigned // This is to respect the technical possibility of unmarshalling to panic. // However, our protobuf generated types do not have custom marshallers that may panic. if r := recover(); r != nil { - rowStr := fmt.Sprintf("%v", r) - oerr = errors.Wrap(errors.New(rowStr), "panic recovered") + oerr = recoverPanicErr(r) } }() @@ -242,7 +241,13 @@ func UnsignedDataSetToProto(set UnsignedDataSet) (*pbv1.UnsignedDataSet, error) } // UnsignedDataSetFromProto returns the set from a protobuf. -func UnsignedDataSetFromProto(typ DutyType, set *pbv1.UnsignedDataSet) (UnsignedDataSet, error) { +func UnsignedDataSetFromProto(typ DutyType, set *pbv1.UnsignedDataSet) (_ UnsignedDataSet, oerr error) { + defer func() { + if r := recover(); r != nil { + oerr = recoverPanicErr(r) + } + }() + if set == nil || len(set.GetSet()) == 0 { return nil, errors.New("invalid unsigned data set fields", z.Any("set", set)) } @@ -261,6 +266,17 @@ func UnsignedDataSetFromProto(typ DutyType, set *pbv1.UnsignedDataSet) (Unsigned return resp, nil } +func recoverPanicErr(r any) error { + var err error + if recoveredErr, ok := r.(error); ok { + err = recoveredErr + } else { + err = errors.New(fmt.Sprint(r)) + } + + return errors.Wrap(err, "panic recovered") +} + // marshal marshals the given value into bytes, either as SSZ if supported by the type (and if enabled) or as json. func marshal(v any) ([]byte, error) { // First try SSZ diff --git a/core/proto_internal_test.go b/core/proto_internal_test.go index b6a3fdc32a..1960b243e6 100644 --- a/core/proto_internal_test.go +++ b/core/proto_internal_test.go @@ -5,11 +5,17 @@ package core import ( "encoding/hex" "encoding/json" + stderrors "errors" "testing" eth2v1 "github.com/attestantio/go-eth2-client/api/v1" eth2p0 "github.com/attestantio/go-eth2-client/spec/phase0" + ssz "github.com/ferranbt/fastssz" "github.com/stretchr/testify/require" + + "github.com/obolnetwork/charon/app/errors" + pbv1 "github.com/obolnetwork/charon/core/corepb/v1" + "github.com/obolnetwork/charon/eth2util" ) // TestMarshal tests the marshal() internal function. @@ -94,6 +100,111 @@ func TestMarshal(t *testing.T) { } } +// TestUnsignedDataSetFromProtoMalformedSSZOffset verifies that malformed SSZ bytes with an +// out-of-bounds offset field return an error instead of panicking. +func TestUnsignedDataSetFromProtoMalformedSSZOffset(t *testing.T) { + // versionedBlindedOffset = 13: 8 (version uint64) + 1 (blinded uint8) + 4 (offset uint32). + // A 13-byte buffer with the offset field encoding 14 caused slice bounds [14:13] before the fix. + proposerBuf := []byte{ + 0, 0, 0, 0, 0, 0, 0, 0, // version = Phase0 (0) + 0, // blinded = false + 14, 0, 0, 0, // offset = 14, but len(buf) = 13 + } + + // versionedOffset = 12: 8 (version uint64) + 4 (offset uint32). + // A 12-byte buffer with the offset field encoding 13 caused slice bounds [13:12] before the fix. + aggregatorBuf := []byte{ + 0, 0, 0, 0, 0, 0, 0, 0, // version = Phase0 (0) + 13, 0, 0, 0, // offset = 13, but len(buf) = 12 + } + + t.Run("versioned_blinded_helper_returns_offset_error", func(t *testing.T) { + _, _, err := unmarshalSSZVersionedBlinded(proposerBuf, func(eth2util.DataVersion, bool) (sszType, error) { + t.Fatal("valFunc must not be called for an out-of-bounds offset") + + return nil, stderrors.New("unexpected valFunc call") + }) + require.Error(t, err) + require.True(t, errors.Is(err, ssz.ErrOffset), "error must wrap ssz.ErrOffset: %v", err) + require.NotContains(t, err.Error(), "panic recovered") + }) + + t.Run("versioned_helper_returns_offset_error", func(t *testing.T) { + _, err := unmarshalSSZVersioned(aggregatorBuf, func(eth2util.DataVersion) (sszType, error) { + t.Fatal("valFunc must not be called for an out-of-bounds offset") + + return nil, stderrors.New("unexpected valFunc call") + }) + require.Error(t, err) + require.True(t, errors.Is(err, ssz.ErrOffset), "error must wrap ssz.ErrOffset: %v", err) + require.NotContains(t, err.Error(), "panic recovered") + }) + + tests := []struct { + name string + duty DutyType + buf []byte + }{ + { + name: "proposer/versioned_blinded_offset_oob", + duty: DutyProposer, + buf: proposerBuf, + }, + { + name: "aggregator/versioned_offset_oob", + duty: DutyAggregator, + buf: aggregatorBuf, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + set := &pbv1.UnsignedDataSet{ + Set: map[string][]byte{"pk": tt.buf}, + } + + _, err := UnsignedDataSetFromProto(tt.duty, set) + require.Error(t, err) + require.NotContains(t, err.Error(), "panic recovered") + }) + } +} + +func TestRecoverPanicErr(t *testing.T) { + sentinel := stderrors.New("sentinel") + + tests := []struct { + name string + recovered any + is error + contains string + }{ + { + name: "error", + recovered: sentinel, + is: sentinel, + contains: "sentinel", + }, + { + name: "string", + recovered: "plain panic", + contains: "plain panic", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := recoverPanicErr(tt.recovered) + require.ErrorContains(t, err, "panic recovered") + require.ErrorContains(t, err, tt.contains) + + if tt.is != nil { + require.ErrorIs(t, err, tt.is) + } + }) + } +} + // TestUnmarshal tests the unmarshal() internal function. // unmarshal() tries SSZ first (if the type implements ssz.Unmarshaler), // falling back to JSON when SSZ fails and the data starts with '{'. diff --git a/core/ssz.go b/core/ssz.go index 517efc2a64..4087cb8275 100644 --- a/core/ssz.go +++ b/core/ssz.go @@ -734,7 +734,7 @@ func unmarshalSSZVersionedBlinded(buf []byte, valFunc func(eth2util.DataVersion, // Offset (2) 'Value' o1 := ssz.ReadOffset(buf[9:13]) - if versionedBlindedOffset > o1 { + if versionedBlindedOffset > o1 || o1 > uint64(len(buf)) { return "", false, errors.Wrap(ssz.ErrOffset, "sszValFromVersion offset", z.Any("version", version), z.Bool("blinded", blinded)) } @@ -801,7 +801,7 @@ func unmarshalSSZVersioned(buf []byte, valFunc func(eth2util.DataVersion) (sszTy // Offset (1) 'Value' o1 := ssz.ReadOffset(buf[8:12]) - if versionedOffset > o1 { + if versionedOffset > o1 || o1 > uint64(len(buf)) { return "", errors.Wrap(ssz.ErrOffset, "sszValFromVersion offset", z.Any("version", version)) }