Skip to content

Backport tBTC-relevant BNB v4 hardening#4

Merged
piotr-roslaniec merged 6 commits into
masterfrom
codex/bnb-332-tbtc-fixes
Jun 15, 2026
Merged

Backport tBTC-relevant BNB v4 hardening#4
piotr-roslaniec merged 6 commits into
masterfrom
codex/bnb-332-tbtc-fixes

Conversation

@mswilkison

@mswilkison mswilkison commented May 21, 2026

Copy link
Copy Markdown

Summary

Stacked on #2. This draft backports the BNB bnb-chain#332 fixes that map cleanly to Threshold's current ECDSA/tBTC path without taking BNB's v4 module-path churn or EdDSA/package cleanup.

Included:

  • Bound additional verifier-controlled response scalars before modular exponentiation:
    • DLN Alpha and nil input guards.
    • Threshold Paillier FactorProof W1, W2, Sigma, and V absolute bounds.
    • MtA/RangeProof exact upper-bound rejection and nil modulus guards.
  • Harden panic-DoS paths used by ECDSA signing/keygen:
    • ECPoint scalar multiplication returns nil on invalid inputs.
    • Schnorr proof verifiers reject invalid public points and zero/out-of-range scalars before scalar multiplication.
    • VSS share verification rejects nil, zero, and out-of-range shares/verifiers before scalar multiplication.
    • ECDSA signing round 4 rejects nil theta inverse.
    • ECDSA signing round 9 fixes the decommitment guard from && to ||.
  • Add mta.ErrRangeProofVerify and signing-round attribution text for peer range-proof rejection.
  • Reject non-2048-bit Paillier/NTilde values in ECDSA keygen round-1 message validation, matching the later round-2 contract.

Review follow-up:

  • Removed unused ScalarMultErr / ScalarBaseMultErr public API surface.
  • Confirmed the only production BobMid / BobMidWC callers are in ecdsa/signing/round_2.go, where mta.ErrRangeProofVerify attribution is now wired.
  • Added comments around the MtA and FactorProof response bounds to document why MtA uses exclusive upper bounds while Threshold FactorProof keeps its existing inclusive absolute-bound style.
  • The stricter DLN Alpha check is verifier-only tightening. Honest provers already produce Alpha in (1, N); this rejects malformed values that previously only passed after reduction modulo N.
  • Stabilized a pre-existing EdDSA keygen test flake exposed by CI by passing the reconstructed scalar to edwards.PrivKeyFromScalar as a fixed-width 32-byte buffer.
  • Pinned the existing DLN nil-proof-element rejection with a no-panic regression test for nil Alpha and nil T entries.
  • Made the ProofBobWC.Verify nil result check after xE.Add(pf.U) explicit.
  • Added regression tests for the round-9 decommitment-length guard (extracted as decommitFour, mutation-detectable against the &&|| fix) and the just-below-2048 boundary of the keygen/resharing hasBitLen checks.

Not included in this draft:

Validation

  • go test -count=1 ./crypto ./crypto/dlnproof ./crypto/mta ./crypto/paillier ./crypto/schnorr ./crypto/vss ./ecdsa/keygen ./ecdsa/signing
  • go test -count=1 ./ecdsa/resharing
  • go test -count=1 ./eddsa/keygen ./eddsa/signing ./eddsa/resharing
  • go test -count=1 ./crypto/mta ./ecdsa/signing after the final MtA nil-guard tweak
  • go test -count=1 -timeout 25m ./ecdsa/keygen after the review follow-up commit
  • go test -count=1 -timeout 25m ./eddsa/keygen after the CI scalar-width flake fix
  • go test -count=1 ./crypto/dlnproof ./crypto/mta after the Gemini follow-up commit
  • git diff --check
  • go test -count=1 -run TestDecommitFour ./ecdsa/signing and go test -count=1 -timeout 25m ./ecdsa/keygen ./ecdsa/resharing after the new test additions.

@mswilkison mswilkison marked this pull request as ready for review May 21, 2026 15:54
@mswilkison mswilkison force-pushed the codex/bnb-332-tbtc-fixes branch from 492bdd6 to c09f596 Compare May 21, 2026 17:08
mswilkison added a commit that referenced this pull request May 21, 2026
Doc + small defensive-code follow-ups to the PR #2/#4/#5 review thread.
None of these are security-blocking; they close out the cosmetic and
pre-existing items that did not warrant their own PR earlier.

