fix(crypto): rebase and verify constant-time hardening backport onto current master - #11
Open
piotr-roslaniec wants to merge 7 commits into
Open
piotr-roslaniec wants to merge 7 commits into
piotr-roslaniec wants to merge 7 commits into
Conversation
Port the upstream constant-time framework (common/constant_time.go, built on filippo.io/bigmod) and wire it, behind a default-off runtime toggle, into the secret-exponent modular exponentiations in Paillier Decrypt/Proof and the DLN, factor, and Paillier-Blum modulus proofs. Mitigates the timing side-channel gap (CVE-2023-26557 class) flagged in the threshold-ECDSA variant analysis. - common: EnableConstantTimeOps / DisableConstantTimeOps / IsConstantTimeEnabled toggle + CTModInt (ExpCT / ModInverseCT / MulCT) over filippo.io/bigmod. - Wired ops use odd moduli only; phi(N)-even inverses stay on math/big. - Default off: zero behavior/perf change unless EnableConstantTimeOps() is called. - Bump go directive 1.16 -> 1.23 (filippo.io/bigmod floor). - Tests: framework equivalence, per-proof byte-identical/verifies equivalence, and full constant-time keygen + signing end-to-end.
Close the secret-exponent coverage gap and fix correctness/robustness issues in the opt-in constant-time path: - Harden the remaining secret-exponent modexps: MtA ProveBobWC (h1^x, h1^y), ProveRangeAlice (h1^m), the ring-Pedersen trapdoor setup in keygen (h1i^alpha), and Paillier Encrypt (gamma^m) / HomoMult (c1^m). - Pad the exponent to a fixed width in ExpCT so its running time no longer leaks the secret exponent's magnitude. - ModInverseCT returns nil for non-coprime inputs (matching math/big.ModInverse) instead of a silent wrong value. - reduceToPaddedBytes reduces unconditionally (no secret-dependent branch). - Assert odd modulus at CTModInt construction (fail at build, not at Exp). - Remove the unused TimingProtection / ConstantTimeCompare / Mod() API (the jitter helper also discarded a rand error -> nil-deref panic path). - Document the hardening coverage scope in the package doc. - Add equivalence/regression tests for every new site.
Follow-up to the go.mod merge conflict resolution when rebasing the constant-time-hardening branch onto current master: go mod tidy moves the indirect dependencies (previously listed inline from the branch's older base) into their own require block with versions resolved against master's current module graph. No functional change.
The constant-time-hardening branch's tests were written against an older base, before this fork's own independent hardening added: (1) the 2048-bit floor on dlnproof.Verify's modulus (verifyMinModulusBitLen), (2) the mandatory per-ceremony SetSessionNonce on ECDSA keygen/signing Start(), and (3) the mandatory positive fullBytesLen argument on ECDSA signing constructors. Without these adaptations the ported tests fail/panic against current master even though the underlying constant-time crypto is unaffected: - crypto/dlnproof/constant_time_equiv_test.go: use 1024-bit safe primes (NTilde ~2048 bits) instead of 512-bit, matching the verifyMinModulusBitLen=2048 floor and this fork's own convention (ecdsa/keygen/prepare.go safePrimeBitLen=1024). - ecdsa/keygen/constant_time_e2e_test.go: call params.SetSessionNonce(big.NewInt(1)) before Start(), as required by ecdsa/keygen/round_1.go's fail-closed session-nonce check. - ecdsa/signing/constant_time_e2e_test.go: same SetSessionNonce call, plus pass the now-mandatory fullBytesLen=32 argument to NewLocalParty (ecdsa/signing/local_party.go's validateFullBytesLen).
Two small zero-risk backports bundled together (same file, adjacent sections): - Size errCh to concurrency instead of concurrency*numPrimes (backport of upstream bnb-chain/tss-lib commit 4c83ace). At most 'concurrency' goroutines run at once, each sending at most one error, so the larger buffer was wasted allocation. No functional change. - Fix p=2q+1 typo in GetRandomSafePrimesConcurrent doc comment (backport of upstream commit 27922e0). Doc-only; no functional change.
…_vss.Create Backport of upstream bnb-chain/tss-lib commit b7b73a0. samplePolynomial already sets v[0] = secret (crypto/vss/feldman_vss.go), so this assignment was a no-op. Verified against fork's current samplePolynomial before applying.
…g style consistent Backport of upstream bnb-chain/tss-lib commit 0629cff. Behaviorally identical (skip nil errs vs. include non-nil errs); pure style.
This was referenced Sep 14, 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
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
Rebases and verifies this fork's own unmerged
constant-time-hardeningbranch(commits
50ccd47,4aee357) onto currentmaster, closing the one realsecurity gap identified in a recent dependency-drift/variant-analysis audit of
this fork against
bnb-chain/tss-lib: the optional constant-timemodular-exponentiation hardening from upstream PR bnb-chain#328 (
3709c25,7a10240,0735081, CVE-2023-26557-class timing side-channel), which this fork's ownCHANGELOG.mdexplicitly lists as "Not ported / deferred" and a residualrisk ("the optional constant-time work is not integrated").
A literal cherry-pick of upstream's PR bnb-chain#328 commits is not viable — this fork
restructured
crypto/facproof/crypto/modproofintocrypto/paillier/{factor_proof,mod_proof}.goand independently addedtagged-hash/session-context Fiat-Shamir changes to the exact functions
upstream's commits touch. This fork already had better ground for the
backport: its own unmerged
constant-time-hardeningbranch, whichindependently reimplements the same fix (same upstream-derived
common/constant_time.godesign built onfilippo.io/bigmod, the sameconstant-time bignum library used inside Go's own
crypto/rsa/crypto/ecdsa)directly against this fork's current code shape.
Relationship to #8: #8 opened directly from
constant-time-hardeningagainst
masterat an earlier point and is nowCONFLICTING(unrebased,marked "DO NOT MERGE"). This PR is that same branch rebased onto current
master, with the resulting conflicts/fixups resolved and the whole suiteverified — a supersedes/replaces relationship. Recommend closing #8 in favor
of this one once reviewed.
What's included (7 commits)
feat(crypto): add opt-in constant-time path for secret-exponent modexps—rebase of the branch's
50ccd47. Addscommon/constant_time.go(
CTModInt.ExpCT/ModInverseCT/MulCT,EnableConstantTimeOpstoggle)and wires it into Paillier Decrypt/Proof and the DLN/factor/modulus proofs.
feat(crypto): extend constant-time coverage and harden CT primitives—rebase of the branch's
4aee357, auto-merged cleanly onto master.chore(deps): reconcile indirect dependency graph via go mod tidy— theone real rebase conflict was in
go.mod: the branch's base (pre-datingthis fork's PR Remove unused EdDSA and resharing protocols #5) still had the EdDSA dependencies (
agl/ed25519,decred/dcrd/dcrec/edwards/v2) that PR Remove unused EdDSA and resharing protocols #5 removed. Resolved by keepingmaster's dependency set and adding only the new
filippo.io/bigmodrequirement, then
go mod tidyto reconcile the indirect graph.test(constant-time): adapt ported branch tests to current master API—the branch's tests were written before three pieces of hardening that
landed on
masterafterward (the 2048-bit DLN modulus floor, mandatoryper-ceremony
SetSessionNonce, mandatoryfullBytesLen). Without theseadaptations
TestDLNProofCTVerifiesand bothTestE2EConcurrentConstantTimetests fail/panic even though the underlying constant-time crypto logic is
unaffected.
fix(safe_prime): correct errCh buffer sizing and p=2q+1 doc typostyle(ecdsa/keygen): make round_3 multiErr accumulation error-handling style consistentcleanup(vss): remove redundant poly[0] = secret assignment in feldman_vss.CreateCommits 5-7 are small, independently-verified zero-risk backports of upstream
trivia (a stale doc comment, an over-sized channel buffer, a style
inconsistency, a dead-code assignment) found alongside the constant-time work
during the same audit; they have no security relevance and can be reviewed/
landed independently of 1-4 if preferred.
Verification
go build ./...- clean.go vet ./...- no issues.go mod tidy- resolves cleanly; no EdDSA-only dependencies reintroduced.go test -p 2 -timeout 20m ./...- all 213 tests pass across 13packages, run twice: once on this exact tree before push, and once more
against a fresh clone of the pushed branch as a final sanity check.
Design notes carried over from the source branch
common.EnableConstantTimeOps()is opt-in), matchingupstream's own default and this fork's stated "deferred to a separate
follow-up with benchmarking and side-channel review" caveat. Turning it on
by default is a separate decision this PR does not make.
signatures change.
Non-goals / explicitly out of scope
Per the same audit, the other named security item in scope
(SRC-2026-644, EdDSA round-3 nil-pointer crash) does not apply to this fork -
eddsa/keygen,eddsa/signing,eddsa/resharingwere deleted entirely inPR #5, so there is no
eddsa/signing/round_3.goand no attack surface. Notaddressed here because there is nothing to address.