fix(crypto): extend constant-time coverage to Schnorr proofs and signing rounds 3-5 - #23
piotr-roslaniec wants to merge 7 commits into
Conversation
…t v1.4.0 notes NOT ready to merge/push. Exploratory work pending reconciliation with the live #8/#10/#11/#17 stack (mswilkison) which already rebases+hardens the CT backport more rigorously than this commit (bounded exponent widths, overflow rejection, mode snapshotting) and already flips constantTimeEnabled's default directly. This commit's own contribution -- CT coverage for crypto/schnorr/schnorr_proof.go and ecdsa/signing/round_3-5.go, which none of #8/#10/#11/#17 touch -- is real and not yet duplicated elsewhere, but should land as an addition on top of that stack, not a competing branch. Kept locally, unpushed, for reference pending user/maintainer coordination.
…tion gap - Remove common/constant_time_init.go: PR #17 (mswilkison, stacked on #11) already flips constantTimeEnabled's default to 1 directly in common/constant_time.go, making a separate init() redundant. It was also actively wrong: dlnproof/constant_time_equiv_test.go (and this commit's own new schnorr test) generate their 'non-CT baseline' proof before calling EnableConstantTimeOps(), assuming an ambient disabled default -- an unconditional init() would silently make that baseline vacuous instead of failing loudly. - Update the COVERAGE doc comment in common/constant_time.go to list the Schnorr proof responses and ECDSA signing rounds 3-5 now covered, and to record crypto/mta.AliceEnd/AliceEndWC's Paillier-decrypt timing protection (upstream BNB 3709c25) as a known, deliberately-NOT-ported gap: upstream uses a sleep-based response-time normalizer (NewTimingProtection, ~200ms target + jitter), a different mechanism entirely from the bigmod constant-time path used everywhere else in this file. That primitive exists nowhere in this fork's lineage (checked master, both CT branches, and PRs #12-17). Porting it would inject a fixed ~200ms delay into every MtA share round -- a real latency cost that needs its own sign-off, not a mechanical extension of the existing pattern. - Fix CHANGELOG.md's now-stale 'has not yet published its own tagged release' line (contradicted by the new [1.4.0] section added earlier).
…_init.go Two spots still described CT-enablement as happening via a package init() in common/constant_time_init.go. That file was removed before this branch was ever pushed (superseded by PR #17's direct default-value change, constantTimeEnabled = 1 in common/constant_time.go) -- these two CHANGELOG lines were never updated to match and got carried forward by the cherry-pick onto this branch. Correct the mechanism description in both places.
mswilkison
left a comment
mswilkison
left a comment
There was a problem hiding this comment.
Two P2 findings from the review.
Validation reported by the review: build and vet passed, and the Schnorr tests compiled; test binaries were not executed.
| common.EnableConstantTimeOps() | ||
| defer common.DisableConstantTimeOps() |
There was a problem hiding this comment.
[P2] Explicitly set and restore CT mode in both equivalence tests
CT is enabled by default, so proofOff actually exercises the CT branch when either test runs alone. The deferred disable then leaves subsequent Schnorr tests—including the existing P-256 and session-binding cases—running with CT disabled. This makes coverage depend on test order and removes default-path coverage. Save the previous mode, explicitly disable CT before constructing the baseline, and restore the saved mode during cleanup in both tests.
|
|
||
| --- | ||
|
|
||
| ## [1.4.0] - 2026-09-14 — BNB hardening integration |
There was a problem hiding this comment.
[P2] Keep the untagged hardening stack under Unreleased
This repository currently has no tags or published GitHub releases, including v1.4.0. Moving the hardening stack into this dated release section therefore advertises security changes as released when consumers cannot obtain that version, and both new comparison links reference a nonexistent tag. Keep these entries under [Unreleased] until an actual release contains them.
Address review findings on PR #23: - The equivalence tests built their non-CT baseline without disabling constant-time mode. Since this branch defaults constantTimeEnabled to 1, the baseline actually exercised the CT path, and the bare deferred DisableConstantTimeOps leaked disabled state into subsequent Schnorr tests, making coverage order-dependent and dropping default-path coverage. Save, set, and restore the ambient mode via a withCTMode helper, and assert the baseline really is running with CT off. - Restore the changelog hardening entries to [Unreleased]. This fork has no tags or published releases, so a dated [1.4.0] heading advertised the security work as obtainable, and both comparison links pointed at a tag that does not exist.
|
Both P2 findings addressed in 17e4771. Test CT mode. Confirmed the finding is exactly right: this branch sets Changelog. Also correct, and the |
Addresses review findings on PR #23's constant-time hardening entry: - CHANGELOG.md: tag Breaking Change #8 and the Added CT-symbols entry with PR #17/#23, extend the Composing PRs list (F2) - CHANGELOG.md: soften 'closing the timing side-channel' framing to scope it to the operations covered, cross-reference the mta AliceEnd/AliceEndWC gap instead of implying full closure (F6) - CHANGELOG.md: note the coverage broadening from secret-exponent-only to secret-operand operations (F7) - CHANGELOG.md: restore a 'Not ported / deferred' bullet for the mta gap so the dangling '(see below)' cross-reference resolves again (F8) - CHANGELOG.md: fix the benchmark command to actually run both BenchmarkExpCT and BenchmarkExpStandard, correct the mismatched sample-count claim (F10) - common/constant_time.go: note in the reduceToPaddedBytes NOTE that round_5's rx (a field-prime coordinate) is the one operand reduced into group-order space, and why that's still safe (F3) - common/constant_time_test.go: add BenchmarkMulCT/BenchmarkModInverseCT on a 256-bit-class modulus so the CHANGELOG's performance claim can cite the operations this PR's stack actually added, not just the 2048-bit ExpCT benchmark (F5)
Addresses review findings on PR #23's constant-time hardening in ecdsa/signing: - round_5.go: correct the SECURITY comment's operand list -- m is the public message hash, not secret; name rx = R.X() instead, the operand that actually needed flagging (F9) - constant_time_equiv_test.go (new): add targeted equivalence tests for round_3's thelta/sigma, round_4's thetaInverse, and round_5's si terms -- the five new MulCT/ModInverseCT call sites this PR's stack added had only e2e coverage before this (F13) - constant_time_e2e_test.go: expand TestE2EConcurrentConstantTime's doc comment to name the Schnorr and round 3/4/5 CT paths it now also exercises, not just the pre-existing Paillier/MtA path (F14)
Addresses review findings on PR #23's constant-time hardening in crypto/schnorr: - constant_time_equiv_test.go: pin rand.Reader to a deterministic source (reset immediately before each proof generation, matching the crypto/paillier and crypto/mta sibling convention) and assert Alpha/T/U are bit-identical between the CT and non-CT paths, not just that both proofs verify -- catches a CT branch that silently diverges from, or falls back to, the non-CT computation (F12) - constant_time_equiv_test.go: fix two doc-comment nits -- 'enabled by default in this package' should read 'in this library' (the default is process-wide, set in common/constant_time.go); the primitive equivalence note omitted ModInverseCT, which round-4's change relies on (F11)
2 participants
Context
This is an additive finding for the existing CT-hardening stack (PRs #8 / #10 / #11 / #17) — submitted for review/consideration, not asking for immediate merge. It stacks on #17 (the head branch), in the same stacked-draft style #10 and #16 already use.
Upstream
bnb-chain/tss-libcommit3709c25(folded into theBNB #328series) hardened the constant-time path in two more spots that this fork's stack never picked up:crypto/schnorr/schnorr_proof.go—NewZKProofWithSessionandNewZKVProofWithSessionmultiply the challengecagainst the secret witnessesx,s, andlto build the proof responsestandu. Without CT, thosemath/bigmultiplications leak witness bits through timing.ecdsa/signing/round_3.go/round_4.go/round_5.go—thelta = k·γ,sigma = k·w,thetaInverse = θ⁻¹ mod q, andsi = m·k + rx·σare all secret-key multiplications and inverses on the ECDSA signing hot path.PR #17's own file list (the
d08dc73"extend constant-time coverage" commit) covers Paillier, DLN, MtA, factor/mod proofs, ring-Pedersen keygen, and the existingMulCT/ExpCT/ModInverseCTprimitives — but does not touch either of the two files above. Same gap exists in #8, #10, and #11. This PR closes that gap using the exact same pattern already established incommon/constant_time.go(common.NewCTModInt(...).MulCT/.ModInverseCT, gated onIsConstantTimeEnabled()).Why no
ExpCTWithBitLenadaptation needed (vs. PR #10)The new code calls only
.MulCT(...)and.ModInverseCT(...)— never.ExpCT(...):MulCTtakes no exponent at all (it's a constant-time modular multiplication, not an exponentiation), so PR fix(crypto): handle CT exponent bounds, zero values, and toggle changes #10's bounded-exponent-width hardening does not apply.ModInverseCT's internal exponent is the fixed, modulus-derived constantmod-2(orphiN-1viaNewCTModIntWithPhi) — also a public, fixed-width value, not a caller-supplied variable-width secret. Confirmed bygrep -n '\.ExpCT(' crypto/schnorr/schnorr_proof.go ecdsa/signing/round_3.go ecdsa/signing/round_4.go ecdsa/signing/round_5.goreturning zero hits on this branch.So this addition slots in directly under the existing
IsConstantTimeEnabled()toggle without re-touching the exponent-bound logic PR #10 added.Changes
crypto/schnorr/schnorr_proof.go— wrap thec·xandc·s/c·lmultiplications inNewZKProofWithSession/NewZKVProofWithSessionwith the CT branch.ecdsa/signing/round_3.go— wrapk·γandk·w(thelta/sigma computation) with the CT branch.ecdsa/signing/round_4.go— wrapθ⁻¹ mod qwithModInverseCT.ecdsa/signing/round_5.go— wrapm·kandrx·σ(thesiaddend pair) with the CT branch.crypto/schnorr/constant_time_equiv_test.go— new equivalence test covering bothNewZKProofandNewZKVProof: builds a non-CT baseline proof and a CT proof from the same witnesses and asserts both verify.common/constant_time.go— COVERAGE doc comment updated to list the Schnorr proof responses and ECDSA signing rounds 3-5 among the sites now covered. Default value (constantTimeEnabled = 1, set by PR Enable bounded bigmod operations by default #17) and PR Enable bounded bigmod operations by default #17's "limited coverage" warning are preserved verbatim.CHANGELOG.md— added a Breaking Change feat(crypto): opt-in constant-time path for secret-exponent modexps [DO NOT MERGE] #8 entry under the existing[Unreleased]heading describing the now-on-by-default CT framework plus this PR's Schnorr/signing-rounds extension, anAddedentry for the CT API symbols, and removed two now-stale deferred-CT lines (the "optional constant-time framework ... deferred to a separate follow-up" bullet and "the optional constant-time work is not integrated" line). (An earlier revision of this branch introduced a dated[1.4.0]section header and removed the "has not yet published its own tagged release" line; both were reverted in commit 17e4771 after review feedback that this fork has no tags/releases yet — this description previously went stale relative to that revert.)Verification on this branch (pushed)
The new
TestSchnorrProofCTVerifies/TestSchnorrVProofCTVerifiescases pass.Benchmark (from the local pre-push run on this stack)
Constant-time modexp measured at parity with the standard
math/bigpath on the same hardware:The CPU-cost concern that originally motivated upstream's deferral did not materialize for this primitive.
Out of scope / NOT fixed here — for maintainer awareness only
Upstream's same
3709c25commit also hardenedcrypto/mta/share_protocol.go'sAliceEnd/AliceEndWCPaillier-decrypt paths — but with a different mechanism entirely: a sleep-based response-time normalizer (NewTimingProtection, ~200 ms target + jitter), not the bigmod constant-time path used everywhere else incommon/constant_time.go. That primitive does not exist anywhere in this fork's lineage (master, both CT branches, or PRs #12–#17), and porting it would inject a fixed ~200 ms delay into every MtA share round — a real latency cost that needs its own sign-off rather than a mechanical extension of the existing CT pattern.That gap is documented as a known, deliberately-deferred item in this PR's COVERAGE comment update to
common/constant_time.go, not implemented in code. If/when a follow-up decides to portNewTimingProtection, it should land as its own PR with its own benchmark and a separate latency sign-off; the bigmod CT path doesn't subsume it.