chore: backport proof fix (#582)

Co-authored-by: Marko Baricevic <markobaricevic3778@gmail.com>

CHANGELOG.md

@@ -2,6 +2,12 @@
 
 ## Unreleased
 
+## 0.19.3 (October 8, 2022)
+
+- `ProofInner.Hash()` prevents both right and left from both being set. Only one is allowed to be set. 
+
+> Note: It is recommended to not use the native proof structure of IAVL in its current form. Please refer to [ics23](https://github.com/confio/ics23/tree/master/go) for IAVL proofs
+
 ## 0.19.2 (October 6, 2022)
 
  - [#547](https://github.com/cosmos/iavl/pull/547) Implement `skipFastStorageUpgrade` in order to skip fast storage upgrade and usage. 

Makefile

@@ -22,9 +22,14 @@ else
 endif
 .PHONY: install
 
+test-short:
+	@echo "--> Running go test"
+	@go test ./... $(LDFLAGS) -v --race --short
+.PHONY: test-short
+
 test:
 	@echo "--> Running go test"
-	@go test ./... $(LDFLAGS) -v --race
+	@go test ./... $(LDFLAGS) -v 
 .PHONY: test
 
 tools:

proof.go

@@ -32,6 +32,8 @@ var (
 )
 
 //----------------------------------------
+// ProofInnerNode
+// Contract: Left and Right can never both be set. Will result in a empty `[]` roothash
 
 type ProofInnerNode struct {
 	Height  int8   `json:"height"`
@@ -76,6 +78,10 @@ func (pin ProofInnerNode) Hash(childHash []byte) ([]byte, error) {
 		err = encoding.EncodeVarint(buf, pin.Version)
 	}
 
+	if len(pin.Left) > 0 && len(pin.Right) > 0 {
+		return nil, errors.New("both left and right child hashes are set")
+	}
+
 	if len(pin.Left) == 0 {
 		if err == nil {
 			err = encoding.EncodeBytes(buf, childHash)
@@ -91,6 +97,7 @@ func (pin ProofInnerNode) Hash(childHash []byte) ([]byte, error) {
 			err = encoding.EncodeBytes(buf, childHash)
 		}
 	}
+
 	if err != nil {
 		return nil, fmt.Errorf("failed to hash ProofInnerNode: %v", err)
 	}

proof_forgery_test.go

@@ -0,0 +1,106 @@
+package iavl_test
+
+import (
+	"encoding/hex"
+	"math/rand"
+	"strings"
+	"testing"
+
+	"github.com/cosmos/iavl"
+	"github.com/stretchr/testify/require"
+	"github.com/tendermint/tendermint/crypto/tmhash"
+	db "github.com/tendermint/tm-db"
+)
+
+func TestProofFogery(t *testing.T) {
+	source := rand.NewSource(0)
+	r := rand.New(source)
+	cacheSize := 0
+	tree, err := iavl.NewMutableTreeWithOpts(db.NewMemDB(), cacheSize, nil, false)
+	require.NoError(t, err)
+
+	// two keys only
+	keys := []byte{0x11, 0x32}
+	values := make([][]byte, len(keys))
+	// make random values and insert into tree
+	for i, ikey := range keys {
+		key := []byte{ikey}
+		v := r.Intn(255)
+		values[i] = []byte{byte(v)}
+		tree.Set(key, values[i])
+	}
+
+	// get root
+	root, err := tree.WorkingHash()
+	require.NoError(t, err)
+	// use the rightmost kv pair in the tree so the inner nodes will populate left
+	k := []byte{keys[1]}
+	v := values[1]
+
+	val, proof, err := tree.GetWithProof(k)
+	require.NoError(t, err)
+
+	err = proof.Verify(root)
+	require.NoError(t, err)
+	err = proof.VerifyItem(k, val)
+	require.NoError(t, err)
+
+	// ------------------- FORGE PROOF -------------------
+
+	forgedPayloadBytes := mustDecode("0xabcd")
+	forgedValueHash := tmhash.Sum(forgedPayloadBytes)
+	// make a forgery of the proof by adding:
+	// - a new leaf node to the right
+	// - an empty inner node
+	// - a right entry in the path
+	_, proof2, _ := tree.GetWithProof(k)
+	forgedNode := proof2.Leaves[0]
+	forgedNode.Key = []byte{0xFF}
+	forgedNode.ValueHash = forgedValueHash
+	proof2.Leaves = append(proof2.Leaves, forgedNode)
+	proof2.InnerNodes = append(proof2.InnerNodes, iavl.PathToLeaf{})
+	// figure out what hash we need via https://twitter.com/samczsun/status/1578181160345034752
+	proof2.LeftPath[0].Right = mustDecode("82C36CED85E914DAE8FDF6DD11FD5833121AA425711EB126C470CE28FF6623D5")
+
+	rootHashValid := proof.ComputeRootHash()
+	verifyErr := proof.Verify(rootHashValid)
+	require.NoError(t, verifyErr, "should verify")
+	// forgery gives empty root hash (previously it returned the same one!)
+	rootHashForged := proof2.ComputeRootHash()
+	require.Empty(t, rootHashForged, "roothash must be empty if both left and right are set")
+	verifyErr = proof2.Verify(rootHashForged)
+	require.Error(t, verifyErr, "should not verify")
+
+	// verify proof two fails with valid proof
+	err = proof2.Verify(rootHashValid)
+	require.Error(t, err, "should not verify different root hash")
+
+	{
+		// legit node verifies against legit proof (expected)
+		verifyErr = proof.VerifyItem(k, v)
+		require.NoError(t, verifyErr, "valid proof should verify")
+		// forged node fails to verify against legit proof (expected)
+		verifyErr = proof.VerifyItem(forgedNode.Key, forgedPayloadBytes)
+		require.Error(t, verifyErr, "forged proof should fail to verify")
+	}
+	{
+		// legit node fails to verify against forged proof (expected)
+		verifyErr = proof2.VerifyItem(k, v)
+		require.Error(t, verifyErr, "valid proof should verify, but has a forged sister node")
+
+		// forged node fails to verify against forged proof (previously this succeeded!)
+		verifyErr = proof2.VerifyItem(forgedNode.Key, forgedPayloadBytes)
+		require.Error(t, verifyErr, "forged proof should fail verify")
+	}
+}
+
+func mustDecode(str string) []byte {
+	if strings.HasPrefix(str, "0x") {
+		str = str[2:]
+	}
+	b, err := hex.DecodeString(str)
+	if err != nil {
+		panic(err)
+	}
+	return b
+}

proof_iavl_absence.go

@@ -46,18 +46,22 @@ func AbsenceOpDecoder(pop tmmerkle.ProofOp) (merkle.ProofOperator, error) {
 	if err != nil {
 		return nil, err
 	}
+
 	if n != len(pop.Data) {
 		return nil, fmt.Errorf("unexpected bytes, expected %v got %v", n, len(pop.Data))
 	}
+
 	pbProofOp := &iavlproto.AbsenceOp{}
 	err = proto.Unmarshal(bz, pbProofOp)
 	if err != nil {
 		return nil, err
 	}
+
 	proof, err := RangeProofFromProto(pbProofOp.Proof)
 	if err != nil {
 		return nil, err
 	}
+
 	return NewAbsenceOp(pop.Key, &proof), nil
 }
 
@@ -87,24 +91,28 @@ func (op AbsenceOp) Run(args [][]byte) ([][]byte, error) {
 	if len(args) != 0 {
 		return nil, errors.Errorf("expected 0 args, got %v", len(args))
 	}
+
 	// If the tree is nil, the proof is nil, and all keys are absent.
 	if op.Proof == nil {
 		return [][]byte{[]byte(nil)}, nil
 	}
+
 	// Compute the root hash and assume it is valid.
 	// The caller checks the ultimate root later.
 	root := op.Proof.ComputeRootHash()
 	err := op.Proof.Verify(root)
 	if err != nil {
 		return nil, errors.Wrap(err, "computing root hash")
 	}
+
 	// XXX What is the encoding for keys?
 	// We should decode the key depending on whether it's a string or hex,
 	// maybe based on quotes and 0x prefix?
 	err = op.Proof.VerifyAbsence(op.key)
 	if err != nil {
 		return nil, errors.Wrap(err, "verifying absence")
 	}
+
 	return [][]byte{root}, nil
 }
 

proof_path.go

@@ -67,6 +67,7 @@ func (pl PathToLeaf) stringIndented(indent string) string {
 
 // `computeRootHash` computes the root hash assuming some leaf hash.
 // Does not verify the root hash.
+// Contract: Caller must verify that the roothash is correct by calling `.verify()`.
 func (pl PathToLeaf) computeRootHash(leafHash []byte) ([]byte, error) {
 	var err error
 	hash := leafHash

proof_range.go

@@ -96,13 +96,16 @@ func (proof *RangeProof) VerifyItem(key, value []byte) error {
 	if proof == nil {
 		return errors.Wrap(ErrInvalidProof, "proof is nil")
 	}
+
 	if !proof.rootVerified {
 		return errors.New("must call Verify(root) first")
 	}
+
 	leaves := proof.Leaves
 	i := sort.Search(len(leaves), func(i int) bool {
 		return bytes.Compare(key, leaves[i].Key) <= 0
 	})
+
 	if i >= len(leaves) || !bytes.Equal(leaves[i].Key, key) {
 		return errors.Wrap(ErrInvalidProof, "leaf key not found in proof")
 	}
@@ -207,7 +210,9 @@ func (proof *RangeProof) ComputeRootHash() []byte {
 	if proof == nil {
 		return nil
 	}
+
 	rootHash, _ := proof.computeRootHash()
+
 	return rootHash
 }
 
@@ -283,9 +288,11 @@ func (proof *RangeProof) _computeRootHash() (rootHash []byte, treeEnd bool, err
 			if err != nil {
 				return nil, treeEnd, false, errors.Wrap(err, "recursive COMPUTEHASH call")
 			}
+
 			if !bytes.Equal(derivedRoot, lpath.Right) {
 				return nil, treeEnd, false, errors.Wrapf(ErrInvalidRoot, "intermediate root hash %X doesn't match, got %X", lpath.Right, derivedRoot)
 			}
+
 			if done {
 				return hash, treeEnd, true, nil
 			}

proof_test.go

@@ -30,10 +30,13 @@ func TestTreeGetWithProof(t *testing.T) {
 	require.NoError(err)
 	require.NotEmpty(val)
 	require.NotNil(proof)
+
 	err = proof.VerifyItem(key, val)
 	require.Error(err, "%+v", err) // Verifying item before calling Verify(root)
+
 	err = proof.Verify(root)
 	require.NoError(err, "%+v", err)
+
 	err = proof.VerifyItem(key, val)
 	require.NoError(err, "%+v", err)
 
@@ -42,10 +45,13 @@ func TestTreeGetWithProof(t *testing.T) {
 	require.NoError(err)
 	require.Empty(val)
 	require.NotNil(proof)
+
 	err = proof.VerifyAbsence(key)
 	require.Error(err, "%+v", err) // Verifying absence before calling Verify(root)
+
 	err = proof.Verify(root)
 	require.NoError(err, "%+v", err)
+
 	err = proof.VerifyAbsence(key)
 	require.NoError(err, "%+v", err)
 }