From e80f323d91a666464a2eaa61e1ed4beb897566fc Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Mon, 4 Dec 2023 21:52:52 -0800 Subject: [PATCH] go: bullet-proof against nil dereferences + more fuzzers This change fixes a bunch of issues identified by Orijtech Inc's audit of ics23 which is a critical cosmos-sdk dependency and as per reports about the Dragonberry & Elderberry vulnerability reports, this package was put back on our radar to further audit and voila that uncovered some issues, some of which have beenfixed in this change. While here also added more fuzzers. Fixes #241 Fixes #242 Fixes #243 --- go/fuzz_test.go | 79 +++++++++++++++++++ go/ics23.go | 4 + go/ops.go | 13 +++ go/proof.go | 52 +++++++++--- go/proof_data_test.go | 24 +++--- .../09bebc2fc8d0a79b | 2 + .../19e35d361fe85847 | 2 + .../1f84363823f5c624 | 2 + .../a9ba9cba7c7724a0 | 2 + .../FuzzVerifyMembership/8093511184ad3e25 | 2 + .../FuzzVerifyMembership/99dd1125ca292163 | 2 + go/vectors_data_test.go | 24 +++--- 12 files changed, 172 insertions(+), 36 deletions(-) create mode 100644 go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/09bebc2fc8d0a79b create mode 100644 go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/19e35d361fe85847 create mode 100644 go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/1f84363823f5c624 create mode 100644 go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/a9ba9cba7c7724a0 create mode 100644 go/testdata/fuzz/FuzzVerifyMembership/8093511184ad3e25 create mode 100644 go/testdata/fuzz/FuzzVerifyMembership/99dd1125ca292163 diff --git a/go/fuzz_test.go b/go/fuzz_test.go index a4cfc7e6..89d5de01 100644 --- a/go/fuzz_test.go +++ b/go/fuzz_test.go @@ -1,6 +1,7 @@ package ics23 import ( + "bytes" "encoding/json" "os" "path/filepath" @@ -40,6 +41,38 @@ func FuzzExistenceProofCalculate(f *testing.F) { }) } +func FuzzExistenceProofCheckAgainstSpec(f *testing.F) { + if testing.Short() { + f.Skip("in -short mode") + } + + seedDataMap := CheckAgainstSpecTestData(f) + for _, seed := range seedDataMap { + if seed.IsErr { + // Erroneous data, skip it. + continue + } + if blob, err := json.Marshal(seed); err == nil { + f.Add(blob) + } + } + + // 2. Now run the fuzzer. + f.Fuzz(func(t *testing.T, fJSON []byte) { + pvs := new(CheckAgainstSpecTestStruct) + if err := json.Unmarshal(fJSON, pvs); err != nil { + return + } + if pvs.Proof == nil { + // No need checking from a nil ExistenceProof. + return + } + + ep, spec := pvs.Proof, pvs.Spec + _ = ep.CheckAgainstSpec(spec) + }) +} + var batchVectorDataSeeds []*BatchVectorData func init() { @@ -122,3 +155,49 @@ func FuzzCombineProofs(f *testing.F) { _, _ = CombineProofs(proofs) }) } + +type verifyJSON struct { + Proof *CommitmentProof + Ref *RefData + Spec *ProofSpec +} + +func FuzzVerifyMembership(f *testing.F) { + seeds := VectorsTestData() + + // VerifyMembership requires: + // Spec, ExistenceProof, CommitmentRootref, + for _, seed := range seeds { + proof, ref := LoadFile(f, seed.Dir, seed.Filename) + root, err := proof.Calculate() + if err != nil { + continue + } + if !bytes.Equal(ref.RootHash, root) { + continue + } + + if ref.Value == nil { + continue + } + + // Now serialize this value as a seed. + blob, err := json.Marshal(&verifyJSON{Proof: proof, Ref: ref, Spec: seed.Spec}) + if err == nil { + f.Add(blob) + } + } + + // 2. Now let's run the fuzzer. + f.Fuzz(func(t *testing.T, input []byte) { + var con verifyJSON + if err := json.Unmarshal(input, &con); err != nil { + return + } + spec, ref, proof := con.Spec, con.Ref, con.Proof + if ref == nil { + return + } + _ = VerifyMembership(spec, ref.RootHash, proof, ref.Key, ref.Value) + }) +} diff --git a/go/ics23.go b/go/ics23.go index 1950c877..4772e098 100644 --- a/go/ics23.go +++ b/go/ics23.go @@ -132,6 +132,10 @@ func CombineProofs(proofs []*CommitmentProof) (*CommitmentProof, error) { } func getExistProofForKey(proof *CommitmentProof, key []byte) *ExistenceProof { + if proof == nil { + return nil + } + switch p := proof.Proof.(type) { case *CommitmentProof_Exist: ep := p.Exist diff --git a/go/ops.go b/go/ops.go index 06601de7..bd3d0eaa 100644 --- a/go/ops.go +++ b/go/ops.go @@ -95,6 +95,9 @@ func (op *InnerOp) Apply(child []byte) ([]byte, error) { // CheckAgainstSpec will verify the LeafOp is in the format defined in spec func (op *LeafOp) CheckAgainstSpec(spec *ProofSpec) error { lspec := spec.LeafSpec + if lspec == nil { + return errors.New("spec.LeafSpec must be non-nil") + } if validateSpec(spec) { err := validateIavlOps(op, 0) @@ -123,6 +126,16 @@ func (op *LeafOp) CheckAgainstSpec(spec *ProofSpec) error { // CheckAgainstSpec will verify the InnerOp is in the format defined in spec func (op *InnerOp) CheckAgainstSpec(spec *ProofSpec, b int) error { + if spec == nil || op == nil { + return errors.New("op and spec must be both non-nil") + } + if spec.InnerSpec == nil { + return errors.New("spec.InnerSpec must be non-nil") + } + if spec.LeafSpec == nil { + return errors.New("spec.LeafSpec must be non-nil") + } + if op.Hash != spec.InnerSpec.Hash { return fmt.Errorf("unexpected HashOp: %d", op.Hash) } diff --git a/go/proof.go b/go/proof.go index 95dfdc93..35be2054 100644 --- a/go/proof.go +++ b/go/proof.go @@ -180,11 +180,11 @@ func (p *NonExistenceProof) Calculate() (CommitmentRoot, error) { // CheckAgainstSpec will verify the leaf and all path steps are in the format defined in spec func (p *ExistenceProof) CheckAgainstSpec(spec *ProofSpec) error { - if p.GetLeaf() == nil { + leaf := p.GetLeaf() + if leaf == nil { return errors.New("existence Proof needs defined LeafOp") } - err := p.Leaf.CheckAgainstSpec(spec) - if err != nil { + if err := leaf.CheckAgainstSpec(spec); err != nil { return fmt.Errorf("leaf, %w", err) } if spec.MinDepth > 0 && len(p.Path) < int(spec.MinDepth) { @@ -452,13 +452,41 @@ func orderFromPadding(spec *InnerSpec, inner *InnerOp) (int32, error) { // over-declares equality, which we cosnider fine for now. func (p *ProofSpec) SpecEquals(spec *ProofSpec) bool { - return p.LeafSpec.Hash == spec.LeafSpec.Hash && - p.LeafSpec.PrehashKey == spec.LeafSpec.PrehashKey && - p.LeafSpec.PrehashValue == spec.LeafSpec.PrehashValue && - p.LeafSpec.Length == spec.LeafSpec.Length && - p.InnerSpec.Hash == spec.InnerSpec.Hash && - p.InnerSpec.MinPrefixLength == spec.InnerSpec.MinPrefixLength && - p.InnerSpec.MaxPrefixLength == spec.InnerSpec.MaxPrefixLength && - p.InnerSpec.ChildSize == spec.InnerSpec.ChildSize && - len(p.InnerSpec.ChildOrder) == len(spec.InnerSpec.ChildOrder) + // 1. Compare LeafSpecs values. + switch { + case (p.LeafSpec == nil) != (spec.LeafSpec == nil): // One of them is nil. + return false + + case p.LeafSpec != nil && spec.LeafSpec != nil: + ok := p.LeafSpec.Hash == spec.LeafSpec.Hash && + p.LeafSpec.PrehashKey == spec.LeafSpec.PrehashKey && + p.LeafSpec.PrehashValue == spec.LeafSpec.PrehashValue && + p.LeafSpec.Length == spec.LeafSpec.Length + if !ok { + return false + } + + default: // Both are nil, hence LeafSpec values are equal. + } + + // 2. Compare InnerSpec values. + switch { + case (p.InnerSpec == nil) != (spec.InnerSpec == nil): // One of them is not nil. + return false + + case p.InnerSpec != nil && spec.InnerSpec == nil: // Both are non-nil + ok := p.InnerSpec.Hash == spec.InnerSpec.Hash && + p.InnerSpec.MinPrefixLength == spec.InnerSpec.MinPrefixLength && + p.InnerSpec.MaxPrefixLength == spec.InnerSpec.MaxPrefixLength && + p.InnerSpec.ChildSize == spec.InnerSpec.ChildSize && + len(p.InnerSpec.ChildOrder) == len(spec.InnerSpec.ChildOrder) + if !ok { + return false + } + + default: // Both are nil, hence InnerSpec values are equal. + } + + // By this point all the above conditions pass so they are equal. + return true } diff --git a/go/proof_data_test.go b/go/proof_data_test.go index fe987846..4cc20e47 100644 --- a/go/proof_data_test.go +++ b/go/proof_data_test.go @@ -13,18 +13,18 @@ type ExistenceProofTestStruct struct { Expected []byte } -func ExistenceProofTestData(t *testing.T) map[string]ExistenceProofTestStruct { - t.Helper() +func ExistenceProofTestData(tb testing.TB) map[string]ExistenceProofTestStruct { + tb.Helper() fname := filepath.Join("..", "testdata", "TestExistenceProofData.json") ffile, err := os.Open(fname) if err != nil { - t.Fatal(err) + tb.Fatal(err) } var cases map[string]ExistenceProofTestStruct jsonDecoder := json.NewDecoder(ffile) err = jsonDecoder.Decode(&cases) if err != nil { - t.Fatal(err) + tb.Fatal(err) } return cases } @@ -35,18 +35,18 @@ type CheckLeafTestStruct struct { IsErr bool } -func CheckLeafTestData(t *testing.T) map[string]CheckLeafTestStruct { - t.Helper() +func CheckLeafTestData(tb testing.TB) map[string]CheckLeafTestStruct { + tb.Helper() fname := filepath.Join("..", "testdata", "TestCheckLeafData.json") ffile, err := os.Open(fname) if err != nil { - t.Fatal(err) + tb.Fatal(err) } var cases map[string]CheckLeafTestStruct jsonDecoder := json.NewDecoder(ffile) err = jsonDecoder.Decode(&cases) if err != nil { - t.Fatal(err) + tb.Fatal(err) } return cases } @@ -57,18 +57,18 @@ type CheckAgainstSpecTestStruct struct { IsErr bool } -func CheckAgainstSpecTestData(t *testing.T) map[string]CheckAgainstSpecTestStruct { - t.Helper() +func CheckAgainstSpecTestData(tb testing.TB) map[string]CheckAgainstSpecTestStruct { + tb.Helper() fname := filepath.Join("..", "testdata", "TestCheckAgainstSpecData.json") ffile, err := os.Open(fname) if err != nil { - t.Fatal(err) + tb.Fatal(err) } var cases map[string]CheckAgainstSpecTestStruct jsonDecoder := json.NewDecoder(ffile) err = jsonDecoder.Decode(&cases) if err != nil { - t.Fatal(err) + tb.Fatal(err) } return cases } diff --git a/go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/09bebc2fc8d0a79b b/go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/09bebc2fc8d0a79b new file mode 100644 index 00000000..cca33f95 --- /dev/null +++ b/go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/09bebc2fc8d0a79b @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("{\"Proof\":{\"leAf\":{\"prehAsh_vAlue\":1,\"length\":1,\"prefiX\":\"AA00\"},\"pAth\":[{\"hAsh\":1}]},\"SpeC\":{\"leAf_speC\":{\"prehAsh_vAlue\":1,\"length\":1,\"prefiX\":\"AA00\"},\"inner_speC\":{\"hAsh\":1}}}") diff --git a/go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/19e35d361fe85847 b/go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/19e35d361fe85847 new file mode 100644 index 00000000..3d3897f8 --- /dev/null +++ b/go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/19e35d361fe85847 @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("{\"Proof\":{\"leAf\":{},\"pAth\":[{}]},\"SpeC\":{\"leAf_speC\":{}}}") diff --git a/go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/1f84363823f5c624 b/go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/1f84363823f5c624 new file mode 100644 index 00000000..b0ab56a3 --- /dev/null +++ b/go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/1f84363823f5c624 @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("{\"Proof\":{\"leAf\":{}},\"SpeC\":{}}") diff --git a/go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/a9ba9cba7c7724a0 b/go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/a9ba9cba7c7724a0 new file mode 100644 index 00000000..d29cd40b --- /dev/null +++ b/go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/a9ba9cba7c7724a0 @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("{\"Proof\":{\"leAf\":{}}}") diff --git a/go/testdata/fuzz/FuzzVerifyMembership/8093511184ad3e25 b/go/testdata/fuzz/FuzzVerifyMembership/8093511184ad3e25 new file mode 100644 index 00000000..3f1f65ec --- /dev/null +++ b/go/testdata/fuzz/FuzzVerifyMembership/8093511184ad3e25 @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("{}") diff --git a/go/testdata/fuzz/FuzzVerifyMembership/99dd1125ca292163 b/go/testdata/fuzz/FuzzVerifyMembership/99dd1125ca292163 new file mode 100644 index 00000000..d00a7f7f --- /dev/null +++ b/go/testdata/fuzz/FuzzVerifyMembership/99dd1125ca292163 @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("{\"Ref\":{}}") diff --git a/go/vectors_data_test.go b/go/vectors_data_test.go index fad2e451..f6e453cc 100644 --- a/go/vectors_data_test.go +++ b/go/vectors_data_test.go @@ -165,42 +165,42 @@ func DecompressBatchVectorsTestData(t *testing.T) map[string]*CommitmentProof { } } -func LoadFile(t *testing.T, dir string, filename string) (*CommitmentProof, *RefData) { - t.Helper() +func LoadFile(tb testing.TB, dir string, filename string) (*CommitmentProof, *RefData) { + tb.Helper() // load the file into a json struct name := filepath.Join(dir, filename) bz, err := os.ReadFile(name) if err != nil { - t.Fatalf("Read file: %+v", err) + tb.Fatalf("Read file: %+v", err) } var data TestVector err = json.Unmarshal(bz, &data) if err != nil { - t.Fatalf("Unmarshal json: %+v", err) + tb.Fatalf("Unmarshal json: %+v", err) } // parse the protobuf object var proof CommitmentProof - err = proof.Unmarshal(mustHex(t, data.Proof)) + err = proof.Unmarshal(mustHex(tb, data.Proof)) if err != nil { - t.Fatalf("Unmarshal protobuf: %+v", err) + tb.Fatalf("Unmarshal protobuf: %+v", err) } var ref RefData - ref.RootHash = CommitmentRoot(mustHex(t, data.RootHash)) - ref.Key = mustHex(t, data.Key) + ref.RootHash = CommitmentRoot(mustHex(tb, data.RootHash)) + ref.Key = mustHex(tb, data.Key) if data.Value != "" { - ref.Value = mustHex(t, data.Value) + ref.Value = mustHex(tb, data.Value) } return &proof, &ref } -func mustHex(t *testing.T, data string) []byte { - t.Helper() +func mustHex(tb testing.TB, data string) []byte { + tb.Helper() if data == "" { return nil } res, err := hex.DecodeString(data) if err != nil { - t.Fatalf("decoding hex: %v", err) + tb.Fatalf("decoding hex: %v", err) } return res }