feat: add stricter checks for iavl spec and tendermint spec

go/ops.go

@@ -20,43 +20,90 @@ import (
 	_ "golang.org/x/crypto/ripemd160" //nolint:staticcheck
 )
 
-// validate the IAVL Ops
-func validateIavlOps(op opType, b int) error {
+// validateIavlOps validates the prefix to ensure it begins with
+// the height, size, and version of the IAVL tree. Each varint must be a bounded value.
+// In addition, the remaining bytes are validated to ensure they correspond to the correct
+// length. The layerNum is the inverse of the tree depth, i.e. depth=0 means leaf, depth>=1 means inner node
+func validateIavlOps(op opType, layerNum int) error {
 	r := bytes.NewReader(op.GetPrefix())
 
-	values := []int64{}
-	for i := 0; i < 3; i++ {
-		varInt, err := binary.ReadVarint(r)
-		if err != nil {
-			return err
-		}
-		values = append(values, varInt)
+	height, err := binary.ReadVarint(r)
+	if err != nil {
+		return fmt.Errorf("failed to read IAVL height varint: %w", err)
+	}
+	if int(height) < 0 || int(height) < layerNum {
+		return fmt.Errorf("IAVL height (%d) must be non-negative and greater than or equal to the layer number (%d)", height, layerNum)
+	}
 
-		// values must be bounded
-		if int(varInt) < 0 {
-			return fmt.Errorf("wrong value in IAVL leaf op")
-		}
+	size, err := binary.ReadVarint(r)
+	if err != nil {
+		return fmt.Errorf("failed to read IAVL size varint: %w", err)
 	}
-	if int(values[0]) < b {
-		return fmt.Errorf("wrong value in IAVL leaf op")
+
+	if int(size) < 0 {
+		return fmt.Errorf("IAVL size must be non-negative")
 	}
 
-	r2 := r.Len()
-	if b == 0 {
-		if r2 != 0 {
-			return fmt.Errorf("invalid op")
+	version, err := binary.ReadVarint(r)
+	if err != nil {
+		return fmt.Errorf("failed to read IAVL version varint: %w", err)
+	}
+
+	if int(version) < 0 {
+		return fmt.Errorf("IAVL version must be non-negative")
+	}
+
+	// grab the length of the remainder of the prefix
+	remLen := r.Len()
+	if layerNum == 0 {
+		// leaf node
+		if remLen != 0 {
+			return fmt.Errorf("expected remaining prefix length to be 0, got: %d", remLen)
+		}
+		if height != 0 {
+			return fmt.Errorf("expected leaf node height to be 0, got: %d", remLen)
+		}
+		if size != 1 {
+			return fmt.Errorf("expected leaf node size to be 1, got: %d", remLen)
 		}
 	} else {
-		if !(r2^(0xff&0x01) == 0 || r2 == (0xde+int('v'))/10) {
-			return fmt.Errorf("invalid op")
+		// inner node
+		//
+		// when the child comes from the left, the suffix if filled in
+		// prefix: height | size | version | length byte (1 remainder)
+		//
+		// when the child comes from the right, the suffix is empty
+		// prefix: height | size | version | length byte | 32 byte hash | next length byte (34 remainder)
+		if remLen != 1 && remLen != 34 {
+			return fmt.Errorf("remainder of prefix must be of length 1 or 34, got: %d", remLen)
 		}
-		if op.GetHash()^1 != 0 {
-			return fmt.Errorf("invalid op")
+		if op.GetHash() != HashOp_SHA256 {
+			return fmt.Errorf("IAVL hash op must be %v", HashOp_SHA256)
 		}
 	}
 	return nil
 }
 
+// validateTendermintOps validates the prefix to ensure it begins with []byte{1}.
+func validateTendermintOps(op *InnerOp) error {
+	if len(op.Prefix) == 0 {
+		return fmt.Errorf("inner op prefix must not be empty")
+	}
+
+	innerPrefix := []byte{1}
+	if op.Suffix != nil {
+		if !bytes.Equal(op.Prefix, innerPrefix) {
+			return fmt.Errorf("expected inner op prefix: %v, got: %v", innerPrefix, op.Prefix)
+		}
+	}
+
+	if !bytes.HasPrefix(op.Prefix, innerPrefix) {
+		return fmt.Errorf("expected inner op prefix to begin with: %v, got: %v", innerPrefix, op.Prefix[:1])
+	}
+
+	return nil
+}
+
 // Apply will calculate the leaf hash given the key and value being proven
 func (op *LeafOp) Apply(key []byte, value []byte) ([]byte, error) {
 	if len(key) == 0 {
@@ -102,7 +149,7 @@ func (op *LeafOp) CheckAgainstSpec(spec *ProofSpec) error {
 		return errors.New("spec.LeafSpec must be non-nil")
 	}
 
-	if validateSpec(spec) {
+	if spec.SpecEquals(IavlSpec) {
 		err := validateIavlOps(op, 0)
 		if err != nil {
 			return err
@@ -143,13 +190,20 @@ func (op *InnerOp) CheckAgainstSpec(spec *ProofSpec, b int) error {
 		return fmt.Errorf("unexpected HashOp: %d", op.Hash)
 	}
 
-	if validateSpec(spec) {
+	if spec.SpecEquals(IavlSpec) {
 		err := validateIavlOps(op, b)
 		if err != nil {
 			return err
 		}
 	}
 
+	if spec.SpecEquals(TendermintSpec) {
+		err := validateTendermintOps(op)
+		if err != nil {
+			return err
+		}
+	}
+
 	leafPrefix := spec.LeafSpec.Prefix
 	if bytes.HasPrefix(op.Prefix, leafPrefix) {
 		return fmt.Errorf("inner Prefix starts with %X", leafPrefix)
@@ -228,10 +282,6 @@ func prepareLeafData(hashOp HashOp, lengthOp LengthOp, data []byte) ([]byte, err
 	return doLengthOp(lengthOp, hdata)
 }
 
-func validateSpec(spec *ProofSpec) bool {
-	return spec.SpecEquals(IavlSpec)
-}
-
 type opType interface {
 	GetPrefix() []byte
 	GetHash() HashOp

go/ops_test.go

@@ -9,6 +9,195 @@ import (
 	"testing"
 )
 
+func TestValidateIavlOps(t *testing.T) {
+	var (
+		op       opType
+		layerNum int
+	)
+	cases := []struct {
+		name     string
+		malleate func()
+		expError error
+	}{
+		{
+			"success",
+			func() {},
+			nil,
+		},
+		{
+			"failure: reading varint",
+			func() {
+				op.(*InnerOp).Prefix = []byte{}
+			},
+			errors.New("failed to read IAVL height varint: EOF"),
+		},
+		{
+			"failure: invalid height value",
+			func() {
+				op.(*InnerOp).Prefix = []byte{1}
+			},
+			errors.New("IAVL height (-1) must be non-negative and greater than or equal to the layer number (1)"),
+		},
+		{
+			"failure: invalid size varint",
+			func() {
+				var varintBuf [binary.MaxVarintLen64]byte
+				prefix := convertVarIntToBytes(int64(5), varintBuf)
+				op.(*InnerOp).Prefix = prefix
+			},
+			errors.New("failed to read IAVL size varint: EOF"),
+		},
+		{
+			"failure: invalid size value",
+			func() {
+				var varintBuf [binary.MaxVarintLen64]byte
+				prefix := convertVarIntToBytes(int64(5), varintBuf)
+				prefix = append(prefix, convertVarIntToBytes(int64(-1), varintBuf)...) // size
+				op.(*InnerOp).Prefix = prefix
+			},
+			errors.New("IAVL size must be non-negative"),
+		},
+		{
+			"failure: invalid version varint",
+			func() {
+				var varintBuf [binary.MaxVarintLen64]byte
+				prefix := convertVarIntToBytes(int64(5), varintBuf)
+				prefix = append(prefix, convertVarIntToBytes(int64(10), varintBuf)...)
+				op.(*InnerOp).Prefix = prefix
+			},
+			errors.New("failed to read IAVL version varint: EOF"),
+		},
+		{
+			"failure: invalid version value",
+			func() {
+				var varintBuf [binary.MaxVarintLen64]byte
+				prefix := convertVarIntToBytes(int64(5), varintBuf)
+				prefix = append(prefix, convertVarIntToBytes(int64(10), varintBuf)...)
+				prefix = append(prefix, convertVarIntToBytes(int64(-1), varintBuf)...) // version
+				op.(*InnerOp).Prefix = prefix
+			},
+			errors.New("IAVL version must be non-negative"),
+		},
+		{
+			"failure: invalid remaining length with layer number is 0",
+			func() {
+				layerNum = 0
+			},
+			fmt.Errorf("expected remaining prefix length to be 0, got: 1"),
+		},
+		{
+			"failure: invalid reminaing length with non-zero layer number",
+			func() {
+				layerNum = 1
+				op.(*InnerOp).Prefix = append(op.(*InnerOp).Prefix, []byte{1}...)
+			},
+			fmt.Errorf("remainder of prefix must be of length 1 or 34, got: 2"),
+		},
+		{
+			"failure: invalid hash",
+			func() {
+				op.(*InnerOp).Hash = HashOp_NO_HASH
+			},
+			fmt.Errorf("IAVL hash op must be %v", HashOp_SHA256),
+		},
+	}
+	for _, tc := range cases {
+		tc := tc
+
+		t.Run(tc.name, func(t *testing.T) {
+			op = &InnerOp{
+				Hash:   HashOp_SHA256,
+				Prefix: generateInnerOpPrefix(),
+				Suffix: []byte(""),
+			}
+			layerNum = 1
+
+			tc.malleate()
+
+			err := validateIavlOps(op, layerNum)
+			if tc.expError == nil && err != nil {
+				t.Fatal(err)
+			}
+
+			if tc.expError != nil && err.Error() != tc.expError.Error() {
+				t.Fatalf("expected: %v, got: %v", tc.expError, err)
+			}
+		})
+
+	}
+}
+
+func TestValidateTendermintOps(t *testing.T) {
+	var op *InnerOp
+	cases := []struct {
+		name     string
+		malleate func()
+		expError error
+	}{
+		{
+			"success: valid prefix when suffix populated",
+			func() {},
+			nil,
+		},
+		{
+			"success: valid prefix when suffix empty",
+			func() {
+				op.Prefix = []byte{1, 2}
+				op.Suffix = nil
+			},
+			nil,
+		},
+		{
+			"failure: empty prefix and suffix",
+			func() {
+				op.Prefix = nil
+				op.Suffix = nil
+			},
+			errors.New("inner op prefix must not be empty"),
+		},
+		{
+			"failure: invalid prefix when suffix populated",
+			func() {
+				op.Prefix = []byte{0}
+				op.Suffix = []byte{1}
+			},
+			fmt.Errorf("expected inner op prefix: %v, got: %v", []byte{1}, []byte{0}),
+		},
+		{
+			"failure: invalid prefix when suffix empty",
+			func() {
+				op.Prefix = []byte{2, 1}
+				op.Suffix = nil
+			},
+			fmt.Errorf("expected inner op prefix to begin with: %v, got: %v", []byte{1}, []byte{2}),
+		},
+	}
+
+	for _, tc := range cases {
+		tc := tc
+
+		t.Run(tc.name, func(t *testing.T) {
+			op = &InnerOp{
+				Hash:   HashOp_SHA256,
+				Prefix: []byte{1},
+				Suffix: []byte{1},
+			}
+
+			tc.malleate()
+
+			err := validateTendermintOps(op)
+			if tc.expError == nil && err != nil {
+				t.Fatal(err)
+			}
+
+			if tc.expError != nil && err.Error() != tc.expError.Error() {
+				t.Fatalf("expected: %v, got: %v", tc.expError, err)
+			}
+		})
+
+	}
+}
+
 func TestLeafOp(t *testing.T) {
 	cases := LeafOpTestData(t)
 
@@ -114,7 +303,7 @@ func TestInnerOpCheckAgainstSpec(t *testing.T) {
 			func() {
 				innerOp.Prefix = []byte{0x01}
 			},
-			fmt.Errorf("wrong value in IAVL leaf op"),
+			fmt.Errorf("IAVL height (-1) must be non-negative and greater than or equal to the layer number (1)"),
 		},
 		{
 			"failure: inner prefix starts with leaf prefix",

go/proof_test.go

@@ -52,7 +52,7 @@ func TestCheckAgainstSpec(t *testing.T) {
 			if tc.Err == "" && err != nil {
 				t.Fatalf("Unexpected error: %v", err)
 			} else if tc.Err != "" && tc.Err != err.Error() {
-				t.Fatalf("Expected error: %s, got %s", tc.Err, err.Error())
+				t.Fatalf("Expected error: %s, got: %s", tc.Err, err.Error())
 			}
 		})
 	}

testdata/TestCheckAgainstSpecData.json

@@ -907,7 +907,7 @@
         "hash": 1
       }
     },
-    "Err": "inner, unexpected EOF"
+    "Err": "inner, IAVL height (0) must be non-negative and greater than or equal to the layer number (3)"
   },
   "rejects only inner proof (hash mismatch)": {
     "Proof": {