From feb2ac33ff3f90d101a778d91cbc6d838bc42690 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:24:06 +0200 Subject: [PATCH 01/12] refactor: deobfuscate, many thanks to Damian for the help! --- go/ops.go | 62 +++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 22 deletions(-) diff --git a/go/ops.go b/go/ops.go index 8bda8f4e..8e8e651f 100644 --- a/go/ops.go +++ b/go/ops.go @@ -20,38 +20,56 @@ 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. +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 err + } + if int(height) < 0 || int(height) < layerNum { + return fmt.Errorf("IAVL height (%d) must be non-negative and less than 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 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 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 { + if remLen != 0 { + return fmt.Errorf("expected remaining prefix to be 0, got: %d", r2) } } else { - if !(r2^(0xff&0x01) == 0 || r2 == (0xde+int('v'))/10) { + // 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("invalid op") } - 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 From f94ee2a33de77d14861f20a1a57ee8e7320a5727 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:27:49 +0200 Subject: [PATCH 02/12] fix: test + build --- go/ops.go | 2 +- go/ops_test.go | 2 +- go/proof_test.go | 2 +- testdata/TestCheckAgainstSpecData.json | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/go/ops.go b/go/ops.go index 8e8e651f..f941a384 100644 --- a/go/ops.go +++ b/go/ops.go @@ -57,7 +57,7 @@ func validateIavlOps(op opType, layerNum int) error { remLen := r.Len() if layerNum == 0 { if remLen != 0 { - return fmt.Errorf("expected remaining prefix to be 0, got: %d", r2) + return fmt.Errorf("expected remaining prefix to be 0, got: %d", remLen) } } else { // when the child comes from the left, the suffix if filled in diff --git a/go/ops_test.go b/go/ops_test.go index e2d23912..a2138c03 100644 --- a/go/ops_test.go +++ b/go/ops_test.go @@ -114,7 +114,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 less than the layer number (1)"), }, { "failure: inner prefix starts with leaf prefix", diff --git a/go/proof_test.go b/go/proof_test.go index fd55e019..2c27169c 100644 --- a/go/proof_test.go +++ b/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()) } }) } diff --git a/testdata/TestCheckAgainstSpecData.json b/testdata/TestCheckAgainstSpecData.json index 94a00943..88c152f4 100644 --- a/testdata/TestCheckAgainstSpecData.json +++ b/testdata/TestCheckAgainstSpecData.json @@ -907,7 +907,7 @@ "hash": 1 } }, - "Err": "inner, unexpected EOF" + "Err": "inner, IAVL height (0) must be non-negative and less than the layer number (3)" }, "rejects only inner proof (hash mismatch)": { "Proof": { From e97c91db98103c214ec2133f3918b7ab8bad356d 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:31:01 +0200 Subject: [PATCH 03/12] Update go/ops.go --- go/ops.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/ops.go b/go/ops.go index f941a384..3c754fd8 100644 --- a/go/ops.go +++ b/go/ops.go @@ -57,7 +57,7 @@ func validateIavlOps(op opType, layerNum int) error { remLen := r.Len() if layerNum == 0 { if remLen != 0 { - return fmt.Errorf("expected remaining prefix to be 0, got: %d", remLen) + return fmt.Errorf("expected remaining prefix length to be 0, got: %d", remLen) } } else { // when the child comes from the left, the suffix if filled in From f329a250055867e62c1a7545f04eb2363c48ee0a 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:33:27 +0200 Subject: [PATCH 04/12] chore: update error message to be more informative --- go/ops.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/ops.go b/go/ops.go index 3c754fd8..3ddf4022 100644 --- a/go/ops.go +++ b/go/ops.go @@ -66,7 +66,7 @@ func validateIavlOps(op opType, layerNum int) error { // 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("invalid op") + return fmt.Errorf("remainder of prefix must be of length 1 or 34, got: %d", remLen) } if op.GetHash() != HashOp_SHA256 { return fmt.Errorf("IAVL hash op must be %v", HashOp_SHA256) From b3d6417734d7b864e53015df4ff63d4605db35e1 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:35:57 +0200 Subject: [PATCH 05/12] chore: error wording --- go/ops.go | 2 +- go/ops_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/go/ops.go b/go/ops.go index 3ddf4022..70a892ec 100644 --- a/go/ops.go +++ b/go/ops.go @@ -32,7 +32,7 @@ func validateIavlOps(op opType, layerNum int) error { return err } if int(height) < 0 || int(height) < layerNum { - return fmt.Errorf("IAVL height (%d) must be non-negative and less than the layer number (%d)", height, layerNum) + return fmt.Errorf("IAVL height (%d) must be non-negative and greater than or equal to the layer number (%d)", height, layerNum) } size, err := binary.ReadVarint(r) diff --git a/go/ops_test.go b/go/ops_test.go index a2138c03..d16f3969 100644 --- a/go/ops_test.go +++ b/go/ops_test.go @@ -114,7 +114,7 @@ func TestInnerOpCheckAgainstSpec(t *testing.T) { func() { innerOp.Prefix = []byte{0x01} }, - fmt.Errorf("IAVL height (-1) must be non-negative and less than the layer number (1)"), + 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", From a14131c2afc41fe6e534db31d76ee6aa253ee332 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:39:41 +0200 Subject: [PATCH 06/12] fix: test --- testdata/TestCheckAgainstSpecData.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testdata/TestCheckAgainstSpecData.json b/testdata/TestCheckAgainstSpecData.json index 88c152f4..2c087f0a 100644 --- a/testdata/TestCheckAgainstSpecData.json +++ b/testdata/TestCheckAgainstSpecData.json @@ -907,7 +907,7 @@ "hash": 1 } }, - "Err": "inner, IAVL height (0) must be non-negative and less than the layer number (3)" + "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": { From f4e01fc7606ae2f3167d78ab03736f28645bf4ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 14 Oct 2024 14:39:09 +0200 Subject: [PATCH 07/12] test: add unit tests --- go/ops.go | 6 +-- go/ops_test.go | 118 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 3 deletions(-) diff --git a/go/ops.go b/go/ops.go index 70a892ec..4314403f 100644 --- a/go/ops.go +++ b/go/ops.go @@ -29,7 +29,7 @@ func validateIavlOps(op opType, layerNum int) error { height, err := binary.ReadVarint(r) if err != nil { - return err + 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) @@ -37,7 +37,7 @@ func validateIavlOps(op opType, layerNum int) error { size, err := binary.ReadVarint(r) if err != nil { - return err + return fmt.Errorf("failed to read IAVL size varint: %w", err) } if int(size) < 0 { @@ -46,7 +46,7 @@ func validateIavlOps(op opType, layerNum int) error { version, err := binary.ReadVarint(r) if err != nil { - return err + return fmt.Errorf("failed to read IAVL version varint: %w", err) } if int(version) < 0 { diff --git a/go/ops_test.go b/go/ops_test.go index d16f3969..a76254a8 100644 --- a/go/ops_test.go +++ b/go/ops_test.go @@ -9,6 +9,124 @@ 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 TestLeafOp(t *testing.T) { cases := LeafOpTestData(t) From d8f9ef693fa72cf7a6e8dec35f797ade1fd63ec7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 14 Oct 2024 14:40:29 +0200 Subject: [PATCH 08/12] chore: update godocs as per review suggestion --- go/ops.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/ops.go b/go/ops.go index 4314403f..3f2bb223 100644 --- a/go/ops.go +++ b/go/ops.go @@ -23,7 +23,7 @@ import ( // 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. +// length. The layerNum is the tree depth. func validateIavlOps(op opType, layerNum int) error { r := bytes.NewReader(op.GetPrefix()) From b097d9af37879946e7dfd6da7441adac175119f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 16 Oct 2024 11:27:25 +0200 Subject: [PATCH 09/12] Update go/ops.go Co-authored-by: Damian Nolan --- go/ops.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/ops.go b/go/ops.go index 3f2bb223..071b5c34 100644 --- a/go/ops.go +++ b/go/ops.go @@ -23,7 +23,7 @@ import ( // 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 tree depth. +// 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()) From f92b2699ed3cc25669cb5c4841f0eca89dc0ab31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 17 Oct 2024 11:36:28 +0200 Subject: [PATCH 10/12] refactor: add stricter checks for iavl spec and tendermint spec --- go/ops.go | 36 +++++++++++++++++++++++++ go/ops_test.go | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+) diff --git a/go/ops.go b/go/ops.go index 3f2bb223..95200c42 100644 --- a/go/ops.go +++ b/go/ops.go @@ -56,10 +56,19 @@ func validateIavlOps(op opType, layerNum int) error { // 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 { + // inner node + // // when the child comes from the left, the suffix if filled in // prefix: height | size | version | length byte (1 remainder) // @@ -75,6 +84,26 @@ func validateIavlOps(op opType, layerNum int) error { 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 { @@ -168,6 +197,13 @@ func (op *InnerOp) CheckAgainstSpec(spec *ProofSpec, b int) error { } } + 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) diff --git a/go/ops_test.go b/go/ops_test.go index a76254a8..3203f32a 100644 --- a/go/ops_test.go +++ b/go/ops_test.go @@ -127,6 +127,77 @@ func TestValidateIavlOps(t *testing.T) { } } +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: append([]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) From 20e9f426b8c102a8cfa0382da4a932ff418c250c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 17 Oct 2024 11:37:55 +0200 Subject: [PATCH 11/12] refactor: remove one line fn --- go/ops.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/go/ops.go b/go/ops.go index 3f2bb223..8c07cf80 100644 --- a/go/ops.go +++ b/go/ops.go @@ -120,7 +120,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 @@ -161,7 +161,7 @@ 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 @@ -246,10 +246,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 From b853f3d8a5126a77e1a49e3a879d0b1ebb9638ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 17 Oct 2024 11:40:16 +0200 Subject: [PATCH 12/12] lint --- go/ops_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/ops_test.go b/go/ops_test.go index 3203f32a..f4b1bc7b 100644 --- a/go/ops_test.go +++ b/go/ops_test.go @@ -179,7 +179,7 @@ func TestValidateTendermintOps(t *testing.T) { t.Run(tc.name, func(t *testing.T) { op = &InnerOp{ Hash: HashOp_SHA256, - Prefix: append([]byte{1}), + Prefix: []byte{1}, Suffix: []byte{1}, }