Skip to content

Address residual review items from BNB hardening stack#6

Open
mswilkison wants to merge 1 commit into
codex/remove-unused-protocolsfrom
codex/review-residual-cleanup
Open

Address residual review items from BNB hardening stack#6
mswilkison wants to merge 1 commit into
codex/remove-unused-protocolsfrom
codex/review-residual-cleanup

Conversation

@mswilkison
Copy link
Copy Markdown

Summary

Stacked on #5. Doc + small defensive-code follow-ups closing out the cosmetic and pre-existing items raised in the #2/#4/#5 review thread. None are security-blocking; they did not warrant individual PRs but are worth landing alongside the main hardening stack.

  • common/hash_utils.go — strengthen RejectionSample doc: 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 path emits e ∈ [0, 2^256) while the legacy path emits e ∈ [-(2^256-1), 2^256), and note that FactorVerify's CmpAbs bounds exist to accommodate the legacy signed encoding.
  • tss/params.go — expand SetSessionNonceBytes docstring with the per-ceremony-uniqueness and high-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 its 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) → nil → nil-deref. Pre-existing hazard; not introduced by the hardening stack. Adds focused test coverage for each rejection path.

Validation

  • go build ./... clean
  • go vet ./... clean
  • go test -count=1 ./common ./crypto ./crypto/vss passed
  • go test -count=1 -timeout 600s ./crypto/paillier ./tss passed
  • git diff --check passed

Test plan

  • VSS ReConstruct rejection-path tests (nil share, nil ID/Share, zero-mod-q ID, duplicate ID) all assert NotPanics + error return
  • Existing VSS, common, crypto, tss test suites still green
  • CI on the stacked branch

@mswilkison mswilkison force-pushed the codex/remove-unused-protocols branch from b366e27 to 3284c6b Compare May 21, 2026 17:10
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>
@mswilkison mswilkison force-pushed the codex/review-residual-cleanup branch from fb8602e to c9e9c09 Compare May 21, 2026 17:12
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.

1 participant