From 2c08967d7ab1e5f8f94a074e5520cd21d33df9d7 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Thu, 29 Aug 2024 14:52:35 +0200 Subject: [PATCH 1/8] fix: prevent out of bounds on lookup inners access --- go/compress.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/go/compress.go b/go/compress.go index 173e7c87..d1d7f790 100644 --- a/go/compress.go +++ b/go/compress.go @@ -153,7 +153,9 @@ func decompressExist(exist *CompressedExistenceProof, lookup []*InnerOp) *Existe Path: make([]*InnerOp, len(exist.Path)), } for i, step := range exist.Path { - res.Path[i] = lookup[step] + if int(step) < len(lookup) { + res.Path[i] = lookup[step] + } } return res } From e08b56848d17523a3f230403c092eacd6d305cee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 8 Oct 2024 12:26:39 +0200 Subject: [PATCH 2/8] refactor: return error instead of panic when decompressing proof also added a unit test for decompressExist to cover new error handling --- go/compress.go | 60 +++++++++++++++++------- go/compress_test.go | 112 ++++++++++++++++++++++++++++++++++++++++++++ go/ics23.go | 29 +++++++++--- go/proof.go | 5 +- go/vectors_test.go | 5 +- 5 files changed, 184 insertions(+), 27 deletions(-) create mode 100644 go/compress_test.go diff --git a/go/compress.go b/go/compress.go index d1d7f790..06c21011 100644 --- a/go/compress.go +++ b/go/compress.go @@ -1,5 +1,7 @@ package ics23 +import "fmt" + // IsCompressed returns true if the proof was compressed func IsCompressed(proof *CommitmentProof) bool { return proof.GetCompressed() != nil @@ -23,16 +25,20 @@ func Compress(proof *CommitmentProof) *CommitmentProof { // Decompress will return a BatchProof if the input is CompressedBatchProof // Otherwise it will return the input. // This is safe to call multiple times (idempotent) -func Decompress(proof *CommitmentProof) *CommitmentProof { +func Decompress(proof *CommitmentProof) (*CommitmentProof, error) { comp := proof.GetCompressed() if comp != nil { + batch, err := decompress(comp) + if err != nil { + return nil, err + } return &CommitmentProof{ Proof: &CommitmentProof_Batch{ - Batch: decompress(comp), + Batch: batch, }, - } + }, nil } - return proof + return proof, nil } func compress(batch *BatchProof) *CompressedBatchProof { @@ -106,45 +112,62 @@ func compressStep(step *InnerOp, lookup *[]*InnerOp, registry map[string]int32) return num } -func decompress(comp *CompressedBatchProof) *BatchProof { +func decompress(comp *CompressedBatchProof) (*BatchProof, error) { lookup := comp.LookupInners var entries []*BatchEntry for _, centry := range comp.Entries { - entry := decompressEntry(centry, lookup) + entry, err := decompressEntry(centry, lookup) + if err != nil { + return nil, err + } entries = append(entries, entry) } return &BatchProof{ Entries: entries, - } + }, nil } -func decompressEntry(entry *CompressedBatchEntry, lookup []*InnerOp) *BatchEntry { +func decompressEntry(entry *CompressedBatchEntry, lookup []*InnerOp) (*BatchEntry, error) { if exist := entry.GetExist(); exist != nil { + decompressedExist, err := decompressExist(exist, lookup) + if err != nil { + return nil, err + } return &BatchEntry{ Proof: &BatchEntry_Exist{ - Exist: decompressExist(exist, lookup), + Exist: decompressedExist, }, - } + }, nil } non := entry.GetNonexist() + decompressedLeft, err := decompressExist(non.Left, lookup) + if err != nil { + return nil, err + } + + decompressedRight, err := decompressExist(non.Right, lookup) + if err != nil { + return nil, err + } + return &BatchEntry{ Proof: &BatchEntry_Nonexist{ Nonexist: &NonExistenceProof{ Key: non.Key, - Left: decompressExist(non.Left, lookup), - Right: decompressExist(non.Right, lookup), + Left: decompressedLeft, + Right: decompressedRight, }, }, - } + }, nil } -func decompressExist(exist *CompressedExistenceProof, lookup []*InnerOp) *ExistenceProof { +func decompressExist(exist *CompressedExistenceProof, lookup []*InnerOp) (*ExistenceProof, error) { if exist == nil { - return nil + return nil, nil } res := &ExistenceProof{ Key: exist.Key, @@ -153,9 +176,10 @@ func decompressExist(exist *CompressedExistenceProof, lookup []*InnerOp) *Existe Path: make([]*InnerOp, len(exist.Path)), } for i, step := range exist.Path { - if int(step) < len(lookup) { - res.Path[i] = lookup[step] + if int(step) >= len(lookup) { + return nil, fmt.Errorf("compressed existence proof at index %d has lookup index out of bounds", i) } + res.Path[i] = lookup[step] } - return res + return res, nil } diff --git a/go/compress_test.go b/go/compress_test.go new file mode 100644 index 00000000..f7e557a4 --- /dev/null +++ b/go/compress_test.go @@ -0,0 +1,112 @@ +package ics23 + +import ( + "fmt" + "reflect" + "testing" +) + +func TestDecompressExist(t *testing.T) { + leafOp := &LeafOp{ + Hash: HashOp_SHA256, + PrehashKey: HashOp_NO_HASH, + PrehashValue: HashOp_NO_HASH, + Length: LengthOp_NO_PREFIX, + Prefix: []byte{}, + } + innerOps := []*InnerOp{ + &InnerOp{ + Hash: HashOp_SHA256, + Prefix: generateInnerOpPrefix(), + }, + &InnerOp{ + Hash: HashOp_SHA256, + Prefix: generateInnerOpPrefix(), + Suffix: []byte{1}, + }, + &InnerOp{ + Hash: HashOp_SHA256, + Prefix: generateInnerOpPrefix(), + Suffix: []byte{2}, + }, + } + + var ( + compressedExistProof *CompressedExistenceProof + lookup []*InnerOp + ) + + cases := []struct { + name string + malleate func() + expProof *ExistenceProof + expError error + }{ + { + "success single lookup", + func() { + compressedExistProof.Path = []int32{0} + }, + &ExistenceProof{ + Key: []byte{0}, + Value: []byte{0}, + Leaf: leafOp, + Path: []*InnerOp{innerOps[0]}, + }, + nil, + }, + { + "success multiple lookups", + func() { + compressedExistProof.Path = []int32{0, 1, 0, 2} + }, + &ExistenceProof{ + Key: []byte{0}, + Value: []byte{0}, + Leaf: leafOp, + Path: []*InnerOp{innerOps[0], innerOps[1], innerOps[0], innerOps[2]}, + }, + nil, + }, + { + "failure: path index out of bounds", + func() { + compressedExistProof.Path = []int32{0} + lookup = nil + }, + nil, + fmt.Errorf("compressed existence proof at index %d has lookup index out of bounds", 0), + }, + } + + for _, tc := range cases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + // reset default values for compressedExistProof and lookup + compressedExistProof = &CompressedExistenceProof{ + Key: []byte{0}, + Value: []byte{0}, + Leaf: leafOp, + } + + lookup = innerOps[:] + + tc.malleate() + + proof, err := decompressExist(compressedExistProof, lookup) + if !reflect.DeepEqual(tc.expProof, proof) { + t.Fatalf("expexted proof: %v, got: %v", tc.expProof, proof) + } + + 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) + } + + }) + } +} diff --git a/go/ics23.go b/go/ics23.go index 4772e098..91ef2080 100644 --- a/go/ics23.go +++ b/go/ics23.go @@ -35,12 +35,15 @@ type CommitmentRoot []byte // calculating the root for the ExistenceProof matches the provided CommitmentRoot func VerifyMembership(spec *ProofSpec, root CommitmentRoot, proof *CommitmentProof, key []byte, value []byte) bool { // decompress it before running code (no-op if not compressed) - proof = Decompress(proof) + proof, err := Decompress(proof) + if err != nil { + return false + } ep := getExistProofForKey(proof, key) if ep == nil { return false } - err := ep.Verify(spec, root, key, value) + err = ep.Verify(spec, root, key, value) return err == nil } @@ -51,12 +54,15 @@ func VerifyMembership(spec *ProofSpec, root CommitmentRoot, proof *CommitmentPro // provided key is between the keys of the two proofs func VerifyNonMembership(spec *ProofSpec, root CommitmentRoot, proof *CommitmentProof, key []byte) bool { // decompress it before running code (no-op if not compressed) - proof = Decompress(proof) + proof, err := Decompress(proof) + if err != nil { + return false + } np := getNonExistProofForKey(spec, proof, key) if np == nil { return false } - err := np.Verify(spec, root, key) + err = np.Verify(spec, root, key) return err == nil } @@ -64,7 +70,10 @@ func VerifyNonMembership(spec *ProofSpec, root CommitmentRoot, proof *Commitment // unless there is one item, when a ExistenceProof may work) func BatchVerifyMembership(spec *ProofSpec, root CommitmentRoot, proof *CommitmentProof, items map[string][]byte) bool { // decompress it before running code (no-op if not compressed) - once for batch - proof = Decompress(proof) + proof, err := Decompress(proof) + if err != nil { + return false + } for k, v := range items { valid := VerifyMembership(spec, root, proof, []byte(k), v) if !valid { @@ -78,7 +87,10 @@ func BatchVerifyMembership(spec *ProofSpec, root CommitmentRoot, proof *Commitme // (which should be a BatchProof, unless there is one item, when a NonExistenceProof may work) func BatchVerifyNonMembership(spec *ProofSpec, root CommitmentRoot, proof *CommitmentProof, keys [][]byte) bool { // decompress it before running code (no-op if not compressed) - once for batch - proof = Decompress(proof) + proof, err := Decompress(proof) + if err != nil { + return false + } for _, k := range keys { valid := VerifyNonMembership(spec, root, proof, k) if !valid { @@ -113,7 +125,10 @@ func CombineProofs(proofs []*CommitmentProof) (*CommitmentProof, error) { } else if batch := proof.GetBatch(); batch != nil { entries = append(entries, batch.Entries...) } else if comp := proof.GetCompressed(); comp != nil { - decomp := Decompress(proof) + decomp, err := Decompress(proof) + if err != nil { + return nil, err + } entries = append(entries, decomp.GetBatch().Entries...) } else { return nil, fmt.Errorf("proof neither exist or nonexist: %#v", proof.GetProof()) diff --git a/go/proof.go b/go/proof.go index 91fa232e..7f8e5120 100644 --- a/go/proof.go +++ b/go/proof.go @@ -97,7 +97,10 @@ func (p *CommitmentProof) Calculate() (CommitmentRoot, error) { return n.Calculate() } case *CommitmentProof_Compressed: - proof := Decompress(p) + proof, err := Decompress(p) + if err != nil { + return nil, err + } return proof.Calculate() default: return nil, errors.New("unrecognized proof type") diff --git a/go/vectors_test.go b/go/vectors_test.go index b6e0e909..5e1d6767 100644 --- a/go/vectors_test.go +++ b/go/vectors_test.go @@ -83,7 +83,10 @@ func TestDecompressBatchVectors(t *testing.T) { t.Fatalf("Marshal batch %v", err) } - decomp := Decompress(tc) + decomp, err := Decompress(tc) + if err != nil { + t.Fatalf("Failed to decompress test case") + } if decomp == tc { t.Fatalf("Decompression is a no-op") } From 1c9d7581f1f89a83e699b9639fa3bebade37e10e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 8 Oct 2024 12:27:42 +0200 Subject: [PATCH 3/8] test: add additional test case --- go/compress_test.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/go/compress_test.go b/go/compress_test.go index f7e557a4..562d2c51 100644 --- a/go/compress_test.go +++ b/go/compress_test.go @@ -43,7 +43,7 @@ func TestDecompressExist(t *testing.T) { expError error }{ { - "success single lookup", + "success: single lookup", func() { compressedExistProof.Path = []int32{0} }, @@ -56,7 +56,7 @@ func TestDecompressExist(t *testing.T) { nil, }, { - "success multiple lookups", + "success: multiple lookups", func() { compressedExistProof.Path = []int32{0, 1, 0, 2} }, @@ -68,6 +68,14 @@ func TestDecompressExist(t *testing.T) { }, nil, }, + { + "success: empty exist proof", + func() { + compressedExistProof = nil + }, + nil, + nil, + }, { "failure: path index out of bounds", func() { From c33dd0da36ebd14a8500c4fcd32bdbf48926727a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 8 Oct 2024 12:30:04 +0200 Subject: [PATCH 4/8] chore: changelog entry --- go/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/go/CHANGELOG.md b/go/CHANGELOG.md index d49eff71..ade2e080 100644 --- a/go/CHANGELOG.md +++ b/go/CHANGELOG.md @@ -4,6 +4,7 @@ - deps: bump golang to v1.22 ([#363](https://github.com/cosmos/ics23/pull/363)). - fix: guarantee that `spec.InnerSpec.MaxPrefixLength` < `spec.InnerSpec.MinPrefixLength` + `spec.InnerSpec.ChildSize` ([#369](https://github.com/cosmos/ics23/pull/369)) +- imp: change out of bound inner op lookups in compressed batch proofs to error returns. The `Decompress` function now returns an error, along with `decompress`, `decompressEntry`, and `decompressExist`. # v0.11.0 From 284999c335504b7984c92793a2d4cbab0daab990 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 8 Oct 2024 12:30:50 +0200 Subject: [PATCH 5/8] chore: changelog wording --- go/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/CHANGELOG.md b/go/CHANGELOG.md index ade2e080..7f4fe666 100644 --- a/go/CHANGELOG.md +++ b/go/CHANGELOG.md @@ -4,7 +4,7 @@ - deps: bump golang to v1.22 ([#363](https://github.com/cosmos/ics23/pull/363)). - fix: guarantee that `spec.InnerSpec.MaxPrefixLength` < `spec.InnerSpec.MinPrefixLength` + `spec.InnerSpec.ChildSize` ([#369](https://github.com/cosmos/ics23/pull/369)) -- imp: change out of bound inner op lookups in compressed batch proofs to error returns. The `Decompress` function now returns an error, along with `decompress`, `decompressEntry`, and `decompressExist`. +- imp: change panics on out of bound inner op lookups in compressed batch proofs to error returns. The `Decompress` function now returns an error, along with `decompress`, `decompressEntry`, and `decompressExist`. # v0.11.0 From 365ec7ad6e12411d7b45dc532a06737da24e8612 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 8 Oct 2024 12:37:28 +0200 Subject: [PATCH 6/8] lint --- go/compress_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/go/compress_test.go b/go/compress_test.go index 562d2c51..e7ebc4d5 100644 --- a/go/compress_test.go +++ b/go/compress_test.go @@ -15,16 +15,16 @@ func TestDecompressExist(t *testing.T) { Prefix: []byte{}, } innerOps := []*InnerOp{ - &InnerOp{ + { Hash: HashOp_SHA256, Prefix: generateInnerOpPrefix(), }, - &InnerOp{ + { Hash: HashOp_SHA256, Prefix: generateInnerOpPrefix(), Suffix: []byte{1}, }, - &InnerOp{ + { Hash: HashOp_SHA256, Prefix: generateInnerOpPrefix(), Suffix: []byte{2}, @@ -98,7 +98,7 @@ func TestDecompressExist(t *testing.T) { Leaf: leafOp, } - lookup = innerOps[:] + lookup = innerOps tc.malleate() From 891d26e8aae8de83f47e4376f87eae22ed7fb4c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 8 Oct 2024 12:40:22 +0200 Subject: [PATCH 7/8] lint spacing --- go/compress_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/go/compress_test.go b/go/compress_test.go index e7ebc4d5..6e154c8c 100644 --- a/go/compress_test.go +++ b/go/compress_test.go @@ -114,7 +114,6 @@ func TestDecompressExist(t *testing.T) { if tc.expError != nil && err.Error() != tc.expError.Error() { t.Fatalf("expected: %v, got: %v", tc.expError, err) } - }) } } From d7055a0b0b6b29a0598697228da8403e3aaaec5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 10 Oct 2024 13:45:50 +0200 Subject: [PATCH 8/8] chore: add comment --- go/compress.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/go/compress.go b/go/compress.go index 06c21011..0755a695 100644 --- a/go/compress.go +++ b/go/compress.go @@ -166,6 +166,8 @@ func decompressEntry(entry *CompressedBatchEntry, lookup []*InnerOp) (*BatchEntr } func decompressExist(exist *CompressedExistenceProof, lookup []*InnerOp) (*ExistenceProof, error) { + // when proving non-existence, if we are proving the leftmost key or rightmost key, + // the left existence proof or right existence proof will be nil, so we safely return. if exist == nil { return nil, nil }