From 7042791e31ca2c4e3ff77fbffc1762514f76a5eb Mon Sep 17 00:00:00 2001 From: Preston Van Loon Date: Fri, 13 May 2022 02:02:43 -0500 Subject: [PATCH] fuzz: Fix off by one error in sparse merkle trie item insertion (#10668) * fuzz: Fix off by one error in sparse merkle trie item insertion * remove new line * Move validation to the proto constructor * fix build * Add a unit test because @nisdas is going to ask for it * fix up * gaz * Update container/trie/sparse_merkle.go * Update container/trie/sparse_merkle_test.go Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> Co-authored-by: nisdas --- beacon-chain/blockchain/BUILD.bazel | 2 + beacon-chain/blockchain/service_test.go | 5 +- beacon-chain/powchain/service.go | 7 +- container/trie/BUILD.bazel | 1 + container/trie/sparse_merkle.go | 44 ++++++----- container/trie/sparse_merkle_test.go | 73 ++++++++++++++++++- .../trie/sparse_merkle_trie_fuzz_test.go | 28 ++++--- ...z-97663548a1e2280a081745e5fa25c289f7121c0b | 4 + 8 files changed, 129 insertions(+), 35 deletions(-) create mode 100644 container/trie/testdata/fuzz/FuzzSparseMerkleTrie_Insert/fuzzbuzz-97663548a1e2280a081745e5fa25c289f7121c0b diff --git a/beacon-chain/blockchain/BUILD.bazel b/beacon-chain/blockchain/BUILD.bazel index f21e55b54..0452008a4 100644 --- a/beacon-chain/blockchain/BUILD.bazel +++ b/beacon-chain/blockchain/BUILD.bazel @@ -137,6 +137,7 @@ go_test( "//config/fieldparams:go_default_library", "//config/params:go_default_library", "//consensus-types/wrapper:go_default_library", + "//container/trie:go_default_library", "//encoding/bytesutil:go_default_library", "//proto/prysm/v1alpha1:go_default_library", "//testing/assert:go_default_library", @@ -188,6 +189,7 @@ go_test( "//beacon-chain/powchain/testing:go_default_library", "//config/params:go_default_library", "//consensus-types/wrapper:go_default_library", + "//container/trie:go_default_library", "//encoding/bytesutil:go_default_library", "//proto/prysm/v1alpha1:go_default_library", "//testing/assert:go_default_library", diff --git a/beacon-chain/blockchain/service_test.go b/beacon-chain/blockchain/service_test.go index 5b721e6ca..869c7a203 100644 --- a/beacon-chain/blockchain/service_test.go +++ b/beacon-chain/blockchain/service_test.go @@ -31,6 +31,7 @@ import ( "github.com/prysmaticlabs/prysm/config/params" "github.com/prysmaticlabs/prysm/consensus-types/interfaces" "github.com/prysmaticlabs/prysm/consensus-types/wrapper" + "github.com/prysmaticlabs/prysm/container/trie" "github.com/prysmaticlabs/prysm/encoding/bytesutil" ethpb "github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1" "github.com/prysmaticlabs/prysm/testing/assert" @@ -86,9 +87,11 @@ func setupBeaconChain(t *testing.T, beaconDB db.Database) *Service { bState, _ := util.DeterministicGenesisState(t, 10) pbState, err := v1.ProtobufBeaconState(bState.InnerStateUnsafe()) require.NoError(t, err) + mockTrie, err := trie.NewTrie(0) + require.NoError(t, err) err = beaconDB.SavePowchainData(ctx, ðpb.ETH1ChainData{ BeaconState: pbState, - Trie: ðpb.SparseMerkleTrie{}, + Trie: mockTrie.ToProto(), CurrentEth1Data: ðpb.LatestETH1Data{ BlockHash: make([]byte, 32), }, diff --git a/beacon-chain/powchain/service.go b/beacon-chain/powchain/service.go index 1c62c9a7f..3a3dc8034 100644 --- a/beacon-chain/powchain/service.go +++ b/beacon-chain/powchain/service.go @@ -767,9 +767,12 @@ func (s *Service) initializeEth1Data(ctx context.Context, eth1DataInDB *ethpb.ET if eth1DataInDB == nil { return nil } - s.depositTrie = trie.CreateTrieFromProto(eth1DataInDB.Trie) - s.chainStartData = eth1DataInDB.ChainstartData var err error + s.depositTrie, err = trie.CreateTrieFromProto(eth1DataInDB.Trie) + if err != nil { + return err + } + s.chainStartData = eth1DataInDB.ChainstartData if !reflect.ValueOf(eth1DataInDB.BeaconState).IsZero() { s.preGenesisState, err = v1.InitializeFromProto(eth1DataInDB.BeaconState) if err != nil { diff --git a/container/trie/BUILD.bazel b/container/trie/BUILD.bazel index 62489a5c7..fc91b8c58 100644 --- a/container/trie/BUILD.bazel +++ b/container/trie/BUILD.bazel @@ -13,6 +13,7 @@ go_library( "//encoding/bytesutil:go_default_library", "//math:go_default_library", "//proto/prysm/v1alpha1:go_default_library", + "@com_github_pkg_errors//:go_default_library", ], ) diff --git a/container/trie/sparse_merkle.go b/container/trie/sparse_merkle.go index 5507c8f05..54d6975c0 100644 --- a/container/trie/sparse_merkle.go +++ b/container/trie/sparse_merkle.go @@ -4,9 +4,9 @@ package trie import ( "bytes" "encoding/binary" - "errors" "fmt" + "github.com/pkg/errors" "github.com/prysmaticlabs/prysm/crypto/hash" "github.com/prysmaticlabs/prysm/encoding/bytesutil" "github.com/prysmaticlabs/prysm/math" @@ -29,7 +29,7 @@ func NewTrie(depth uint64) (*SparseMerkleTrie, error) { } // CreateTrieFromProto creates a Sparse Merkle Trie from its corresponding merkle trie. -func CreateTrieFromProto(trieObj *protodb.SparseMerkleTrie) *SparseMerkleTrie { +func CreateTrieFromProto(trieObj *protodb.SparseMerkleTrie) (*SparseMerkleTrie, error) { trie := &SparseMerkleTrie{ depth: uint(trieObj.Depth), originalItems: trieObj.OriginalItems, @@ -39,7 +39,29 @@ func CreateTrieFromProto(trieObj *protodb.SparseMerkleTrie) *SparseMerkleTrie { branches[i] = layer.Layer } trie.branches = branches - return trie + + if err := trie.validate(); err != nil { + return nil, errors.Wrap(err, "invalid sparse merkle trie") + } + + return trie, nil +} + +func (m *SparseMerkleTrie) validate() error { + if len(m.branches) == 0 { + return errors.New("no branches") + } + if len(m.branches[len(m.branches)-1]) == 0 { + return errors.New("invalid branches provided") + } + if m.depth >= uint(len(m.branches)) { + return errors.New("depth is greater than or equal to number of branches") + } + if m.depth >= 64 { + return errors.New("depth exceeds 64") // PowerOf2 would overflow. + } + + return nil } // GenerateTrieFromItems constructs a Merkle trie from a sequence of byte slices. @@ -82,10 +104,6 @@ func (m *SparseMerkleTrie) Items() [][]byte { // Spec Definition: // sha256(concat(node, self.to_little_endian_64(self.deposit_count), slice(zero_bytes32, start=0, len=24))) func (m *SparseMerkleTrie) HashTreeRoot() ([32]byte, error) { - if len(m.branches) == 0 || len(m.branches[len(m.branches)-1]) == 0 { - return [32]byte{}, errors.New("invalid branches provided to compute root") - } - enc := [32]byte{} depositCount := uint64(len(m.originalItems)) if len(m.originalItems) == 1 && bytes.Equal(m.originalItems[0], ZeroHashes[0][:]) { @@ -101,12 +119,6 @@ func (m *SparseMerkleTrie) Insert(item []byte, index int) error { if index < 0 { return fmt.Errorf("negative index provided: %d", index) } - if len(m.branches) == 0 { - return errors.New("invalid trie: no branches") - } - if m.depth > uint(len(m.branches)) { - return errors.New("invalid trie: depth is greater than number of branches") - } for index >= len(m.branches[0]) { m.branches[0] = append(m.branches[0], ZeroHashes[0][:]) } @@ -153,16 +165,10 @@ func (m *SparseMerkleTrie) MerkleProof(index int) ([][]byte, error) { if index < 0 { return nil, fmt.Errorf("merkle index is negative: %d", index) } - if len(m.branches) == 0 { - return nil, errors.New("invalid trie: no branches") - } leaves := m.branches[0] if index >= len(leaves) { return nil, fmt.Errorf("merkle index out of range in trie, max range: %d, received: %d", len(leaves), index) } - if m.depth > uint(len(m.branches)) { - return nil, errors.New("invalid trie: depth is greater than number of branches") - } merkleIndex := uint(index) proof := make([][]byte, m.depth+1) for i := uint(0); i < m.depth; i++ { diff --git a/container/trie/sparse_merkle_test.go b/container/trie/sparse_merkle_test.go index f0b12ba85..7d362431b 100644 --- a/container/trie/sparse_merkle_test.go +++ b/container/trie/sparse_merkle_test.go @@ -16,6 +16,68 @@ import ( "github.com/prysmaticlabs/prysm/testing/require" ) +func TestCreateTrieFromProto_Validation(t *testing.T) { + h := hash.Hash([]byte("hi")) + genValidLayers := func(num int) []*ethpb.TrieLayer { + l := make([]*ethpb.TrieLayer, num) + for i := 0; i < num; i++ { + l[i] = ðpb.TrieLayer{ + Layer: [][]byte{h[:]}, + } + } + return l + } + tests := []struct { + trie *ethpb.SparseMerkleTrie + errString string + }{ + { + trie: ðpb.SparseMerkleTrie{ + Layers: []*ethpb.TrieLayer{}, + Depth: 0, + }, + errString: "no branches", + }, + { + trie: ðpb.SparseMerkleTrie{ + Layers: []*ethpb.TrieLayer{ + { + Layer: [][]byte{h[:]}, + }, + { + Layer: [][]byte{h[:]}, + }, + { + Layer: [][]byte{}, + }, + }, + Depth: 2, + }, + errString: "invalid branches provided", + }, + { + trie: ðpb.SparseMerkleTrie{ + Layers: genValidLayers(3), + Depth: 12, + }, + errString: "depth is greater than or equal to number of branches", + }, + { + trie: ðpb.SparseMerkleTrie{ + Layers: genValidLayers(66), + Depth: 65, + }, + errString: "depth exceeds 64", + }, + } + for _, tt := range tests { + t.Run(tt.errString, func(t *testing.T) { + _, err := trie.CreateTrieFromProto(tt.trie) + require.ErrorContains(t, tt.errString, err) + }) + } +} + func TestMarshalDepositWithProof(t *testing.T) { items := [][]byte{ []byte("A"), @@ -52,7 +114,7 @@ func TestMarshalDepositWithProof(t *testing.T) { func TestMerkleTrie_MerkleProofOutOfRange(t *testing.T) { h := hash.Hash([]byte("hi")) - m := trie.CreateTrieFromProto(ðpb.SparseMerkleTrie{ + m, err := trie.CreateTrieFromProto(ðpb.SparseMerkleTrie{ Layers: []*ethpb.TrieLayer{ { Layer: [][]byte{h[:]}, @@ -61,11 +123,12 @@ func TestMerkleTrie_MerkleProofOutOfRange(t *testing.T) { Layer: [][]byte{h[:]}, }, { - Layer: [][]byte{}, + Layer: [][]byte{h[:]}, }, }, - Depth: 4, + Depth: 2, }) + require.NoError(t, err) if _, err := m.MerkleProof(6); err == nil { t.Error("Expected out of range failure, received nil", err) } @@ -128,6 +191,7 @@ func TestMerkleTrie_VerifyMerkleProof(t *testing.T) { []byte("G"), []byte("H"), } + m, err := trie.GenerateTrieFromItems(items, params.BeaconConfig().DepositContractTreeDepth) require.NoError(t, err) proof, err := m.MerkleProof(0) @@ -209,7 +273,8 @@ func TestRoundtripProto_OK(t *testing.T) { depositRoot, err := m.HashTreeRoot() require.NoError(t, err) - newTrie := trie.CreateTrieFromProto(protoTrie) + newTrie, err := trie.CreateTrieFromProto(protoTrie) + require.NoError(t, err) root, err := newTrie.HashTreeRoot() require.NoError(t, err) require.DeepEqual(t, depositRoot, root) diff --git a/container/trie/sparse_merkle_trie_fuzz_test.go b/container/trie/sparse_merkle_trie_fuzz_test.go index 1845fdb0e..4d03309eb 100644 --- a/container/trie/sparse_merkle_trie_fuzz_test.go +++ b/container/trie/sparse_merkle_trie_fuzz_test.go @@ -22,10 +22,10 @@ func FuzzSparseMerkleTrie_HashTreeRoot(f *testing.F) { Layer: [][]byte{h[:]}, }, { - Layer: [][]byte{}, + Layer: [][]byte{h[:]}, }, }, - Depth: 4, + Depth: 2, } b, err := proto.Marshal(pb) require.NoError(f, err) @@ -36,10 +36,13 @@ func FuzzSparseMerkleTrie_HashTreeRoot(f *testing.F) { if err := proto.Unmarshal(b, pb); err != nil { return } - _, err := trie.CreateTrieFromProto(pb).HashTreeRoot() + smt, err := trie.CreateTrieFromProto(pb) if err != nil { return } + if _, err := smt.HashTreeRoot(); err != nil { + return + } }) } @@ -54,10 +57,10 @@ func FuzzSparseMerkleTrie_MerkleProof(f *testing.F) { Layer: [][]byte{h[:]}, }, { - Layer: [][]byte{}, + Layer: [][]byte{h[:]}, }, }, - Depth: 4, + Depth: 2, } b, err := proto.Marshal(pb) require.NoError(f, err) @@ -68,10 +71,13 @@ func FuzzSparseMerkleTrie_MerkleProof(f *testing.F) { if err := proto.Unmarshal(b, pb); err != nil { return } - _, err := trie.CreateTrieFromProto(pb).MerkleProof(i) + smt, err := trie.CreateTrieFromProto(pb) if err != nil { return } + if _, err := smt.MerkleProof(i); err != nil { + return + } }) } @@ -86,10 +92,10 @@ func FuzzSparseMerkleTrie_Insert(f *testing.F) { Layer: [][]byte{h[:]}, }, { - Layer: [][]byte{}, + Layer: [][]byte{h[:]}, }, }, - Depth: 4, + Depth: 2, } b, err := proto.Marshal(pb) require.NoError(f, err) @@ -100,7 +106,11 @@ func FuzzSparseMerkleTrie_Insert(f *testing.F) { if err := proto.Unmarshal(b, pb); err != nil { return } - if err := trie.CreateTrieFromProto(pb).Insert(item, i); err != nil { + smt, err := trie.CreateTrieFromProto(pb) + if err != nil { + return + } + if err := smt.Insert(item, i); err != nil { return } }) diff --git a/container/trie/testdata/fuzz/FuzzSparseMerkleTrie_Insert/fuzzbuzz-97663548a1e2280a081745e5fa25c289f7121c0b b/container/trie/testdata/fuzz/FuzzSparseMerkleTrie_Insert/fuzzbuzz-97663548a1e2280a081745e5fa25c289f7121c0b new file mode 100644 index 000000000..7c5460c22 --- /dev/null +++ b/container/trie/testdata/fuzz/FuzzSparseMerkleTrie_Insert/fuzzbuzz-97663548a1e2280a081745e5fa25c289f7121c0b @@ -0,0 +1,4 @@ +go test fuzz v1 +[]byte("\b\x03\x12\"2 00000000000000000000000000000000\x12\"2 00000000000000000000000000000000\x12\x00") +[]byte("") +int(0)