- common/hash_utils.go: strengthen RejectionSample doc so the name does
  not mislead future readers — the function is modular reduction, not
  true rejection sampling, and the bias bound depends on q vs the hash
  width.
- crypto/paillier/factor_proof.go: document that the tagged and legacy
  FactorChallenge paths emit different challenge distributions
  (positive-only vs signed), and note that FactorVerify's CmpAbs bounds
  exist to accommodate the legacy signed encoding.
- tss/params.go: expand SetSessionNonceBytes docstring with the
  collision/uniqueness/entropy guidance reviewers asked for.
- ecdsa/{keygen,signing}/rounds.go: document the round-1-capture
  invariant for getSSID so future refactors do not silently drift the
  hashed round.number domain separator.
- crypto/ecpoint.go: flag SetCurve's in-place mutation in the doc
  comment; the chained-call style is a footgun on shared points.
- crypto/vss/feldman_vss.go: reject nil shares, nil/zero share IDs, and
  duplicate IDs in ReConstruct before the Lagrange loop, so malformed
  inputs return an error instead of panicking through ModInverse(0).
  Add focused test coverage for each rejection path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
piotr-roslaniec pushed a commit that referenced this pull request Jun 15, 2026
Doc + small defensive-code follow-ups to the PR #2/#4/#5 review thread.
None of these are security-blocking; they close out the cosmetic and
pre-existing items that did not warrant their own PR earlier.

- common/hash_utils.go: strengthen RejectionSample doc so the name does
  not mislead future readers — the function is modular reduction, not
  true rejection sampling, and the bias bound depends on q vs the hash
  width.
- crypto/paillier/factor_proof.go: document that the tagged and legacy
  FactorChallenge paths emit different challenge distributions
  (positive-only vs signed), and note that FactorVerify's CmpAbs bounds
  exist to accommodate the legacy signed encoding.
- tss/params.go: expand SetSessionNonceBytes docstring with the
  collision/uniqueness/entropy guidance reviewers asked for.
- ecdsa/{keygen,signing}/rounds.go: document the round-1-capture
  invariant for getSSID so future refactors do not silently drift the
  hashed round.number domain separator.
- crypto/ecpoint.go: flag SetCurve's in-place mutation in the doc
  comment; the chained-call style is a footgun on shared points.
- crypto/vss/feldman_vss.go: reject nil shares, nil/zero share IDs, and
  duplicate IDs in ReConstruct before the Lagrange loop, so malformed
  inputs return an error instead of panicking through ModInverse(0).
  Add focused test coverage for each rejection path.
@piotr-roslaniec piotr-roslaniec force-pushed the codex/bnb-332-tbtc-fixes branch from 5e9e99e to e041e82 Compare June 15, 2026 09:01
mswilkison and others added 6 commits June 15, 2026 09:18
The round-9 `!ok || len(values) != 4` guard prevents a malicious peer from
binding their commitment to more than four secrets, sending the same payload
back as the decommitment, and letting round 9 silently read `values[0..3]`
as attacker-chosen Uj/Tj coordinates. No hash collision is required, since
the attacker controls both C and D in their own messages.

Extract the check into `decommitFour` and add focused tests that fail when
the operator is reverted to `&&` (the pre-fix logic).
`hasBitLen` was tightened from `>= bits` to `== bits` in the review-feedback
pass of this hardening series, and the same exact-equality contract lives in
`ecdsa/keygen/round_2.go`. Existing tests cover BitLen=1 (way under) and
BitLen=2049 (over by one) but leave the just-below-2048 case implicit; add
explicit assertions in both the keygen and resharing message test files so
a future relaxation of the comparison cannot silently let a 2047-bit
modulus through.
Record the tBTC-relevant BNB bnb-chain#332 defensive hardening as non-breaking
entries (EC point/Schnorr/DLN/FactorProof/MtA/VSS verifier guards, the
round-9 decommitment and round-4 nil-inverse fixes, and the keygen
round-1 modulus-width check), plus the new mta.ErrRangeProofVerify
sentinel. Tag entries with PR #4 and add it to the composing-PRs list.
No new wire/source breaking changes vs PR #2.
@piotr-roslaniec piotr-roslaniec force-pushed the codex/bnb-332-tbtc-fixes branch from e041e82 to ee9b6c5 Compare June 15, 2026 09:19
piotr-roslaniec pushed a commit that referenced this pull request Jun 15, 2026
Doc + small defensive-code follow-ups to the PR #2/#4/#5 review thread.
None of these are security-blocking; they close out the cosmetic and
pre-existing items that did not warrant their own PR earlier.

