celestiaorg / celestia-node

Celestia Data Availability Nodes
Apache License 2.0
926 stars 925 forks source link

blob: CommitmentProof.Verify should check and ensure that subtreeRootThreshold is >= 1 lest a divide-by-zero-panic #3728

Closed odeke-em closed 1 month ago

odeke-em commented 1 month ago

Celestia Node version

1a1286f454c1dd670af856153a8fc8674cd91192

OS

darwin

Install tools

No response

Others

No response

Steps to reproduce it

While fuzzing the code for CommitmentProof.Verify, if I run this code

package blob_test       

import (
        "testing"

        "github.com/celestiaorg/celestia-node/blob"
)       

func TestCommitmentProofVerifyZeroSubThreshold(t *testing.T) {
        cp := new(blob.CommitmentProof)
        _, _ = cp.Verify(nil, 0)
}

we get a panic

Expected result

No panic and instead an error

Actual result

panic: runtime error: integer divide by zero
            goroutine 9589 [running]:
            runtime/debug.Stack()
                /Users/emmanuelodeke/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.0.darwin-amd64/src/runtime/debug/stack.go:26 +0x9b
            testing.tRunner.func1()
                /Users/emmanuelodeke/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.0.darwin-amd64/src/testing/testing.go:1591 +0x1c8
            panic({0x106c66820?, 0x108121230?})
                /Users/emmanuelodeke/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.0.darwin-amd64/src/runtime/panic.go:785 +0x132
            github.com/celestiaorg/go-square/inclusion.SubTreeWidth(0xc0002596b7?, 0x1?)
                /Users/emmanuelodeke/go/pkg/mod/github.com/celestiaorg/go-square@v1.1.0/inclusion/blob_share_commitment_rules.go:63 +0x212
            github.com/celestiaorg/celestia-node/blob.(*CommitmentProof).Verify(0xc00c545c20, {0x0, 0x0, 0x0}, 0x0)
                /Users/emmanuelodeke/go/src/github.com/celestiaorg/celestia-node/blob/commitment_proof.go:101 +0x25a
            github.com/celestiaorg/celestia-node/blob.FuzzCommitmentProofVerify.func1(0x0?, {0xc00112fb00, 0x17, 0xd80})
                /Users/emmanuelodeke/go/src/github.com/celestiaorg/celestia-node/blob/blob_fuzz_test.go:83 +0xfb
            reflect.Value.call({0x106bca9a0?, 0x106f64180?, 0x13?}, {0x1060d7eca, 0x4}, {0xc00d0580f0, 0x2, 0x2?})
                /Users/emmanuelodeke/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.0.darwin-amd64/src/reflect/value.go:581 +0xca6
            reflect.Value.Call({0x106bca9a0?, 0x106f64180?, 0x103a35ded?}, {0xc00d0580f0?, 0x106f5fe00?, 0xf?})
                /Users/emmanuelodeke/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.0.darwin-amd64/src/reflect/value.go:365 +0xb9
            testing.(*F).Fuzz.func1.1(0xc00d053ba0?)
                /Users/emmanuelodeke/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.0.darwin-amd64/src/testing/fuzz.go:335 +0x305
            testing.tRunner(0xc00d053ba0, 0xc00d055320)
                /Users/emmanuelodeke/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.0.darwin-amd64/src/testing/testing.go:1690 +0xf4
            created by testing.(*F).Fuzz.func1 in goroutine 22
                /Users/emmanuelodeke/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.0.darwin-amd64/src/testing/fuzz.go:322 +0x577

Suggested fix

diff --git a/blob/commitment_proof.go b/blob/commitment_proof.go
index 8fa74671..f9d1af06 100644
--- a/blob/commitment_proof.go
+++ b/blob/commitment_proof.go
@@ -2,6 +2,7 @@ package blob

 import (
    "bytes"
+   "errors"
    "fmt"

    "github.com/celestiaorg/celestia-app/v2/pkg/appconsts"
@@ -93,6 +94,10 @@ func (commitmentProof *CommitmentProof) Verify(root []byte, subtreeRootThreshold
        numberOfShares += proof.End() - proof.Start()
    }

+   if subtreeRootThreshold <= 0 {
+       return false, errors.New("subtreeRootThreshould must be > 0")
+   }
+
    // use the computed total number of shares to calculate the subtree roots
    // width.
    // the subtree roots width is defined in ADR-013:

/cc @liamsi @rootulp @musalbas

odeke-em commented 1 month ago

I have mailed out a PR with the fixes along with the fuzzers per https://github.com/celestiaorg/celestia-node/pull/3732