v4: security fixes (12 audit findings + module bump)#332
Open
yycen wants to merge 34 commits into
Open
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 performs a major upgrade from module path Sensitive ContentNo sensitive content detected. Security IssuesNo serious security issues detected. Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
SortedPartyIDs.Less previously used `Cmp <= 0`, returning `true` for both `Less(a,b)` and `Less(b,a)` when keys are equal — a violation of Go's sort.Interface strict-weak-ordering contract that produces undefined behaviour with sort.Sort. Tighten to `Cmp < 0`. In the distinct-key case this is a behaviour-preserving change; the bug surfaces only when duplicate keys are passed to SortPartyIDs. Add an explicit duplicate-key panic at the entry of SortPartyIDs to fail fast on caller-side configuration errors that would otherwise produce subtle sort-order divergences. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add panic-on-construction checks for invalid threshold / partyCount combinations: require partyCount >= 2 (a real threshold scheme has at least two parties), threshold >= 1, and threshold < partyCount. Without these checks, invalid configurations propagated silently and surfaced as opaque panics deep in protocol execution (e.g. the PrepareForSigning "index of two parties are equal" trap or the VSS polynomial Create error path). Failing at construction time gives callers an immediate, actionable signal instead of a runtime crash buried in the protocol state machine. Two test fixtures (TestStartRound1Paillier, TestFinishAndSaveH1H2) were using a degenerate n=1 / t=1 scaffold to exercise Paillier prep in isolation; they now use n=2 / t=1 to satisfy the validation without changing the actual prep behaviour being tested (fixtures are overridden from disk shortly after). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tighten EdDSA Round 3 / finalisation against malformed partial signatures: - eddsa/signing/messages.go: SignRound3Message.ValidateBasic now rejects payloads exceeding 32 bytes. Previously NonEmptyBytes alone admitted >32-byte values, which then silently truncated inside bigIntToEncodedBytes / copyBytes and produced a scalar different from the one on the wire. - eddsa/signing/finalize.go: explicitly reject sj outside [1, L-1] before bigIntToEncodedBytes; attribute the failure to the actual culprit Pj rather than producing an opaque aggregate-level "signature verification failed". Both fixes are defensive: an attacker can still cause session abort by refusing to participate, but they no longer get to exploit the silent-truncation / no-blame paths to produce a degenerate verify failure with no attribution. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Each of the six protocol drivers (ecdsa/{signing,keygen,resharing},
eddsa/{signing,keygen,resharing}) previously stored incoming peer
messages with unconditional overwrite. A peer could send a second
message of the same round-type and silently replace the originally-
stored value — defeating the GG18/GG20 commit-reveal binding intent
(the attack chain itself does not close, because the commitment
scheme is hiding and Round 2/3 outputs lock in the original gamma,
but the library's contract was: caller is responsible for replay
protection).
Each case arm now rejects a stored-then-different message from a
peer:
selfIdx := p.PartyID().Index
isDup := fromPIdx != selfIdx
...
if isDup && p.temp.<round>s[fromPIdx] != nil &&
!tss.IsSameMessage(p.temp.<round>s[fromPIdx], msg) {
return dupErr()
}
p.temp.<round>s[fromPIdx] = msg
Two tss-lib semantics had to be preserved:
1. Round Start() pre-populates `temp.<round>s[self.Index]` with the
party's own outgoing message before broadcasting. The check skips
the slot when fromPIdx == self.Index so the party doesn't reject
its own pre-stored content.
2. The resharing test dispatcher (and any real at-least-once
transport) re-delivers identical messages to a party. The new
tss.IsSameMessage helper compares wire-encoded bytes so legitimate
redelivery is idempotent; only content-different replacement
trips the error.
The pre-existing comment "this does not handle message replays. we
expect the caller to apply replay and spoofing protection" is
updated to reflect the now-library-enforced behaviour.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pull Request ReviewThis PR introduces a major-version bump from Sensitive ContentNo sensitive content detected. Security IssuesNo serious security issues detected. Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
Re-run gofmt across the tree to normalize generated *.pb.go files and a struct field alignment in crypto/paillier/paillier.go that drifted from canonical formatting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pull Request ReviewThis PR delivers a major Sensitive ContentNo sensitive content detected. Security IssuesNo serious security issues detected. Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
levymeit
reviewed
May 24, 2026
| // LocalPreParams stores NTilde = P*Q where P, Q are safe primes; use | ||
| // these same primes (NOT the Paillier SK primes) for the NTilde proof. | ||
| nTildeModProof, err = modproof.NewProof(ContextI, round.save.NTildei, | ||
| round.save.LocalPreParams.P, round.save.LocalPreParams.Q, round.Rand()) |
There was a problem hiding this comment.
NTilde = (2p+1)(2q+1), but you passed Germain p, q into the mod proof.
Wrong factors, the proof would fail for honest parties.
The tests are still passing because e2e uses SetNoProofMod(), which skips this check. E2E should not set these noProof flags if you want proof verifications to run.
Contributor
Author
There was a problem hiding this comment.
Thanks @levymeit — both observations were spot on, and both are fixed in commit 6ba5e0d (now pushed to this branch):
-
Wrong factors:
round_2.gonow derives the safe primes(2p+1, 2q+1)fromLocalPreParams.P/Q(which store the Germain primes, used for the DLN subgroup order) and passes those tomodproof.NewProoffor the NTilde proof. The same shape is applied to the resharing path's newnTildeModProofin726937a. -
SetNoProofMod()in tests: removed from bothecdsa/keygen/local_party_test.goandecdsa/resharing/local_party_test.goin the same commit, so e2e tests now exercise themodProofandfacProofverification paths end-to-end.
NTilde = (2p+1)(2q+1), but round_2 was passing LocalPreParams.P, Q — the Germain primes p, q used for the DLN subgroup order — as the factors of NTilde to modproof.NewProof. The proof was built from wrong factors, so honest verifiers would reject it. The bug was masked because the keygen and resharing e2e tests called SetNoProofMod()/SetNoProofFac(), skipping proof generation and verification entirely. Remove those flags so the proof paths are exercised end-to-end, and derive the safe primes 2p+1, 2q+1 in round_2 from the stored Germain primes. Reported by @levymeit on PR bnb-chain#332. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The pre-existing Verify only ran trial division up to 1000 against pkN
and checked the per-iteration equality xi == pf[i]^pkN mod pkN. By
Fermat's little theorem, every prime pkN > 1000 satisfies
x^pkN ≡ x (mod pkN) for all x ∈ Z_{pkN}*, so a malicious prover with
a prime modulus could set pf[i] = xs[i] (the verifier-derived
challenge) and pass without proving any factorization. The
TestProofVerifyRejectsPrimePkN test constructs exactly that proof.
Add up-front validation that pkN is non-nil, positive, odd, ≥2048
bits, and composite (ProbablyPrime(30) → reject), and that every
pf[i] is a non-nil unit in Z_{pkN}*. Also reject nil k / nil or
off-curve ecdsaPub so GenerateXs cannot panic or loop on malformed
inputs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…site
ProofMod.Verify called isQuadraticResidue (big.Jacobi) on W and N
before checking N's shape, so a malformed message with N == nil,
N <= 0, or even N would panic inside big.Jacobi. The oddness and
ProbablyPrime checks were also late, leaving an obvious DoS surface
on the verifier.
Move the N validation (nil, sign, odd, ≥ 2048 bits, composite via
Miller-Rabin 30 rounds) to the top of Verify so the proof is rejected
cleanly before any modular arithmetic runs. Drop the TODO comment
("add basic properties checker") that was tracking this gap. Add
TestVerifyRejectsMalformedN covering each rejected shape including
the prime-N case.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add two cross-cutting validation helpers in common/: - IsUsableUnknownOrderModulus(n, minBits): non-nil, positive, odd, composite (Miller-Rabin 30 rounds), and at least minBits long. Used to reject prime / undersized / malformed N and NCap before any modular operation that would otherwise panic or accept a trivially passing modulus. - IsCanonicalGenerator(n, v): v in (1, n) and coprime to n. Stricter than IsNumberInMultiplicativeGroup which accepts v == 1. Apply to DLN Proof.Verify: - Require N to satisfy IsUsableUnknownOrderModulus(N, 2048). - Require h1, h2, and each Alpha[i] to be canonical generators. Replaces the earlier (h mod N) check, which let non-canonical wire bytes h1+N pass even though their mod-N reduction landed in (1, N); the raw bytes are what downstream consumers see when they hash or compare h1/h2 outside the Verify boundary. Apply to FacProof.Verify: - Require N0 and NCap to satisfy IsUsableUnknownOrderModulus. - Require s, t to be canonical generators of Z_NCap*, and s != t. - Require prover-supplied commitments P, Q, A, B, T to be units of Z_NCap* (IsNumberInMultiplicativeGroup). The equality checks in the Sigma relation took these as raw big integers, so non-unit / zero / >= NCap values could otherwise pass without binding the prover to a real factorization commitment. Add TestDLNVerifyRejectsMalformedInputs and extend the FacProof test file with TestVerifyRejectsMalformedInputs covering each rejected shape (prime modulus, h = 1, h = N, h shares factor, s == t, P = 0, T = NCap, etc.). DLN test uses a sync.Once-cached safe-prime fixture so each subtest reuses the parameters (~2s total instead of ~150s). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add IsCanonicalPaillierCiphertext(c, N) to common/validation.go and apply group-membership / canonical-encoding checks to both MtA verifiers: RangeProofAlice.Verify: - pk.N and NTilde must satisfy IsUsableUnknownOrderModulus. - h1, h2 must be canonical generators of Z_NTilde* and distinct. - c must be a canonical Paillier ciphertext (0 < c < N², gcd(c,N)=1). Replaces the standalone gcd(c, N) check. ProofBobWC.Verify / ProofBob.Verify: - Same modulus / generator checks for pk.N, NTilde, h1, h2. - c1 and c2 must both be canonical Paillier ciphertexts. - In the with-check branch, reject e == 0 (consistent with Schnorr), enforce X.ValidateBasic() and same-curve as ec, and guard X.ScalarMult(e) and ScalarBaseMult against nil returns before the .Add() / .Equals() chain. Previously a malformed direct-API X or a zero challenge could panic with nil dereference inside the proof equality check. Add TestRangeProofAliceVerifyRejectsMalformedInputs and TestProofBobWCVerifyRejectsMalformedInputs covering each rejected shape (prime moduli, h1 == h2, h1 == 1, c == 0, c ≥ N², etc.). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
For Weierstrass curves (secp256k1) the identity is the point-at-infinity encoded as (0, 0), which fails the affine curve equation and is already rejected by isOnCurve. For Edwards curves (Ed25519) the affine identity is (0, 1) and IS on-curve, so it passed ValidateBasic untouched. A malicious party could submit (0, 1) as a Schnorr commitment Alpha or as the proven public key X in EdDSA keygen / signing message unmarshalling (both go through crypto.NewECPoint), reducing the proof to trivial "prove discrete log of identity is 0". Changes: - crypto/ecpoint.go: add IsIdentity() method covering Edwards (0, 1) and the (0, 0) infinity placeholder. ValidateBasic now additionally rejects identity. Honest paths in tss-lib never produce identity as a commitment (proven by the full e2e test suite still passing). - crypto/schnorr/schnorr_proof.go: in ZKProof.Verify and ZKVProof.Verify add a same-curve check (tss.SameCurve) between Alpha / X and Alpha / V / R, plus nil-return guards around ScalarBaseMult / ScalarMult so degenerate cases return false rather than panic on a nil .Add chain. Tests: - TestIsIdentityAndValidateBasic covers Edwards (0, 1) rejection plus the (0, 0) placeholder. - TestSchnorrEdwardsIdentityRejected exercises both Alpha = identity and X = identity rejection paths on tss.Edwards(). - TestSchnorrCurveMismatch covers cross-curve Alpha vs X rejection. Low-order Edwards point rejection (cofactor-8 subgroup) is deferred: the existing EightInvEight projection in VSS / round_3 already clears the small-subgroup component for protocol-critical commitments, and a standalone subgroup-membership check would cost a full ScalarMult by the curve order per verification. Tracked as a follow-up in the audit document. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ProofBobWC.Verify checked gcd(V, pk.N²) at line ~273 and gcd(V, pk.N) at line ~290. These are equivalent (N and N² share the same prime factors), so the second was redundant — drop it. RangeProofAlice.Verify missed the gcd(S, pk.N) check that its sibling ProofBob.Verify already had. Honest S = r^e · beta mod N is a unit (beta is sampled coprime to N, r is in Z_N*), so requiring it directly matches the existing pattern and removes a small asymmetry between the two MtA verifiers. Add a test that forges S = sk.P (a prime factor of pk.N, so gcd > 1 but still inside the pk.N range) to confirm rejection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Clears the trivial P2/P3 backlog from the ZKP audit: - modproof.Verify: gcd(Z[i],N)==1 and gcd(X[i],N)==1 for prover-supplied roots. Honest provers always produce units; the checks close a defense- in-depth gap. Covered by TestVerifyRejectsNonUnitZX. - vss.Share.Verify: guard vjt = vs[j].ScalarMult(t) against nil before calling .Add to remove a latent nil-deref. (The earlier proposal to also enforce canonical 0 < share.ID < q was dropped: tss generates 256-bit random PartyID keys and Ed25519's q ≈ 2^252, so most honest IDs exceed q. share.ID is a Lagrange x-coordinate reduced mod q downstream, so the existing `idModQ.Sign() == 0` rejection is the correct binding requirement.) - facproof.Verify, mta.RangeProofAlice.Verify, mta.ProofBob/WC.Verify: reject e == 0 after Fiat-Shamir challenge derivation, matching the Schnorr verifier. Negligible under Fiat-Shamir but a zero challenge collapses the Σ-relation binding. - mta.ProofBobWC.Verify: explicit pf.U.ValidateBasic() in the WC branch. The deserialization path already validates via NewECPoint, but direct API consumers can bypass that. - commitments.HashCommitDecommit.Verify: nil-receiver guard and per- element nil check for the decommitment array. common.SHA512_256i panics on nil *big.Int entries (unlike _TAGGED). Covered by TestVerifyHandlesMalformedInputs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tss.SortPartyIDs deduplicates PartyIDs by raw KeyInt() bytes, but every
downstream consumer (vss.Shares.ReConstruct, ecdsa/eddsa signing
prepare.go Lagrange coefficient computation) treats the key as
`KeyInt() mod q`. A malicious party that registers with key =
honest_key + q has byte-distinct but mod-q-equal keys; SortPartyIDs
lets the configuration through, then signing prepare.go computes
`ModInverse((ksj - ksi) mod q, q) = ModInverse(0, q) = nil` and panics
on the next `modQ.Mul`. The result is a remote-triggerable signing-time
DoS.
Two layers of fix:
1) tss/params.go: NewParameters and NewReSharingParameters now panic
up-front if any pair of party keys collides mod q. The check is a
one-pass scan keyed on `KeyInt() mod q .Text(16)`. This catches the
bad configuration before any protocol round runs and gives a clear,
locally-attributable error pointing at the two colliding keys.
2) {ecdsa,eddsa}/signing/prepare.go: PrepareForSigning previously
panicked on `ksj.Cmp(ksi) == 0` (raw-byte equality only). Convert
to a returned error and compare residues `ksj mod q` vs `ksi mod q`
so direct API consumers that bypass tss.NewParameters still get a
clean error instead of a deep ModInverse panic. Callers in
{ecdsa,eddsa}/signing/round_1.go and ecdsa/resharing/round_1 and
eddsa/resharing/round_1 updated to propagate the error.
Tests in tss/params_test.go cover the rejected configuration (k vs
k+q) and the honest configuration (k vs k+1).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four bite-sized items from the ZKP audit cleanup pass:
1) common.IsInIntervalPositive(b, bound) helper added (open lower
bound `(0, bound)`, vs IsInInterval's `[0, bound)`). Applied to
FacProof's six prover-supplied range-check fields (Z1, Z2, W1, W2,
Sigma, V); the honest prover samples all six via GetRandomPositiveInt,
so b == 0 is never produced by the spec and is now rejected.
2) paillier.Proof.Verify rejects k.Sign() < 0. GenerateXs hashes
k.Bytes() (absolute value), so a negative k would alias to the same
transcript as +|k| and could silently produce ambiguous proofs.
3) paillier.Proof.Verify docstring now states that the caller is
responsible for verifying ecdsaPub.Curve() matches the expected
curve. The verifier itself only consults ecdsaPub through
ValidateBasic (on-curve relative to the point's own stored curve);
external reuse outside the keygen flow should add an explicit
tss.SameCurve check up-front.
4) common.ModReduceHash is the new primary name for what was
misleadingly called common.RejectionSample. The function is a
one-line `eHash.Mod(eHash, q)` — not rejection sampling. The
updated docstring explains the actual bias bound and points callers
that need a uniform sample to paillier.GenerateXs (which does real
rejection sampling). RejectionSample is kept as a Deprecated alias.
All internal call sites in crypto/{facproof,modproof,mta,schnorr}
migrated to ModReduceHash to clear the deprecation warnings.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Every proof type previously used the caller-supplied Session as the
sole tag input to SHA512_256i_TAGGED. Because each proof's transcript
has a different arity / length-encoded shape, cross-proof collisions
were not an immediate concern, but the lack of explicit domain
separation made auditing harder and would have left a trap for any
future proof that happens to align with an existing transcript shape.
Each proof package now prepends a stable, hierarchical FS domain tag
to Session before hashing:
tss-lib.v4.modproof (modproof.NewProof / Verify)
tss-lib.v4.facproof (facproof.NewProof / Verify)
tss-lib.v4.dlnproof (dlnproof.NewDLNProof / Verify)
tss-lib.v4.schnorr.zk (schnorr.NewZKProof / ZKProof.Verify)
tss-lib.v4.schnorr.zkv (schnorr.NewZKVProof / ZKVProof.Verify)
tss-lib.v4.mta.range-alice (mta.ProveRangeAlice / RangeProofAlice.Verify)
tss-lib.v4.mta.bob (mta.ProveBob / ProofBob.Verify variant)
tss-lib.v4.mta.bob-wc (mta.ProveBobWC / ProofBobWC.Verify variant)
The tags are inlined into a per-package `fsSession*` helper alongside
the existing constants. The "bob" vs "bob-wc" split mirrors the
existing if-X-nil dispatch inside ProofBob{,WC}, making the two MtA
Bob variants disjoint even at the FS tag level.
This is wire-incompatible with v3 by design — proofs minted under the
v3 module path will not verify under v4. Consumed by the existing v4
module bump.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both NewProof and Verify previously derived each Fiat-Shamir challenge
Y_i as `ModReduceHash(N, SHA512_256i_TAGGED(...))`. For 2048-bit
NTilde this was effectively a no-op reduction — the 256-bit hash
output fit entirely inside [0, 2^256) ⊂ [0, N), so Y_i lived in a
roughly 2^-1792 fraction of the intended Z_N domain. The
Paillier-Blum proof's formal soundness assumes Y <- Z_N (or J_N);
collapsing the support to a 256-bit subset does not introduce a known
attack against the 80-iteration repetition, but it's a textbook-vs-
implementation gap that a reviewer should not have to defend.
Replace with `sampleYModN`, an expand-then-reject sampler:
1. Seed = SHA512_256i_TAGGED(fsSession(Session), transcript...), padded
to 32 bytes.
2. For counter = 0, 1, 2, ... expand `ceil(N.BitLen()/256)` blocks via
counter-mode SHA512_256(seed, counter, blockIdx).
3. Mask the concatenated output to exactly N.BitLen() bits.
4. Accept if candidate < N; otherwise retry with next counter.
Because N ∈ (2^{bitlen-1}, 2^bitlen] after masking, the rejection
probability per attempt is at most 1/2 — practical loop length is
1–2 iterations. The pad-to-32-byte step makes the seed → block
function invariant of leading-zero quirks in big.Int.Bytes(). The
counter is 4 bytes (uint32, big-endian) and the block index is a
single byte, so they don't ambiguously concatenate with the seed.
Wire-incompatible with v3 by design — proofs minted under the v3
module path will not verify under v4. Consumed by the existing v4
module bump. Existing modproof e2e tests (TestMod, TestModCT,
TestAttackMod, TestVerifyRejectsNonUnitZX, TestVerifyRejectsMalformedN)
all still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switch RangeProofAlice.Verify and ProofBob{,WC}.Verify range checks
on prover-supplied Z / U / W / S / ZPrm / T / V from
`common.IsInInterval(_, bound)` (closed lower bound `[0, bound)`) to
`common.IsInIntervalPositive(_, bound)` (open lower bound
`(0, bound)`).
All six values are derived from group operations on units in
`Z_NTilde*` (Z, ZPrm, T, W), `Z_{N²}*` (U, V), or `Z_N*` (S), so an
honest prover never produces 0 — the iteration that would produce 0
has negligible probability and is anyway already caught downstream by
the gcd checks added in `0f67b50`/`087f6ae`/`625b8bf`. Tightening the
lower bound now matches the policy applied to FacProof in `3091f00`
and removes a small asymmetry the audit doc had flagged as a
candidate follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
For Ed25519 (cofactor 8) on-curve membership is strictly weaker than
prime-order subgroup membership: there are 8 small-order points
(orders 1, 2, 4, 8) that satisfy the curve equation and would pass
ECPoint.ValidateBasic, but lie outside the prime-order subgroup. A
malicious party can submit such a point as a Schnorr commitment, VSS
share, or MtA WC commitment to mount small-subgroup-style attacks.
Add ECPoint.IsInPrimeOrderSubgroup() — computes `[curve.N] * p` and
checks the result is identity. For prime-order curves (secp256k1,
NIST family, cofactor 1) this is structurally guaranteed by IsOnCurve
and the explicit check costs only one extra ScalarMult; for Ed25519
the check is load-bearing.
Add ECPoint.ValidateInSubgroup() as the stricter sibling of
ValidateBasic, used wherever an EC point arrives from attacker-
controlled wire bytes:
- crypto/schnorr/schnorr_proof.go: ZKProof.Verify (X, Alpha) and
ZKVProof.Verify (V, R, Alpha).
- crypto/vss/feldman_vss.go: every vs[j] commitment.
- crypto/mta/proofs.go: ProofBobWC.Verify's X and pf.U in the with-
check branch.
To avoid paying the ScalarMult on prime-order curves where the check
is structurally redundant, ValidateInSubgroup short-circuits via
tss.HasCompositeCofactor(p.curve). tss/curve.go gains the per-curve
cofactor classification (secp256k1 → false, ed25519 → true, unknown
→ true conservatively).
Closes the explicitly-deferred 🔥 P1 item from the ZKP audit. The
EightInvEight() projection in eddsa/{keygen,signing,resharing}/
round_* still runs at the protocol level, so the new check is a
defense-in-depth backstop for direct API callers and for code paths
not covered by EightInvEight (Schnorr Alpha, MtA pf.U). Performance
cost: ~30-50µs per Ed25519 point validated; ECDSA paths are unchanged.
Test in crypto/ecpoint_test.go covers a synthesized Ed25519 order-2
point (0, p-1), a prime-order point, the identity (which is rejected
by ValidateBasic before the subgroup check), and a secp256k1 sanity
case. Full ECDSA / EdDSA e2e keygen / resharing / signing suites
still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Six audit follow-ups in the VSS module, all in crypto/vss: 1. Shares.ReConstruct: reject mod-q ID collision with a returned error instead of triggering ModInverse(0, q) → nil → modN.Mul nil-receiver panic. Same shape as the dcb631d signing/prepare.go fix. Not on the production code path (no call sites outside tests) but exported, so external callers shouldn't be able to crash the library by submitting shares with k vs k+q IDs. 2. Shares.ReConstruct: reject empty shares slice up-front instead of panicking on shares[0].Threshold index access for non-nil but empty input. 3. Shares.ReConstruct: per-share validation — non-nil share, non-nil ID/Share, and consistent threshold across all shares. Without these the Lagrange loop would silently mix shares of different threshold polynomials or nil-deref. 4. Create: bound check num < threshold+1 (was num < threshold). A degree-`threshold` polynomial needs at least threshold+1 distinct shares to reconstruct; the old check admitted num == threshold which produced an unreconstructable share set. Production callers (keygen round_1, resharing round_1_old_step_1) already satisfy the tighter bound via tss.NewParameters' partyCount > threshold invariant, so no honest path regresses. 5. Create: reject nil ec and nil rand up-front; the previous code would nil-deref ec.Params().N or panic inside GetRandomPositiveInt. 6. Share.Verify: replace four `vs[j].SetCurve(ec)` mutations with an explicit `tss.SameCurve(vs[j].Curve(), ec)` gate. The earlier code silently re-attached ec to whatever curve the caller had assigned to vs[j], which hid curve-confusion mistakes and leaked a mutating side-effect into the caller's ECPoint instances. The new check rejects mismatched-curve vs[j] up-front; honest paths (production keygen always passes vs constructed via UnFlattenECPoints(ec, ...)) are unaffected. New tests cover each rejection path: nil ec/rand, num == threshold boundary, curve mismatch, empty shares, nil share element, mixed threshold, and the k-vs-k+q mod-q collision case (with NotPanics). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ECDSA resharing was the last⚠️ P1 item in the ZKP audit. Keygen already linked peer NTilde to a Blum-integer ModProof in `6ba5e0d` (via `KGRound2Message2.nTildeModProof`, verified in ecdsa/keygen/round_3.go before NTildej is saved). Resharing didn't carry the same binding: `DGRound2Message1` shipped Paillier ModProof + NTilde + H1 + H2 + two DLN proofs, and round_4_new_step_2 saved NTildej / H1j / H2j after the Paillier and DLN checks — leaving NTilde itself only "locally plausible" rather than proven to be the safe-prime product the QR-group argument assumes. This commit mirrors the keygen pattern in resharing: - protob/ecdsa-resharing.proto: `DGRound2Message1` gains a `repeated bytes nTildeModProof = 8` field. Optional on the wire so v3-vintage peers that omit it can be accepted under `params.NoProofMod()`; v4 default mode (the production setting in this branch's e2e test, see `0f67b50` removing the resharing SetNoProofMod) requires the proof. - ecdsa/resharing/messages.go: `NewDGRound2Message1` takes the new `nTildeModProof` argument; `UnmarshalNTildeModProof` mirrors the keygen helper. - ecdsa/resharing/round_2_new_step_1.go: generate the proof alongside the Paillier ModProof, using the safe-prime factors derived from `LocalPreParams.P/Q` (same fix shape as keygen's `6ba5e0d`). - ecdsa/resharing/round_4_new_step_2.go: verify the proof in the same goroutine as the Paillier ModProof, attributing failures to the peer via `paiProofCulprits`. Honors `NoProofMod()` for v3 backward compatibility. Other regenerated `.pb.go` diffs in the commit are protoc-tooling churn from `make protob` rebuilding all protos; no semantic changes to other messages. Full e2e ecdsa/resharing and eddsa/resharing suites still pass. Closes the last⚠️ P1 row in `_local_only/ZKP_BASIC_PROPERTY_AUDIT.md`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two self-review findings on top of the pre-push review pass: 1. tss/params.go assertDistinctIDsModQ now also panics when any PartyID's KeyInt() mod q is exactly 0 (previously it only caught collisions between two distinct keys with the same residue). A party with KeyInt() ≡ 0 mod q is fatal: in Shamir secret sharing f(0) is the secret, so the polynomial evaluated at that party's "x-coordinate" gives them the raw secret outright. Even if keygen blocks the bad ID via vss.Create's CheckIndexes, the signing path (PrepareForSigning in ecdsa/signing/prepare.go) would, when the quorum includes the zero-residue party as one of the j != i others, compute iota = ksc * ModInverse(ksc - ksj) = 0, then bigWj.ScalarMult(0) returns nil, then the next ScalarMult panics. Direct API consumers loading legacy LocalPartySaveData and going straight to signing previously bypassed the vss.Create check; asserting at the NewParameters layer closes that path. 2. common/random.go GetRandomPositiveInt now strictly returns a value in the open interval (0, lessThan). The previous implementation sampled from [0, lessThan), so the name's "Positive" claim was technically false and verifiers using IsInIntervalPositive (added in 3091f00 / 004f48e) on these sampler outputs had a 1/lessThan ≈ 2^-256 chance of incorrectly rejecting an honest proof. Probability was negligible but the API/comment contract was inconsistent; the loop now rejects try.Sign() == 0 alongside the existing try >= lessThan rejection. Also reject lessThan <= 1 up-front (no value exists in (0, 1)). Tests in common/random_test.go and tss/params_test.go cover the new rejection paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dennislky
approved these changes
May 27, 2026
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
3 participants
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
Major version bump from
tss-lib/v3→v4carrying two waves ofsecurity work plus a few module-hygiene commits.
Wave 1 — initial audit response (12 commits, original PR scope):
Paillier constant-time decrypt by default, ZK response-scalar bounds,
panic-DoS guards,
NTildeModProof shipped in keygen, mandatorySessionNonce,|N| ≥ 2048floors, unconditional MtA timing padding,sentinel errors, etc.
Wave 2 — broader ZKP verifier audit pass (17 commits, follow-up
inside this same PR). Triggered by @levymeit's review comment flagging
wrong factors in the NTilde ModProof and the test-side
SetNoProofMod()bypass; both fixes opened up a systematic re-read of every
Verify()in
crypto/. Each verifier is now audited for nil checks, rangechecks, group membership (
gcd(x, N) = 1), canonical-inputenforcement, EC subgroup membership, and Fiat-Shamir transcript
discipline.
Wave 1 — original audit findings (12)
44a95b23e6f49f72eaa3c91b8692ScalarMult/ScalarBaseMultreturnnilinstead of panicking5946f07038f858SessionNonce9c81a5fKGRound1Message.ValidateBasicenforces |N| ≥ 2048b858ae1DGRound2Message1.ValidateBasicmirrors keygen0b86e22AliceEnd/AliceEndWCf8a894emta.ErrRangeProofVerifysentinel for cleaner attributionf8a5030_local_only/0cfe470Wave 2 — ZKP verifier audit pass (17), grouped by area
Paillier / NTilde ModProof:
6ba5e0dSetNoProofMod()in e2e (fixes @levymeit's review comment)d3e8502paillier.Proof.Verifyrejects primepkN(Fermat bypass), nil inputs, non-unitpf[i]89892a1modproof.VerifyvalidatesNbeforebig.Jacobi; 2048-bit floor; reject primea756377Y_iuniform sampling viasampleYModN(expand-then-reject); closes 256-bit support set under 2048-bitN726937aDGRound2Message1.nTildeModProofgenerate + verify, mirrors keygen6ba5e0dDLN / FacProof / MtA group-membership:
0f67b50IsUsableUnknownOrderModulus/IsCanonicalGenerator; apply to DLN + FacProof (canonicalh1,h2,P/Q/A/B/T)087f6aeRangeProofAlice/ProofBob{,WC}: canonical Paillier ciphertext + NTilde/h1/h2 generators + WC EC branch nil-safe +e == 0guard625b8bfProofBobWCredundantgcd(V, N)dedupe + missinggcd(S, N)inRangeProofAlice004f48eIsInIntervalPositiveconsistency with FacProofbbbc9e7Z[i]/X[i]; VSSvjtnil guard;e == 0in FacProof / RangeProofAlice / ProofBob; explicitpf.U.ValidateBasic;HashCommitDecommitnil guards)EC point / curve handling:
eff1777ECPoint.IsIdentityrejects Edwards(0, 1); Schnorr same-curve check + nil-return guardsba34365ECPoint.IsInPrimeOrderSubgroup+ValidateInSubgroup;tss.HasCompositeCofactor; applied to Schnorr / VSS / MtA — rejects Ed25519 low-order pointsFiat-Shamir / transcript hygiene:
4a9a88btss-lib.v4.<proof>); 8 disjoint tags3091f00common.IsInIntervalPositive;paillier.Proof.Verifyrejects negativek+ curve-doc; renameRejectionSample→ModReduceHashVSS:
76fa474Create/Verify/ReConstructhardening (mod-q ID collision → error not panic; nil-share guard;Createboundary off-by-one; removeSetCurvemutation)Party-ID / Lagrange invariants:
dcb631dtss.NewParametersrejects mod-q duplicatePartyIDs (signing-time Lagrange DoS);PrepareForSigningpanic → errorba6d6f3assertDistinctIDsModQalso rejects zero-residue Keys;GetRandomPositiveIntreturns strictly(0, lessThan)Breaking changes (justifying v4)
Original (Wave 1):
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.Additional (Wave 2):
NewDGRound2Message1takes a new requirednTildeModProof *modproof.ProofMod; wire format addsnTildeModProof(proto field 8).{ecdsa,eddsa}/signing.PrepareForSigningreturns an additionalerror(was panic on mod-q collision).Y_iderivation changed to expand-then-reject sampling — same v3↔v4 wire incompatibility.tss.NewParameters/NewReSharingParametersnow panic up-front on duplicate-mod-q or zero-residuePartyIDconfigurations (the panic was previously thrown deeper insidesigning/prepare.go).vss.Share.Verifyno longer mutates the caller's*ECPointviaSetCurve; same-curve mismatch is now an explicit rejection.common.RejectionSampleis now aDeprecated:alias ofcommon.ModReduceHash(semantics unchanged; old name was misleading).common.GetRandomPositiveIntnow returns strictly> 0; previously could return 0 with1/lessThanprobability.Test plan
go build ./...— cleango test -short -timeout 1200s ./...— all 18 packages passgo.mod,Makefile, all import sites atv4NewKGRound2Message2,NewDGRound2Message1, orsigning.PrepareForSigning)🤖 Generated with Claude Code