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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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 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 10/11] 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 cf1e6adc94a60cdedb043a10286e8c59a98bdbc7 Mon Sep 17 00:00:00 2001 From: Aditya <14364734+AdityaSripal@users.noreply.github.com> Date: Mon, 21 Oct 2024 10:27:40 +0200 Subject: [PATCH 11/11] Update go/ops_test.go --- 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 a76254a8..b3045941 100644 --- a/go/ops_test.go +++ b/go/ops_test.go @@ -86,7 +86,7 @@ func TestValidateIavlOps(t *testing.T) { fmt.Errorf("expected remaining prefix length to be 0, got: 1"), }, { - "failure: invalid reminaing length with non-zero layer number", + "failure: invalid remaining length with non-zero layer number", func() { layerNum = 1 op.(*InnerOp).Prefix = append(op.(*InnerOp).Prefix, []byte{1}...)