v4: security fixes (12 audit findings + module bump) - #331
Closed
yycen wants to merge 12 commits into
Closed
Conversation
Flip constantTimeEnabled from 0 to 1 so the existing bigmod-backed constant-time path is the default for Paillier Decrypt and related private-key operations. The previous opt-in default left signing sessions exposed to a Kocher-class timing side channel that recovers LambdaN and, via the MtA share decryption, an honest party's ECDSA share — a P1 vulnerability per BNB Chain bug bounty (decryption vuln). Add TestConstantTimeOpsEnabledByDefault as a regression guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three ZK Verify paths previously admitted attacker-controlled multi-megabyte exponents into modular exponentiation, allowing any participant to peg honest verifiers' CPU for seconds per session: - crypto/mta/proofs.go (ProofBobWC.Verify): add upper bound S2/T2 < 2*q^3*NTilde derived from honest-prover sampling (e*rho + rhoPrm, rho < q*NTilde, rhoPrm < q^3*NTilde, e < q). Closes SRC-2026-771 / bnb-chain#15 ProofBob branch. - crypto/mta/range_proof.go (RangeProofAlice.Verify): same 2*q^3*NTilde upper bound for S2. Closes the same-shape DoS reported in v1.18 (RangeProofAlice has identical structure to ProofBob in the malformed-exponent path). - crypto/dlnproof/proof.go: switch raw range check on T[i]/Alpha[i] to operate on the unreduced big.Int (was `Mod(p.T[i], N)` which only constrained the residue). Adds nil/h1/h2/N guards and removes the redundant nil check inside the inner loop. Closes B17 (DLNProof oversized T[i] amplification, ~200x on toy 1024-bit N) and B14 (nil-deref in same loop) together. - crypto/facproof/proof.go (ProofFac.Verify): bound W1/W2 by 2*q^3*NCap, V by 4*q^3*N0*NCap, Sigma by q*N0*NCap, all derived from Fig.28 sampling (alpha,beta < q^3*sqrt(N0); mu,nu < q*NCap; sigma < q*N0*NCap; r < q^3*N0*NCap; e < q). Closes B18 (FacProof W1/W2/V amplification, ~165x on toy 2048-bit NCap). Production tests added in proof_test.go (facproof) and range_proof_test.go (mta) assert oversized response scalars are rejected before any modexp. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tightens six paths previously prone to panics, wire-reachable malformed scalars, or unauthenticated parameters: - ecdsa/signing/round_4.go: nil guard on thetaInverse after ModInverse, preventing the round_5.go ScalarMult nil-deref panic when malicious peers craft theta values summing to 0 mod q. Closes SRC-2026-759 / bnb-chain#12. - ecdsa/signing/round_9.go: change `!ok && len(values) != 4` to `!ok || len(values) != 4` so a peer cannot ship a 3-secret decommitment that survives the guard and panics at `values[3]` out-of-bounds. Closes B13. - crypto/schnorr/schnorr_proof.go: ZKProof.Verify and ZKVProof.Verify now reject zero-scalar T/U (and on-curve validate the public point) BEFORE entering ScalarMult. Closes B16 / SRC-2026-776 Path A (wire-reachable []byte{0x00} → ScalarBaseMult(0) panic) and the off-curve point sub-case (B15). - crypto/vss/feldman_vss.go: Share.Verify rejects zero/over-q shares, nil fields, and off-curve verifiers BEFORE the ScalarBaseMult call that produces the panic. Closes B19 (wire-reachable Share=[0x00] → ScalarBaseMult(0) panic). - ecdsa/resharing/round_4_new_step_2.go: enforce PaillierN.BitLen() == 2048 and NTilde.BitLen() == 2048, bringing resharing in line with keygen round_2. Closes B21. - tss/params.go: SetNoProofMod and SetNoProofFac now emit a warning log; struct comment marks them legacy compatibility only. Closes B20 (the unregistered NoProofMod counterpart of bnb-chain#9 NoProofFac) and addresses bnb-chain#9 deprecation messaging. Production tests added across schnorr_proof_test.go and feldman_vss_test.go validate that the new guards reject the attacker payloads at the wire-level entry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…chain#5) Replace the panic-on-error path in (*ECPoint).ScalarMult and crypto.ScalarBaseMult with a nil return. Both functions previously panicked when the curve produced an identity result (k ≡ 0 mod n) or any other off-curve coordinate, dragging the entire host process down via the wrapper's `panic(fmt.Errorf(...))`. The nil return lets callers detect the degenerate case and abort the session gracefully — the pattern adopted by the recent Schnorr (B16), VSS (B19), and Round-4 ModInverse (bnb-chain#12 / SRC-2026-759) hardening PRs, which had to layer upstream guards because of this panic behavior. Also add explicit-error variants `ScalarMultErr` and `ScalarBaseMultErr` for internal callers that prefer to propagate the off-curve error rather than the nil-on-error convention. EightInvEight now short-circuits to nil when the first ScalarMult returns nil, matching the rest of the API. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a ModProof for each party's own NTilde to KGRound2Message2 so
the receiver can verify that NTilde is a Blum-integer product of
safe primes — closing the smooth-subgroup NTilde injection path
that the existing 2048-bit bit-length check cannot detect.
- protob/ecdsa-keygen.proto: new repeated bytes field
`nTildeModProof` on KGRound2Message2 (proto field 3).
- ecdsa/keygen/round_2.go: each peer creates a second ModProof
using LocalPreParams.{P, Q} (the safe primes of NTilde, NOT
the Paillier SK primes) and includes it in the round-2 broadcast.
- ecdsa/keygen/round_3.go: verify the NTildeModProof against
round.save.NTildej[j]. Backward compatible — peers shipping
an empty NTildeModProof are accepted under the existing
NoProofMod() compatibility branch with a warning log,
matching the Paillier-N ModProof's compatibility handling.
- ecdsa/keygen/messages.go: NewKGRound2Message2 takes the new
proof and writes it into the wire field; adds
UnmarshalNTildeModProof.
Resharing path is unaffected (NTilde comes from the existing
keygen save data); the smooth-N concern there is mitigated by
the resharing round_4 bit-length gate added in B21.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nb-chain#13) Previously ecdsa/keygen, eddsa/keygen, and ecdsa/resharing fell back to ssidNonce = SetUint64(0) when the caller did not provide a SessionNonce, causing every keygen run on a given peer set to share the same SSID. Under SSID collision, an attacker who observes one honest keygen could replay DLN / ModProof / FacProof from session A into session B (specifically when the caller reuses preParams), per the v1.11 audit of CVE-2022-47930's residual gap. Hard-error rather than silently default — the caller is responsible for coordinating a fresh, agreed-upon nonce for each keygen / resharing run, as the GG20 session-binding doc has stated since the SessionNonce API was introduced. Signing keeps its message-hash fallback (per-message uniqueness is the right shape for that path). Test fixtures in ecdsa/{keygen,resharing}/local_party_test.go, eddsa/{keygen,resharing}/local_party_test.go are updated to call params.SetSessionNonce(big.NewInt(1)) immediately after NewParameters / NewReSharingParameters. BREAKING: callers of NewLocalParty for keygen or resharing must call params.SetSessionNonce(...) with a coordinator-assigned value before starting the protocol. Callers that previously relied on the implicit 0 fallback will receive a clear error message from round 1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GG18 §3 (defect D3) requires PaillierN > q^8 for secp256k1 — i.e. |N| >= 2048. Previously the only place this floor was enforced was ecdsa/keygen/round_2.go via `BitLen() != paillierBitsLen` after Unmarshal. Move the check into ValidateBasic so a 1-byte PaillierN / NTilde is rejected at the message boundary — useful for any caller that runs ValidateBasic without reaching round_2 (monitoring code, replay-detection, integration tests, etc.). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror the keygen B5 fix on the resharing message-decode layer. DGRound2Message1.ValidateBasic now rejects PaillierN / NTilde below 2048 bits at the message boundary, in addition to the existing bit-length check in resharing/round_4_new_step_2.go (B21). The previous NonEmptyBytes-only check matched the keygen behavior that B5 just tightened; resharing should not be the weaker layer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (B7) Previously mtaTimingProtection.ProtectBigInt was applied only when common.IsConstantTimeEnabled() returned true — the same flag that governs the CT modular-exponentiation path. With bnb-chain#14, the flag now defaults to true, but the response-time normalisation is a separate correctness concern (it caps the observable latency of Paillier Decrypt regardless of the modexp implementation) and must not be contingent on caller opt-in. Closes B7 in SECURITY_AUDIT_REPORT v1.22. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…C-2026-723 / bnb-chain#10) BobMid / BobMidWC previously returned errors.New(...) for a failed RangeProofAlice verification, identical in shape to internal arithmetic failures. round_2.go's error-attribution then blamed Pj for everything, including failures that may have been caused by the local party's own gamma / w inputs interacting with Pj's peer-supplied pkA. Introduce a sentinel `mta.ErrRangeProofVerify` that BobMid / BobMidWC return when the peer's RangeProofAlice rejection is the root cause. The signing round 2 attribution helper now wraps errors with a clearer "peer RangeProofAlice rejected" message in that case, and labels other arithmetic failures separately. Both error paths still attribute the culprit to Pj because HomoMult / HomoAdd / pkA-arithmetic failures most plausibly stem from peer-supplied pkA; the distinction is in diagnostic clarity, not blame routing. A full identifiable-abort framework (B9 / GG20 §3 IA) would route to the actual misbehaving party — that is left for the long-term CGGMP21 migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
local_only/ holds copies of audit reports, PoC tests, and any other untracked artefacts that should not enter the published history. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The session-bound NTilde ModProof, mandatory SessionNonce, hardened ZK Verify bounds, ScalarMult panic-to-nil shift, and stricter ValidateBasic semantics introduced in the previous ten commits all break the v3 public API, requiring a major version bump per Go semver: - NewKGRound2Message2 gained a required nTildeProof parameter - KGRound1Message / DGRound2Message1 ValidateBasic now reject N < 2048 bits - Keygen / resharing now error if Parameters.SessionNonce is unset - ScalarMult / ScalarBaseMult return nil instead of panicking - AliceEnd / AliceEndWC apply timing protection unconditionally - BobMid / BobMidWC surface a typed ErrRangeProofVerify sentinel - The KGRound2Message2 wire format adds nTildeModProof (proto field 3) Adjust go.mod, Makefile, and every internal import accordingly. The Makefile MODULE value was inadvertently left at v2 in a prior PR; it is restored to track go.mod (now v4). Also gitignore _local_only/ for off-tree audit artefacts. The leading underscore makes Go's `./...` skip the directory automatically. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pull Request ReviewThis PR introduces a major-version migration from Sensitive ContentNo sensitive content detected. Security IssuesNo serious security issues detected. Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Twelve commits closing the security findings from the recent audit.
The cumulative API and behavioural changes require a major-version
bump, so the module path moves from
tss-lib/v3totss-lib/v4.Findings closed
44a95b23e6f49f72eaa3c91b8692ScalarMult/ScalarBaseMultreturnnilinstead of panicking5946f07038f858SessionNonce9c81a5fKGRound1Message.ValidateBasicenforces |N| ≥ 2048b858ae1DGRound2Message1.ValidateBasicmirrors keygen0b86e22AliceEnd/AliceEndWCf8a894emta.ErrRangeProofVerifysentinel for cleaner attributionf8a5030_local_only/0cfe470Breaking changes (justifying v4)
NewKGRound2Message2takes a new requirednTildeProof *modproof.ProofMod.KGRound2Message2wire format addsnTildeModProof(proto field 3).Parameters.SessionNonceis unset; callers must callSetSessionNoncewith a coordinator-assigned value.(*ECPoint).ScalarMultandcrypto.ScalarBaseMultreturnnilon identity / off-curve results instead of panicking. Explicit-error variantsScalarMultErr/ScalarBaseMultErrare added.KGRound1Message/DGRound2Message1ValidateBasicreject sub-2048-bitPaillierN/NTilde.AliceEnd/AliceEndWCalways apply 200ms timing-protection padding; the previousIsConstantTimeEnabled()gate is removed.BobMid/BobMidWCreturn the new typedmta.ErrRangeProofVerifysentinel when the peer'sRangeProofAlicerejects.Test plan
go build ./...— cleango test -count=1 -timeout 800s ./...— 16/16 packages passgo.mod,Makefile, all 309 import sites atv4NewKGRound2Message2)🤖 Generated with Claude Code