fix(go): bullet-proof against nil dereferences + more fuzzers (#244)

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. To ensure that the fuzzers can run alright,
added -short to any invocations of "go test".

Fixes #241
Fixes #242
Fixes #243

Author: odeke-em

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() {
@@ -89,6 +122,57 @@ func FuzzVerifyNonMembership(f *testing.F) {
 	})
 }
 
+// verifyJSON is necessary so we already have sources of Proof, Ref and Spec
+// that'll be mutated by the fuzzer to get much closer to values for greater coverage.
+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.
+		// The use of already calculated values is necessary
+		// for the fuzzers to have a basis of much better coverage
+		// generating values.
+		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)
+	})
+}
+
 func FuzzCombineProofs(f *testing.F) {
 	// 1. Load in the CommitmentProofs
 	baseDirs := []string{"iavl", "tendermint", "smt"}

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

go/ops.go

@@ -94,7 +94,13 @@ 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 {
+	if spec == nil {
+		return errors.New("op and spec must be non-nil")
+	}
 	lspec := spec.LeafSpec
+	if lspec == nil {
+		return errors.New("spec.LeafSpec must be non-nil")
+	}
 
 	if validateSpec(spec) {
 		err := validateIavlOps(op, 0)
@@ -123,6 +129,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 {
+		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)
 	}
@@ -146,6 +162,10 @@ func (op *InnerOp) CheckAgainstSpec(spec *ProofSpec, b int) error {
 		return fmt.Errorf("innerOp prefix too long (%d)", len(op.Prefix))
 	}
 
+	if spec.InnerSpec.ChildSize <= 0 {
+		return errors.New("spec.InnerSpec.ChildSize must be >= 1")
+	}
+
 	// ensures soundness, with suffix having to be of correct length
 	if len(op.Suffix)%int(spec.InnerSpec.ChildSize) != 0 {
 		return fmt.Errorf("InnerOp suffix malformed")

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
 }

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
 }

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

go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/19e35d361fe85847

@@ -0,0 +1,2 @@
+go test fuzz v1
+[]byte("{\"Proof\":{\"leAf\":{},\"pAth\":[{}]},\"SpeC\":{\"leAf_speC\":{}}}")

go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/1f84363823f5c624

@@ -0,0 +1,2 @@
+go test fuzz v1
+[]byte("{\"Proof\":{\"leAf\":{}},\"SpeC\":{}}")

go/testdata/fuzz/FuzzExistenceProofCheckAgainstSpec/a9ba9cba7c7724a0

@@ -0,0 +1,2 @@
+go test fuzz v1
+[]byte("{\"Proof\":{\"leAf\":{}}}")

go/testdata/fuzz/FuzzVerifyMembership/8093511184ad3e25

@@ -0,0 +1,2 @@
+go test fuzz v1
+[]byte("{}")