- common/hash_utils.go: strengthen RejectionSample doc so the name does
  not mislead future readers — the function is modular reduction, not
  true rejection sampling, and the bias bound depends on q vs the hash
  width.
- crypto/paillier/factor_proof.go: document that the tagged and legacy
  FactorChallenge paths emit different challenge distributions
  (positive-only vs signed), and note that FactorVerify's CmpAbs bounds
  exist to accommodate the legacy signed encoding.
- tss/params.go: expand SetSessionNonceBytes docstring with the
  collision/uniqueness/entropy guidance reviewers asked for.
- ecdsa/{keygen,signing}/rounds.go: document the round-1-capture
  invariant for getSSID so future refactors do not silently drift the
  hashed round.number domain separator.
- crypto/ecpoint.go: flag SetCurve's in-place mutation in the doc
  comment; the chained-call style is a footgun on shared points.
- crypto/vss/feldman_vss.go: reject nil shares, nil/zero share IDs, and
  duplicate IDs in ReConstruct before the Lagrange loop, so malformed
  inputs return an error instead of panicking through ModInverse(0).
  Add focused test coverage for each rejection path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
piotr-roslaniec added a commit that referenced this pull request Jun 15, 2026
Record PR #7's non-breaking hardening: signing round-9 decommitment
curve-point validation (layered on PR #4's decommitFour guard), round-1
hashed-message range validation, and the Paillier FactorVerify
distinct-generator check. Add PR #7 to the composing-PRs list. No new
breaking changes.
piotr-roslaniec added a commit that referenced this pull request Jun 15, 2026
Add the exported tss.ReSharingParameters / NewReSharingParameters and
crypto.ECPoint.EightInvEight removals to the Removed section (both are
source/compile breaks), correct the source-compatibility note that
wrongly claimed a single source break, drop the stale EightInvEight
reference from the PR #4 EC-guard entry, and fix the protob/message.proto
attribution.
piotr-roslaniec pushed a commit that referenced this pull request Jun 15, 2026
Doc + small defensive-code follow-ups to the PR #2/#4/#5 review thread.
None of these are security-blocking; they close out the cosmetic and
pre-existing items that did not warrant their own PR earlier.

- common/hash_utils.go: strengthen RejectionSample doc so the name does
  not mislead future readers — the function is modular reduction, not
  true rejection sampling, and the bias bound depends on q vs the hash
  width.
- crypto/paillier/factor_proof.go: document that the tagged and legacy
  FactorChallenge paths emit different challenge distributions
  (positive-only vs signed), and note that FactorVerify's CmpAbs bounds
  exist to accommodate the legacy signed encoding.
- tss/params.go: expand SetSessionNonceBytes docstring with the
  collision/uniqueness/entropy guidance reviewers asked for.
- ecdsa/{keygen,signing}/rounds.go: document the round-1-capture
  invariant for getSSID so future refactors do not silently drift the
  hashed round.number domain separator.
- crypto/ecpoint.go: flag SetCurve's in-place mutation in the doc
  comment; the chained-call style is a footgun on shared points.
- crypto/vss/feldman_vss.go: reject nil shares, nil/zero share IDs, and
  duplicate IDs in ReConstruct before the Lagrange loop, so malformed
  inputs return an error instead of panicking through ModInverse(0).
  Add focused test coverage for each rejection path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
piotr-roslaniec added a commit that referenced this pull request Jun 15, 2026
Record PR #7's non-breaking hardening: signing round-9 decommitment
curve-point validation (layered on PR #4's decommitFour guard), round-1
hashed-message range validation, and the Paillier FactorVerify
distinct-generator check. Add PR #7 to the composing-PRs list. No new
breaking changes.
@piotr-roslaniec piotr-roslaniec changed the base branch from integrate-bnb-hardening to master June 15, 2026 17:17
@piotr-roslaniec piotr-roslaniec merged commit 62acaf8 into master Jun 15, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants