From 7f698ba3fa232c54109e5b4ea42562bbecdb1bf8 Mon Sep 17 00:00:00 2001 From: Marko Date: Sun, 9 Oct 2022 00:14:55 +0200 Subject: [PATCH] chore: backport proof fix (#582) Co-authored-by: Marko Baricevic --- CHANGELOG.md | 6 +++ Makefile | 7 ++- proof.go | 7 +++ proof_forgery_test.go | 106 ++++++++++++++++++++++++++++++++++++++++++ proof_iavl_absence.go | 8 ++++ proof_path.go | 1 + proof_range.go | 7 +++ proof_test.go | 6 +++ 8 files changed, 147 insertions(+), 1 deletion(-) create mode 100644 proof_forgery_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 3506d0254..301696d3f 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/Makefile b/Makefile index 028594ee9..0364b7350 100644 --- a/Makefile +++ b/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: diff --git a/proof.go b/proof.go index 37f9242f8..d6dd5b77f 100644 --- a/proof.go +++ b/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) } diff --git a/proof_forgery_test.go b/proof_forgery_test.go new file mode 100644 index 000000000..241db1cd7 --- /dev/null +++ b/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 +} diff --git a/proof_iavl_absence.go b/proof_iavl_absence.go index e81ec3123..e18dbe3f4 100644 --- a/proof_iavl_absence.go +++ 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,10 +91,12 @@ 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() @@ -98,6 +104,7 @@ func (op AbsenceOp) Run(args [][]byte) ([][]byte, error) { 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? @@ -105,6 +112,7 @@ func (op AbsenceOp) Run(args [][]byte) ([][]byte, error) { if err != nil { return nil, errors.Wrap(err, "verifying absence") } + return [][]byte{root}, nil } diff --git a/proof_path.go b/proof_path.go index 95397858a..690ce0ab3 100644 --- a/proof_path.go +++ b/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 diff --git a/proof_range.go b/proof_range.go index ada2a3e15..79db502b0 100644 --- a/proof_range.go +++ b/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 } diff --git a/proof_test.go b/proof_test.go index 8eb568ab0..5e1c0ce48 100644 --- a/proof_test.go +++ b/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) }