Skip to content

Enable bounded bigmod operations by default - #17

Open
mswilkison wants to merge 1 commit into
constant-time-hardening-backportfrom
codex/ct-defaults-and-bounds
Open

mswilkison wants to merge 1 commit into
constant-time-hardening-backportfrom
codex/ct-defaults-and-bounds

Conversation

@mswilkison

@mswilkison mswilkison commented Sep 14, 2026

Copy link
Copy Markdown

Depends on #11 and is stacked on constant-time-hardening-backport. This carries the public corrections from #10 onto the current backport and enables the covered bigmod operations by default, following upstream's public default-on change.

Selectively adapts public commits 3169e56d59bf022817dad838ca81e62b02dd08e9 and cf15377f96bf8f3d522c6de7a626f4a13e6e3649, and upstream 44a95b248096396b4ef8a611bed789948be46267.

Encode exponents at a fixed public width, including zero, reject overflow instead of silently widening the operation, and use explicit public bounds where proof exponents exceed the arithmetic modulus width. Snapshot the mode across multi-step operations. Decryption uses its existing Carmichael exponent for inversion, preserving support for keys without the optional PhiN field.

Coverage remains limited to the documented exponentiation paths: surrounding conversions, reductions, and other math/big operations remain variable-time. The explicit opt-out API remains available. Kept as a draft for application performance and rollout review.

Validation: focused default-mode, zero, width, inverse, mode-toggle, unequal-modulus, and nil-PhiN compatibility tests passed in common, crypto/paillier, and crypto/mta. Repository-wide go vet, formatting, diff checks, independent patch review, and correction verification passed. The repository-wide test run is in progress. Existing 2048-bit exponentiation benchmarks ran successfully; these are primitive measurements on one machine and do not establish application throughput or side-channel resistance.

Full test CI and formatting CI were dispatched for this branch because the automatic PR trigger only covers master.

Port the zero-exponent path, public exponent bounds, and toggle snapshot
corrections from public PR #10 commits
3169e56 and
cf15377. Adapt the default-on behavior
from upstream PR bnb-chain#332 commit 44a95b2
on top of public PR #11 at 68686af.

Keep the existing exported method and toggle signatures, add an explicit
public exponent bound, and snapshot the mode across each multi-step
operation. Use LambdaN for decryption's inverse so keys without PhiN
remain supported. Restore prior toggle state in tests and select disabled
baselines explicitly. Document the limited coverage of the bigmod path.
@piotr-roslaniec

Copy link
Copy Markdown

Heads up — a separate review pass turned up one more CT gap that none of #8 / #10 / #11 / this PR cover: upstream bnb-chain/tss-lib's 3709c25 also hardens crypto/schnorr/schnorr_proof.go (witness multiplications in NewZKProofWithSession / NewZKVProofWithSession) and ecdsa/signing/round_3.go / round_4.go / round_5.go (k·γ, k·w, θ⁻¹, m·k, rx·σ). I cherry-picked the port onto a new stacked draft — #23 (ct-hardening-schnorr-signing-coverage, base codex/ct-defaults-and-bounds) — using the exact same IsConstantTimeEnabled() / NewCTModInt(...).MulCT / .ModInverseCT pattern. gofmt -l ., go build ./..., go vet ./..., and go test ./... all clean there; new Schnorr equivalence test (crypto/schnorr/constant_time_equiv_test.go) passes.

Separately, the same upstream commit also touches crypto/mta/share_protocol.go's AliceEnd / AliceEndWC — but via a different mechanism (sleep-based NewTimingProtection, ~200 ms + jitter) that doesn't exist in this fork and isn't a mechanical extension of the bigmod CT pattern. I deliberately did not port that, but I did add a COVERAGE comment in common/constant_time.go recording it as a known, intentionally-deferred gap so it doesn't get lost — see #23 for that doc update.

piotr-roslaniec added a commit that referenced this pull request Sep 18, 2026
…_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
mswilkison marked this pull request as ready for review September 18, 2026 14:37
piotr-roslaniec added a commit that referenced this pull request Sep 21, 2026
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)
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.

2 participants