Repository navigation
Conversation
|
Test failure currently due to ML-DSA 87 issue #137 |
|
The 3 missed are XOR/OR-equivalence class mutations this repo accepts (e.g.
|
|
Building a constant-time math is the project assigned to @laruizlo . David, I would like you to coordinate collaboratively with Luis on this work, please. |
This is a PR about ECDSA. Why on earth does that have anything to do with a currently-open ML-DSA bug? Like why is that even worth commenting on? |
|
The current test code fails silently if the ACVP or Whycheproof vectors are not present, this PR fixes the issue of getting the test data present but as it's still got the broken version of ML-DSA that test fails. The tests associated with the actual PR still pass though. |
|
@laruizlo I would welcome any feedback, please feel free to feedback/update the code in this PR as you see fit. Note: this PR will not be covering MQV, IES, or ECDHC, it would certainly help if you could come up with something for that (especially the last one, IES is actually "popular" as well... MQV I'm not so sure, ACVP does test for it, but I haven't actually run into anyone using it (I suspect due to patent issues from the "old days")). One word of note, it turned out to be relatively simple to build an RSA crate on to of this, I would recommend holding off on making changes till you have seen the RSA crate as well. I will get that up here shortly. One word of warning, mutant testing on the full EC crate takes days. ecdsa/sm2 not so bad, if you make any changes do the mutant testing incrementally or you will be needing to find something else to do for quite a while. |
58051d2 to
bec4253
Compare
1c53336 to
58051d2
Compare
009540b to
52481c6
Compare
3c170c9 to
81cef74
Compare
`keygen()` instantiated the library's default DRBG inside the call and `keygen_from_rng(rng)` was the form that took one; the no-argument form goes and the RNG-taking form is `keygen(rng: &mut dyn RNG)`, in every curve's `keys_*` module and in SM2's, the rule the rest of the library follows for anything that draws randomness. The strength check on the RNG (FIPS 186-5 Appendix A.2.1) is unchanged. The CLI's `keygen` actions draw from `HashDRBG_SHA512::new_from_os()`, as the signers do; the `ecdsa_cmd` generic takes `fn(&mut dyn RNG) -> ..`. Tests, benches, the stack harnesses and the crate docs pass an RNG; the shared suites' `keygen: fn() -> ..` parameter is satisfied with a closure. Assisted-by: Claude:claude-mythos-5-1
…ed over the rebase onto its release-based history This branch took feature/key-rework-memory by merge, and the merge commits carried the adaptations to what arrived. The rebase onto the branch's new history (key-rework-memory rebased onto release/0.1.3alpha, which carries feature/key-rework as one squash commit) dropped those merges; this commit restores the affected files to the branch's pre-rebase tip, so the tree is identical to it. Assisted-by: Claude:claude-mythos-5-1
…ed over the rebase onto its release-based history This branch took feature/key-rework-memory by merge, and the merge commits carried the adaptations to what arrived. The rebase onto the branch's new history (key-rework-memory rebased onto release/0.1.3alpha, which carries feature/key-rework as one squash commit) dropped those merges; this commit restores the affected files to the branch's pre-rebase tip, so the tree is identical to it. Assisted-by: Claude:claude-mythos-5-1
…or every curve [For humans] - P-256's generate_k said a rejected candidate is "discarded entirely"; RFC 6979 chains, so it is not - P-256 now carries the PRF-stream argument once; the three brainpool copies shrink to their rates - secp256k1, P-384 and P-521 state their rejection probability (2^-128, 2^-194, 2^-262) - P-256 says the loop retries only out-of-range candidates, not an r = 0, where signing errors - bp512r1's rate is 33 %, not 34 %; the fingerprint is one to two bits per message, not "about a bit" - docs only ====================================================================== [For agents] b11afd6 fixed the brainpool rfc6979 modules' rejection-loop text and deferred crate::rfc6979's as a cross-curve item: its structural sentence ("a rejected candidate is discarded entirely and nothing derived from it is ever used, because the only thing its iteration count can reveal is how many candidates were rejected") misdescribes RFC 6979 on every curve. After a candidate V is rejected the code computes K' = HMAC_K(V || 0x00) and V' = HMAC_K'(V), and the next candidate is HMAC_K'(V'): the accepted k is derived from the rejected V through two HMAC applications under a key that is itself derived from V. (b11afd6's shorthand "the accepted k is an HMAC of the rejected V" is inexact in the same direction and is replaced by that formula.) The argument that does hold is the one b11afd6 wrote three times, once per brainpool module: the iteration count is a function of the HMAC-DRBG output stream, i.e. of PRF outputs under a key derived from (d, h1), so under the PRF assumption it is independent of the accepted k (uniform on [1, n-1] conditioned on the count) and reveals nothing about d; what must be constant time is everything done with the accepted k. That argument is curve-independent, so per QUALITY_AND_STYLE.md "Proportion" (each fact has one home) it now lives once, in crate::rfc6979::generate_k's docs under a "The rejection loop" heading, which also states P-256's rate (n within 2^-32 of 2^256). The three brainpool generate_k docs keep what is specific to them: the rate (0.336, 0.4503, 0.3326, i.e. 34 %, 45 % and 33 % of signatures; the last was rounded to 34 % before), the mean extra iterations, the three HMAC calls per extra iteration, and the practical consequence of a deterministic per-(d, msg) count: a fingerprint worth one to two bits per message (the entropy of the geometric rejection count is 1.39, 1.81 and 1.38 bits; "about a bit" undersold it), usable for confirming a candidate d, not for recovering one. They link to P-256 for the argument. Their module docs said the loop is near-dead "for P-256 and secp256k1" in one file and "on the NIST curves" in the other two; all three now say "on the NIST curves and secp256k1", the phrase the crate docs use. rfc6979_p256k1 and rfc6979_p521 already deferred to P-256; they, and rfc6979_p384, which said nothing about the loop, now state their own rejection probability, computed from N_LIMBS as (2^qlen - n) / 2^qlen: 2^-127.65 (secp256k1, so "n within 2^-127 of 2^256", rejection about 2^-128), 2^-194.18 and 2^-262.49 (P-521 after the 521-bit truncation). The crate docs' Security Considerations link for the argument moves from rfc6979_bp256r1::generate_k to rfc6979::generate_k, with the brainpool modules named for the rates, and its range becomes 33 % to 45 %. The P-256 docs also say what the loop does not do: it rejects only out-of-range candidates, and a k whose r would be 0 is not retried, signing returns an error instead (FIPS 186-5 step 11, documented in ecdsa_p256 and in the crate's Security Considerations). This is stated in terms of the code's behaviour only. b11afd6's message notes that RFC 6979's step h.3 treats such a k as rejected; the RFC text could not be fetched from this sandbox (rfc-editor.org and ietf.org are not reachable through its proxy), so the quote and a sentence on the deviation are left for a session with access to the document. For the same reason the existing quotes ("otherwise ... loop", "not reduced modulo q", "utterly improbable") are carried over unchanged, not re-verified. cargo doc -p bouncycastle-ecdsa: no new warnings (the rfc6979_p521 private-item links predate this change). cargo test -p bouncycastle-ecdsa --doc and --lib pass. Docs only; no code change. Assisted-by: Claude:claude-fable-5-1
…1-2016 S6.1
[For humans]
- GB/T 32918.1-2016 S6.1 a) generates dA in [1, n-2]; the crate used [1, n-1], which is k's range
- dA = n - 1 loaded, and keygen could draw it (2^-256), but it cannot sign: (1 + dA)^-1 is of 0
- keygen now reduces the DRBG output mod n - 2, and from_bytes rejects n - 1 and above
- extra_bits gains reduce_wide_bits_mod_n_minus_2 beside the n - 1 reduction that k keeps using
- tests: seven n - 2 known answers from Python, the bit-serial reference on both moduli, n - 2 signs
- fallibility counts unchanged; bc-rust sm2 sign and pk-from-sk refuse d = n - 1 and accept n - 2
======================================================================
[For agents]
GB/T 32918.1-2016 S6.1 (密钥对的生成, key pair generation) reads, in full: input, a valid set of
elliptic curve system parameters; output, a key pair (d, P) for them; a) 用随机数发生器产生整数
d∈[1,n-2] ("generate an integer d in [1, n-2] with a random number generator"); b) P = [d]G;
c) the key pair is (d, P), d the private key and P the public key. GB/T 32918.2-2016 S5.3
(用户密钥对) says the signer's key pair "shall conform to GB/T 32918.1-2016 chapter 6", and its
S6.1 step A3 draws the per-message secret from [1, n-1] (用随机数发生器产生随机数k∈[1,n-1]), so
the two ranges differ by one on purpose: step A6 divides by 1 + dA, and dA = n - 1 would make that
a division by 0. draft-shen-sm2-ecdsa-02, which the crate cited for the key range, states none;
its key generation section is a placeholder. The two GB/T parts were read for this change from
their PDFs; the draft is not reachable from this sandbox and its lack of a range is as the review
found it. The OpenSSL data point was reproduced here: OpenSSL 3.0.22's sm2_sig_gen computes (1 + dA)^-1 once and loops on s = 0, so
pkeyutl -sign with d = n - 1 never returns (the review had the same result on 3.0.13), while
d = n - 2 signs and the signature verifies.
The crate's from_bytes compared the raw encoding against 0 and n, so d = n - 1 loaded and
round-tripped (the old test asserted exactly that), and keygen reduced the 320-bit DRBG output
mod n - 1 and added 1, which reaches n - 1 with probability 2^-256. Such a key's cached
(1 + dA)^-1 is Fermat's inverse of 0, which is 0, so every signature came out s = 0 and sign_with_k
returned its "regenerate k and retry" error forever.
extra_bits now has a private reduce_wide_bits(bytes, m, mu_low) and two one-line public wrappers:
reduce_wide_bits_mod_n_minus_1 for k, unchanged in behaviour, and reduce_wide_bits_mod_n_minus_2
for dA. The new constants, computed in Python from n and checked against the n - 1 values already
in the file:
N_MINUS_2_LIMBS = [0x53bbf40939d54121, 0x7203df6b21c6052b, 0xffffffffffffffff, 0xfffffffeffffffff]
MU_LOW_LIMBS (n-2) = [0x12ac6361f15149a2, 0x8dfc2096fa323c01, 0x0000000100000001, 0x0000000100000001]
mu = floor(2^512 / (n-2)) is 2^256 + mu_low with only the low limb differing from the n - 1 value
(...49a1). Barrett's preconditions hold for the new modulus as for the old: (n-2) >> 255 = 1 and
2^504 < (n-2) * 2^256, so the 63-byte input limit is the same. FIPS 186-5 A.4.1's bias bound
(2rho(1-rho)m <= 2^-64 * 2^l, rho = (2^l mod m)/m) re-evaluated with m = n - 2: l = 256 fails,
l = 319 is the smallest passing length, l = 320 passes with the ratio at 2^-31, the same three
numbers as for n - 1, so EXTRA_BITS_DRBG_OUTPUT_LEN = 40 serves both. N_MINUS_1_LIMBS becomes
pub(crate) so from_bytes can compare against it: borrow out of limbs - (n - 1) is 1 iff limbs < n - 1
iff limbs <= n - 2. The check is still a load-time branch on public constants, the argument in the
comment above it is unchanged.
Known answers for the n - 2 reduction, each int.from_bytes(x, 'big') % (n - 2) + 1 in Python:
01020304 repeated to 40 bytes -> 0306090c...0e773203ec4d33fd; 40 bytes of 0xff ->
00000001000000008dfc20956c361b6a187a276050a8c5aa726ecad5c62abedf; 40 zero bytes -> 1; n - 2
(left-padded) -> 1, n - 3 -> n - 2, n - 1 -> 2; 63 bytes of 0xff -> a21eb5e3...3783fc97. The
existing n - 1 answers reproduce from the same one-liner with m = n - 1. reference_bit_serial takes
the modulus as a parameter and the pseudorandom agreement test and the 63-byte test run both
functions. sm2_tests' range test is renamed private_key_rejects_values_outside_1_to_n_minus_2:
n - 1, n, n + 1 and 2^256 - 1 are rejected, n - 2 loads, encodes back unchanged, signs and verifies
(1 + dA = n - 1, the inverse of -1, a corner the old range never reached).
Left alone: k's range and reduction (GB/T 32918.2-2016 S6.1 A3 is [1, n-1], as before), Sm2Scalar,
sign_with_k, from_validated_scalar's expect message ([dA]G is never infinity for dA in [1, n-1]
still holds a fortiori), the memory tables, the CLI (it loads through from_bytes, which is where
the new behaviour was checked: bc-rust sm2 pk-from-sk and sign refuse d = n - 1 and accept
d = n - 2, whose signature verify accepts). quality_stats.sh before and after: the fallibility
counts are identical. cargo test -p bouncycastle-sm2: 5 binaries, all pass.
Assisted-by: Claude:claude-fable-5-1
[For humans] - sm2_scalar's redc ends with a select of high - n when high >= n; no test reached that select - cargo mutants' "select forced to high" survived both crates' suites: a non-canonical u = c + n - downstream, negate, == and is_zero assume canonical form, so s = n would pass s == ZERO - three constructed products now land there, in both operand orders, and are pinned from Python - the test asserts canonical output (add zero is a no-op, negate gives n - expected), not just value - tests only; no code change ====================================================================== [For agents] redc (sm2_scalar.rs) reduces the 8-limb product with four SOS rounds, then, when the 9th limb is 0, selects high - n over high if high >= n. The operands that reach that select need the Montgomery-form product abR mod n below 2^256 - n, about 2^224, and the pre-subtraction value u = (T + m n) / R to be the representative above n, which happens for about 2^-32 of products; 3000 pseudorandom pairs do not find one, and the (n-1)^2 and round-trip tests do not either. The mutant that forces the select to high survived cargo mutants on both bouncycastle-ec and bouncycastle-sm2. It is load-bearing: Sm2ScalarField's negate (n - x, selecting 0 when is_zero), PartialEq (limb compare) and is_zero all read the limbs as canonical, so a product that came out as n would compare equal to ZERO and a signing s equal to n would be emitted as 0. The three cases were constructed in the review rather than searched: pick the Montgomery operand a and the target pre-subtraction value u in [n, 2^256), solve m = uR n^-1 mod a and b = (uR - mn)/a, which makes REDC(a b) land exactly on u. Each case was re-checked here in Python before being committed: a b mod n equals the expected limbs, the SOS pre-subtraction value is in [n, 2^256) in both operand orders, u - n equals abR mod n, and abR mod n is below 2^224. from_limbs(expected) was also checked to pre-reduce to a value at or above n, which is why mul(a, b) == from_limbs(expected) alone would pass under the mutant (both sides would hold the same non-canonical limbs) and why the test asserts prod == prod.add(ZERO) and prod.negate() == n - expected as well as prod.to_limbs() == expected. The mutant was applied by hand to confirm: with the select forced to high, the test fails at the add-zero assertion on the first case; on the unmutated code the sm2_scalar_tests binary passes, 18 tests. The 5p -> 4p mutant in sm2.rs's base-field reduce is a different matter: the four D words sit at bits 64-95, so V > -4 * 2^96 and any bias of 2^98 or more keeps the accumulator non-negative; that mutant is equivalent and its documentation is corrected in the following docs commit, not here. redc's doc comment is left as it is; the test's comment carries the argument. Assisted-by: Claude:claude-fable-5-1
…SM2 rather than ECDSA [For humans] - every section and step cited in the sm2 crate now names GB/T 32918.1/.2-2016, the draft in parentheses - the verification steps use GB/T S7.1's numbering (B1-B2 range, B3-B4 hash, B5 t, B6 point, B7 R) - the ec crate's sm2_* modules describe SM2's own equations, not ECDSA's k^-1, u, v or FIPS 186-5 - sm2 base-field reduce: the D words sit at bits 64-95, so V > -4 * 2^96; 5p is for the limb bound - docs and comments only; no code change ====================================================================== [For agents] GB/T 32918.2-2016 was read from its PDF for this change: S5.3 defers the key pair to part 1 chapter 6; S5.4.2 admits any approved hash "such as SM3"; S5.5 defines ZA and IDA; S6.1 is signing A1-A7 (A1 prepends ZA, A2 hashes, A3 draws k in [1, n-1], A4 [k]G, A5 r, A6 s, A7 output); S7.1 is verification B1-B7 (B1 r' range, B2 s' range, B3 prepends ZA, B4 hashes, B5 t = r' + s' with t = 0 failing, B6 [s']G + [t]PA, B7 R = e' + x1' compared with r'). The crate's old B labels followed the draft (B1 for both range checks, B4 for t, B5 for the point, B6 for R); they are renumbered to the standard's, which is what the code now cites first. The draft's section numbers given in parentheses (S5.1.4.2 hash, S5.1.4.4 ZA, S5.2.1 signing, S5.3 verification) are taken from the review's mapping: the draft is not reachable from this sandbox, so they were not re-verified here and should be spot-checked against draft-shen-sm2-ecdsa-02 before merge. The two ctx-required error strings now cite S5.5, where IDA is defined, rather than the signing and verification sections. In the ec crate, sm2_scalar.rs's module and item docs described ECDSA's k^-1, u and v and FIPS 186-5 S6.4.2; they now describe the SM2 quantities the sm2 crate computes ((1 + dA)^-1 once per key, r = e + x1, s, t = r' + s') and point at bouncycastle_sm2's sign_with_k. invert_vartime has no SM2 caller; its docs say so and that it is kept for parity with the ECDSA curves' public scalar types (removing it is an API decision left to the crate's owner). sm2_wnaf.rs names [s']G + [t]PA and S7.1 B6. sm2_sec1.rs says SM2 and GB/T 32918.5-2017 where it said P-256 and SP 800-186 S3.2.1.3. In sm2.rs, mul_small's "operand-independent count" is corrected: acc[4] is a function of the operands, in [4, 18], and only a multiplicand; reduce's precondition paragraph now says mul and square are the two call sites and that the bounds hold for any 8-limb input; and the bias argument is corrected: the four D words occupy column 2 (bits 64-95), so V > -4 * 2^96, any bias of 2^98 or more keeps the accumulator non-negative, and 5p is chosen for the top-limb bound of 18. Checked in Python against the column table: the all-ones 512-bit input reaches top limb 18, and no product sampled (random, near-p, and (p-1)^2 at 14) exceeded 16, so the field tests' "4 through 17" is the product range. This is why cargo mutants' 5p -> 4p mutant survives: it is equivalent. Docs and comments only; no code change. cargo fmt and cargo doc clean on the touched crates (the two pre-existing private-item link warnings in bouncycastle-sm2 are unchanged). Assisted-by: Claude:claude-fable-5-1
…tract [For humans] - the crate docs said every operation on dA and k is branch-free; sign_with_k has two branches - step A5 (r = 0 or r + k = n) and step A6 (s = 0) each branch, then discard the k they branched on - the A5 comment called the check a branch on the public output r; r + k = n is a function of k - the Security Considerations bullet now names both checks as the exceptions, with the probability - the heading becomes the `# 🚨 Security Considerations 🚨` form QUALITY_AND_STYLE.md requires - docs and comments only; no code change ====================================================================== [For agents] The same move as the ecdsa crate's commit that named the rfc6979 loop in its timing contract. sign_with_k's comment above the step A5 check said the comparison was "the draft's own mandated success/failure branch on the just-computed public output r". Only r = 0 is a condition on the public output; (r + k) mod n = 0, which for k in [1, n-1] and r in [0, n-1] means r + k = n, is a function of k. The comment now says what is true: both disjuncts are compared in constant time (Sm2ScalarField's PartialEq compares every limb through Condition masks), the if is one of two data-dependent branches on the signing path, the other being step A6's s = 0, each disjunct is taken with probability 1/(n-1) per signature, and a taken branch discards that k, so neither reveals anything about a k that is used. The crate docs' first Security Considerations bullet claimed "every operation performed on it or on k ... is branch-free". It now says everything done with dA and the used k is branch-free and indexes no memory by their value, and names GB/T 32918.2-2016 S6.1's two retry checks as the exceptions, with their probability (about 2^-255 each, since each is one value out of n) and why they are harmless. The heading is changed to the exact form QUALITY_AND_STYLE.md's Docs section requires; thirteen crates in the workspace already use it, and the ec and ecdsa crates, which this branch does not touch for that purpose, still use the plain heading. Docs and comments only; no code change. cargo test -p bouncycastle-sm2 passes; cargo doc shows the same two pre-existing private-item link warnings and nothing new. Assisted-by: Claude:claude-fable-5-1
… to_bool_opaque goes [For humans] - to_bool was `self.0 != 0`: on a mask built from secret values LLVM may branch on each value - to_bool now tests the mask after the volatile barrier, on all four widths; to_bool_opaque goes - the seven rfc6979 candidate_in_range sites go back to to_bool; its 99 other callers change nothing - to_bool is no longer const; nothing in the workspace called it in const context - release asm: every generate_k keeps one conditional jump (two on P-521), as with to_bool_opaque - no ecdsa function changes its conditional-jump count; sign_with_k gains 11 instructions - rejection-loop shape, same outcome, different failing check: 9 ticks apart before, 0.00 after ====================================================================== [For agents] Condition's consumers that do masked arithmetic (select, mov, swap, conditional_select) pass the mask through barrier(), a volatile store and load, before using it. to_bool did not: it was a const fn returning `self.0 != 0`, and when the mask was built from arithmetic the compiler can see (an OR-chain over limbs, a subtraction's borrow) LLVM may fold the mask away and emit one conditional branch per input comparison. The outcome of the decision is the same, but the timing then reveals which condition decided it. PR 182's earlier commit added to_bool_opaque, `self.barrier().0 != 0`, as an opt-in second method and left to_bool alone pending a census of its callers. The review that followed (its findings are summarised here, since its documents are not in the repo) read LLVM 22's SelectionDAGBuilder::visitBr: it splits a branch on an and/or of conditions whenever jumps are cheap, which on x86 means whenever the second condition costs more than two cycles of latency, and on AArch64, ARM, RISC-V, WebAssembly, AVR and MSP430 whenever it can. So every to_bool caller whose mask came from secret values was exposed, and an opt-in method only protects the callers that know to opt in. This commit makes the barriered form the only one. The census: 99 to_bool() calls across ec, ecdsa, sm2 and utils before this change. 64 are in the eight *_point.rs files (add_vartime and add_vartime_affine, on public data, plus to_affine's infinity check on secret-dependent points), 16 are the PartialEq limb comparisons on the field and scalar types, which are documented as constant time, 8 are comb-table asserts, 7 are the r = 0 and s = 0 checks in sign_with_k, 2 are ct_eq_bytes and ct_eq_zero_bytes in ct.rs, and one each are in inverse_vartime and the SM2 verifier. None is in const context: `cargo check --workspace --all-targets` passes with the four to_bool no longer const, and the plans directory's prototype text that shows a `const fn to_bool` is a historical document, not code. ct_eq_bytes and ct_eq_zero_bytes now pass a second barrier after acc_is_zero's own; harmless, and left as they are. Per CLAUDE.md the four `impl Condition<W>` blocks are hand-written copies and a change to one width is applied to every width, so to_bool is changed and to_bool_opaque deleted in all four. The rustdoc states the contract only (the test is made on the whole mask, best-effort, and a condition whose value must stay secret should not be converted at all); the mechanism and its pointer to the "Optimisation barrier" section comment stay in a `//` comment, as the review of to_bool_opaque's docs settled. That section comment gains one sentence naming the branch-splitting form of the risk, and barrier()'s doc names to_bool as a consumer. The four to_bool_opaque tests in ct_tests.rs are deleted and no test is added: to_bool is already checked by assert_canonical (21, 21, 39 and 39 calls in the u64, u32, i64 and i32 modules) and the operator tests, and whether the barrier is effective is not visible from a unit test; it is checked in the assembly, as for select and swap. cargo mutants -p bouncycastle-utils --re to_bool (with examine_globs dropped from the config so the filter applies): 12 mutants, 12 caught. Evidence, all on x86_64 release builds with codegen-units=1. Assembly of bouncycastle_ecdsa before (eb0bca4, to_bool_opaque) and after: every generate_k has exactly one conditional jump, the loop decision on the barrier-loaded mask (`jne`), and P-521 has two, the second being its public 66-byte copy-loop counter; both builds identical. A diff of every function in the crate, with labels and hashes normalised, shows no function changing its conditional-jump count; the only bodies that change length are the seven sign_with_k (+10 or +11 instructions, the r = 0 and s = 0 checks now loading the mask through the barrier) and the seven do_verify_final (+12). A scratch rejection loop in the shape of generate_k, built against the previous utils and this one: two conditional jumps (`jne` on the all-zero test, `je` on the borrow) with the old to_bool, one with the new; this is the finding that started PR 182's candidate_in_range work, reproduced on plain to_bool. A scratch OAEP-style check (Y zero, lHash' equal, 0x01 separator present) compiled to one jump with both versions, so the three-jump form the review observed on that shape depends on how the mask is written and was not reproduced here. Timing on the rejection loop, 1,000,000 interleaved samples per class with the order shuffled each round, rdtscp, top decile trimmed, Welch's t: with the old to_bool a zero candidate and an all-ones candidate (both rejected once, then accept the same value) sit 9.14 ticks apart, |t| = 492, while the control pair (two different candidates that both fail the below-n test) is 0.01 ticks apart; with this change the same pair is 0.00 ticks apart, |t| = 0.1, and the control is 0.00. The OAEP shape shows no difference between failure causes in either build, as its single jump predicts. cargo test --workspace: all 251 test binaries pass with bc-test-data and wycheproof present (no "not found" skips); cargo test --workspace --all-targets --no-run compiles the benches and mem_usage_benches; cargo fmt --all --check passes; cargo doc for utils and ecdsa reports the same 15 pre-existing warnings as before, nothing new. quality_stats.sh on crypto/utils and crypto/ecdsa is unchanged apart from one more docstring line in ecdsa. No to_bool_vartime escape hatch is added: nothing needs an unbarriered or const conversion, and the `_vartime` name is available if one ever does. Criterion benches for ecdsa (keygen, sign and verify on all seven curves), before against after on the shared 2-vCPU VM: the 21 differences run from -9 % to +3 % in both directions, and the largest are speedups (P-384 sign -9 %, P-521 verify -8 %) where this change can only add instructions, so the run resolves nothing below the machine's drift between the two passes; no slowdown is attributable to the change. Assisted-by: Claude:claude-fable-5-1
[For humans]
- reduce()'s third fold re-added C and selected it on a carry-out that bound (3) proves is 0
- the second add and the select are gone; the carry is debug_asserted like the fold loop's own
- the bounds proof stays in reduce(); the pinned product's construction lives only in the test file
- no behaviour change: the borrow-selected value the live path already produced is now returned
- ec tests green incl. the third-fold known answer; cargo mutants on p256k1.rs: 97 before and after
- the 2 surviving mutants are documented equivalents on unchanged lines; reduce()'s own are caught
======================================================================
[For agents]
p256k1::reduce folds a 512-bit product twice with 2^256 = p + C, then folds the one bit that can
remain above 2^256 in under a mask. After that masked add of C it also computed folded + C a second
time and selected it on carry1, the carry out of the masked add. Bound (3) in the same comment,
lo_2 + C < 2^66 + 2^33 < p whenever the bit is set and nothing added when it is not, makes that carry
identically zero, and the code's own comment said so ("never selected"). The add and the select were
therefore dead code in the secret path: harmless for timing, but untestable, since no input makes
the selected arm the result. cargo-mutants never saw it either: the two statements contain nothing
it mutates (no operator, no function of this file), so the file lists the same 97 mutants before
and after this change, which is why reasoning rather than a surviving mutant flagged it.
The change: the second add and the second select are removed, the carry is
debug_assert_eq!(carry, 0, ..) in the same style as the fold loop's carry assert and
widening_square's two, and the borrow-selected value is returned directly. Release builds compile
the assert out, so the function gains no branch on secret-derived data; in a debug build the assert
can only fire if bound (3) is wrong. Behaviour is unchanged on every input: when the bit is set the
old code's live path was already the borrow select of folded against p, and when it is not the fold
adds zero. folded < 2^256 < 2p either way, so the single conditional subtraction suffices.
The comment restructure, from the same review: the bounds (1)-(3) stay in reduce(), where (3) now
also justifies the assert; the operand construction for the product that reaches the fold
(a = 2^255, b = 2 * hi_0 with hi_0 * C = 2^256 - 1 mod 2^256, and the Python that computes it) and
the argument that sampling never reaches it (about 2^-190 per product; (p-1)^2 falls short by
C - 1) live once, in p256k1_field_tests.rs next to the pinned product. reduce() points at the test
for the construction and the test at reduce() for the bounds, so each derivation has one home. The
module docs no longer say reduce()'s comment gives the operands.
Verification: cargo test -p bouncycastle-ec, 53 test binaries green including
known_answer_mul_reaching_the_third_fold; cargo +nightly fmt --all --check clean. cargo mutants
scoped to p256k1.rs (the checked-in config minus examine_globs, --test-package bouncycastle-ec,
bc-test-data and wycheproof symlinked under /tmp): 97 mutants, 79 caught, 15 unviable, 2 missed,
1 timeout. reduce()'s two function-level mutants are caught. The two misses are on unchanged lines
and are the equivalents their comments already accept: widening_mul's tail-loop `carry = s >> 64`
mutated to `<<`, and widening_square's doubling `|` mutated to `^`. The timeout is widening_mul's
tail-loop `k += 1` mutated to `k *= 1`, an infinite loop, so a kill rather than a false alarm.
Assisted-by: Claude:claude-fable-5-1
…text one home
[For humans]
- four docs said RFC 6979 §3.4 calls candidate rejection "utterly improbable"; it says so of r = 0
- step h.3 attaches no probability to its loop; the parenthetical is dropped at all four sites
- rfc6979_bp256r1 is the one home for the brainpool rejection text; bp384r1/bp512r1 point at it
- the crate docs' timing bullet now names ECDetDSA*; ECDSA* draws k per FIPS 186-5 A.3.1, no loop
- docs and comments only; cargo doc warning set identical before and after; all ecdsa suites green
======================================================================
[For agents]
RFC 6979 uses "utterly improbable" twice, in §2.4 item 3 and in the §3.4 Usage Notes, both about
r turning out to be 0 and a new k having to be selected. Step h.3 of §3.2 says only that bits2int's
result is compared to q, not reduced modulo q, and that the process loops if the value is not between
1 and q-1; it makes no claim about how often. The three brainpool rfc6979 module docs attributed the
phrase to the rejection loop and then explained it away with an assumption ("n close to 2^qlen")
the RFC never states, and the P-256 module's #[cfg(test)] comment made the same misattribution. The
RFC text was read first-hand on 2026-10-31 (rfc-editor.org is not reachable from the build
container, so the quotes are from that reading, not from recall). rfc6979_bp256r1's module docs now
carry the one correct sentence: no probability is attached to the loop, and the phrase is about
r = 0, which this crate reports as an error rather than retrying. The P-256 test comment states the
curve fact instead: 2^256 - n is about 2^224, so a uniform candidate is >= n with probability about
2^-32. generate_k's doc had "n is within 2^-32 of 2^256", which is loose (the absolute distance is
about 2^224); it now says so directly.
The brainpool rejection-loop text (rate, three HMAC calls per extra iteration, the fingerprint it
amounts to for an observer) had been copied verbatim into all three generate_k docs, with a rate
sentence plus pointer in each module doc and a rate recap in each test comment. Per
QUALITY_AND_STYLE.md "Proportion" each fact gets one home: rfc6979_bp256r1::generate_k keeps the
full text under a heading that names the brainpool curves, and its module doc keeps the
normal-path-versus-near-dead-branch contrast; rfc6979_bp384r1 and rfc6979_bp512r1 state their own
rate and mean in both places and link to bp256r1. The rate-independent PRF argument keeps its
existing home in rfc6979::generate_k (P-256), which the brainpool text already cited. The three
test-module comments shrink to the coverage statement (the reject path is reached from the
rejecting cases in the vectors files; the boundaries 0 and n are what those cannot pin). The
vectors files are untouched: their per-case rejection counts are file-local provenance and already
point at the module docs.
The crate docs listed "RFC 6979's candidate loop" as a timing exception without saying which signer
it belongs to. ECDSA* draws k per FIPS 186-5 Appendix A.3.1 (extra random bits reduced mod n-1, plus
1), which has no rejection loop, so the bullet now names ECDetDSA*; the 33 % to 45 % summary stays
as the crate-level one-liner, with pointers to the argument, the rates and the fingerprint text.
Verification: cargo test -p bouncycastle-ecdsa, 33 test binaries green with the bc-test-data and
wycheproof suites present (CAVP and wycheproof run, not skipped); the brainpool vectors suites with
their rejection pins are untouched and pass. cargo doc --no-deps for ecdsa, ec and sm2 at 735fae3
and after this change emits the same 82 warnings, all pre-existing links to private items, so the
new intra-doc links all resolve. cargo +nightly fmt --all --check clean. Docs and comments only; no
code change.
Assisted-by: Claude:claude-fable-5-1
…what the REDC KAT bounds [For humans] - sm2 keys.rs: the comment says dA is in [1, n-2]; the expect string two lines below said [1, n-1] - ec sm2_scalar_tests.rs: the REDC final-subtraction KAT's doc said "canonical result below 2^224" - the value that must be below 2^256 - n is the product's Montgomery form abR mod n, not ab mod n - the three pinned products' ab mod n are 254 to 256 bits; their abR mod n are 220 to 224 bits - an expect string and a doc comment only; no code change ====================================================================== [For agents] Commit 868e28b moved SM2's private key dA to [1, n-2] per GB/T 32918.1-2016 §6.1 a) and updated the comment above SM2PrivateKey::from_validated_scalar, but not the expect string two lines below it, which still named [1, n-1]. The message is only ever shown if [dA]G were the identity, which the comment explains cannot happen, so this is a string fix with no behaviour change. known_answer_mul_reaching_the_final_subtraction in sm2_scalar_tests.rs pins products whose REDC output before the final conditional subtraction lies in [n, 2^256). Its doc said that needs "the canonical result below 2^224". The test's constants are plain integers a, b and expected = ab mod n, and from_limbs puts a and b into Montgomery form, so mul computes REDC((aR)(bR)) = abR mod n and to_limbs converts back. The pre-subtraction value lies in [n, 2^256) exactly when the canonical Montgomery representative abR mod n is below 2^256 - n (a 225-bit number, about 2^224) and REDC lands on that representative plus n. Recomputed from the test's own constants: the three ab mod n are 255, 256 and 254 bits, so the old sentence named a quantity that is not small at all; the three abR mod n are 224, 220 and 224 bits, all below 2^256 - n, and in all three cases the pre-subtraction value equals abR mod n plus n. The doc now names abR mod n and says what REDC returns versus what the test compares. The rest of the paragraph, the operand-construction recipe and why a plain mul == from_limbs(expected) check would not detect a missing subtraction, is unchanged. Verification: cargo test -p bouncycastle-sm2 and the ec crate's sm2_scalar_tests binary pass; cargo +nightly fmt --all --check clean. An expect string and a doc comment only; no code change. Assisted-by: Claude:claude-fable-5-1
[For humans] - QUALITY_AND_STYLE.md: the heading is always exactly `# 🚨 Security Considerations 🚨` - the ec and ecdsa crate docs had the plain `# Security Considerations`; sm2's was already right - two heading lines; no other change ====================================================================== [For agents] QUALITY_AND_STYLE.md's Docs section fixes the heading so the sections are easy to spot when reading and to find with a search. Every other crate in the workspace that has the section uses that heading; ec and ecdsa were the two exceptions, both at the crate level (neither crate has the section on a module or item). Prose references to the section, "see the crate docs' Security Considerations" in rfc6979.rs and sm2.rs, are not headings and are unchanged. Docs only; no code change. Assisted-by: Claude:claude-fable-5-1
…= 0 is redrawn Step 11 reads: "If r = 0 or if s = 0, and k was generated deterministically (using the procedure in 6.3.2), then output failure. Otherwise, if r = 0 or if s = 0, then go to Step 3." All seven ECDSA* signers returned an error on the random path too; do_sign_final_rng now draws another k and retries. ECDetDSA* keeps its failure, as step 11 directs, with an error string that cites the step instead of telling the caller to "regenerate k and retry", which cannot help when k is a function of (d, message). sign_with_k returns Option, leaving each caller to apply its half of step 11. The crate docs, rfc6979::generate_k and rfc6979_bp256r1 now cite step 11 and note that RFC 6979 §3.4 would instead continue its candidate loop. cargo mutants scoped to sign_with_k, do_sign_final_rng and ECDetDSA*::do_sign_final in the seven ecdsa_*.rs files: 77 mutants, 40 caught, 30 unviable, 7 timeouts, 0 missed. The timeouts are `sign_with_k -> None` on each curve, which makes the new redraw loop spin: kills. Assisted-by: Claude:claude-opus-5-5
…y and IDA citations GB/T 32918.2-2016 §6.1 steps A5 (r = 0 or r + k = n) and A6 (s = 0) send the signer back to step A3 for a new k. The module's "Retry convention" returned an error instead, to match bouncycastle-ecdsa, whose randomised signers now redraw k under FIPS 186-5 §6.4.1 step 11. sign_with_k returns Option and the new sign_with_random_k loops over steps A3-A7; sign, do_sign_final and sign_randomized all go through it. The probability of each A5/A6 branch is about 2^-256 (1/n for r = 0 and s = 0, 1/(n-1) for r + k = n, n close to 2^256); sm2.rs said 1/(n-1) for both A5 disjuncts and lib.rs 2^-255. The CLI's missing-identity error and module docs cite GB/T 32918.2-2016 §5.5 for IDA, as the library's own errors now do, instead of draft-shen-sm2-ecdsa-02 S5. cargo mutants scoped to the signing functions in sm2.rs: 20 mutants, 10 caught, 8 unviable, 2 timeouts, 0 missed. The timeouts (`sign_with_k -> None`, `s == 0` flipped to `!=`) make the redraw loop never accept a k: kills. Assisted-by: Claude:claude-opus-5-5
SM2 verification (GB/T 32918.2-2016 §7.1) inverts nothing, and the method's own docs said bouncycastle-sm2 never calls it. A public variable-time inverse with no caller only invites a secret to be passed through it later. Its agreement test and its criterion entry go with it. Assisted-by: Claude:claude-opus-5-5
…ejection figure Each brainpool module stated its rejection rate twice, in the module docs and in generate_k's; the module docs now point at generate_k, which keeps the figures. rfc6979_p256k1 said n is "within 2^-127 of 2^256" and a candidate is rejected with probability "about 2^-128": for a uniform candidate those are one quantity. It now says 2^256 - n is about 2^128 (2^128.35), giving the 2^-128 rejection probability. Docs only. Assisted-by: Claude:claude-opus-5-5
…heck The seven candidate_in_range bodies were the same four lines (nat::is_zero, nat::sub, Condition::from_lsb, to_bool), differing only in limb count and N_LIMBS. They now call a generic rfc6979::limbs_in_range<L>, so the constant-time shape and its argument live once; each curve keeps its own candidate_in_range for the byte-to-limb conversion and its boundary test. Release asm is unchanged in shape: every generate_k keeps one conditional jump (P-521 two, the second the counter of its public copy loop), no bcmp, and the helper is inlined. cargo mutants scoped to limbs_in_range and the seven candidate_in_range: 19 mutants, 19 caught. Assisted-by: Claude:claude-opus-5-5
`keys`, `rfc6979` and `extra_bits` were the only unsuffixed modules, a relic of P-256 being the first curve written; the CLI had been aliasing `keys as keys_p256`. They are now `keys_p256`, `rfc6979_p256` and `extra_bits_p256`, and the matching test files `wycheproof.rs`, `rfc6979_vectors_tests.rs` and `extra_bits_tests.rs` are `wycheproof_p256.rs`, `rfc6979_p256_vectors_tests.rs` and `extra_bits_p256_tests.rs`, so every curve's modules and tests are found by the same name pattern. No code changes beyond the paths. Assisted-by: Claude:claude-mythos-5-1
`keygen()` instantiated the library's default DRBG inside the call and `keygen_from_rng(rng)` was the form that took one; the no-argument form goes and the RNG-taking form is `keygen(rng: &mut dyn RNG)`, in every curve's `keys_*` module and in SM2's, the rule the rest of the library follows for anything that draws randomness. The strength check on the RNG (FIPS 186-5 Appendix A.2.1) is unchanged. The CLI's `keygen` actions draw from `HashDRBG_SHA512::new_from_os()`, as the signers do; the `ecdsa_cmd` generic takes `fn(&mut dyn RNG) -> ..`. Tests, benches, the stack harnesses and the crate docs pass an RNG; the shared suites' `keygen: fn() -> ..` parameter is satisfied with a closure. Assisted-by: Claude:claude-mythos-5-1
…ed over the rebase onto its release-based history This branch took feature/key-rework-memory by merge, and the merge commits carried the adaptations to what arrived. The rebase onto the branch's new history (key-rework-memory rebased onto release/0.1.3alpha, which carries feature/key-rework as one squash commit) dropped those merges; this commit restores the affected files to the branch's pre-rebase tip, so the tree is identical to it. Assisted-by: Claude:claude-mythos-5-1
…ed over the rebase onto its release-based history This branch took feature/key-rework-memory by merge, and the merge commits carried the adaptations to what arrived. The rebase onto the branch's new history (key-rework-memory rebased onto release/0.1.3alpha, which carries feature/key-rework as one squash commit) dropped those merges; this commit restores the affected files to the branch's pre-rebase tip, so the tree is identical to it. Assisted-by: Claude:claude-mythos-5-1
The roadmap said RSA, ECDSA and ECDH were deliberately off the list. ECDSA is on this branch, RSA is on feature/rsa-sig and ECC CDH is planned, so the three join the signature and key-establishment lists and the aside says there was a change of heart. Assisted-by: Claude:claude-fable-5-1
The FIPS 186-5 Appendix A.4.1 "extra random bits" reduction (`reduce_wide_bits_mod_n_minus_1`: bits2int, mod n-1, plus one) lived in bouncycastle-ecdsa as six `extra_bits_*` modules plus a private copy for P-521 inside `keys_p521.rs`. SP 800-56A Rev. 3 Section 5.6.1.2.1 steps 5-6 are the same computation for key-establishment keys, and a key-agreement crate must not depend on a signature crate for it, so the seven reductions and their Barrett/fold constants move to bouncycastle-ec as `<curve>_extra_bits` modules, with the P-521 one made public under the same signature as the others and its unit tests becoming integration tests in `crypto/ec/tests/`. The six existing test files move with their modules. No reduction, constant or test value changes; the ecdsa diff is imports, doc links and the deleted files. bouncycastle-ec gains bouncycastle-hex as a dev-dependency for the moved tests. Refactor ahead of the ECC CDH work (local/eccdh_plan.md, Phase 1). Assisted-by: Claude:claude-fable-5-1
…urves `<curve>_mul::multiply_point(k, Q)` computes `[k]Q` for a secret `k` and any point `Q`, which ECC CDH (SP 800-56A Rev. 3 Section 5.7.1.2, `P = h d_A Q_B`) needs and neither existing multiplier offers: the comb is fixed-base and the wNAF multiplier is variable-time on public scalars. Fixed window of width 4, read off the scalar's limbs at positions fixed by the loop; a per-call Jacobian table of `[0..16)Q` read by a masked scan of every entry; the complete, branch-free `add` for the accumulator, so the identity, same-point and opposite-point cases need no offset. The table and working entries are scrubbed before returning, through new `zeroize` methods on each curve's field element and Jacobian point; `select_point` becomes `pub(crate)` for the scan. Bits at or above the scalar's width read as zero, which matters only for P-521's overhanging top window. Cost on P-256: 136 us against the comb's 44 us and the wNAF's 106 us; P-521 733 us (`*_point_benches` gain a `multiply_point` case). Tests per curve cross-check `[k]G` against the comb, `[k]Q` against the wNAF multiplier and against `[a k mod n]G`, and cover `k = 0`, `1`, `n-1`, `Q` the identity, `Q` negated and a non-canonical-Z `Q`. Mutants (apollo, scoped to the seven `*_mul.rs`): 231 tested, 175 caught, 14 unviable, 7 timeouts, 35 missed. The timeouts are `low_bit + WIDTH` turned into a subtraction, which wraps the window range to an unbounded loop. The misses are five equivalents per curve: the table's even/odd branch (both arms build the same point), `j < BITS` loosened to `<=` (reads a bit that is zero in a reduced scalar), `j % 64` as `j + 64` (the shift amount is masked in release builds), and `|` as `^` on a cleared low bit. Phase 2 of local/eccdh_plan.md. Assisted-by: Claude:claude-fable-5-1
… the rebase onto release/0.1.4alpha This branch took release/0.1.3alpha (029a17b, the squash merge of PR #190) by merge, and that merge commit carried the adaptations to what arrived. The rebase onto release/0.1.4alpha dropped the merge; this commit restores the affected files to the branch's pre-rebase tip, so the tree is identical to it apart from the 0.1.4 line's own version bump and notes file. Assisted-by: Claude:claude-fable-5-1
|
Rebased onto release/0.1.4alpha (ac20638: the 0.1.3 tree with the version bump and the new notes file) and retargeted there: feature/ecdsa is now 69df164, replayed from a42ce74 with the merge of 029a17b dropped and the tree restored to a reference merge, so it is identical to the old tip apart from the 0.1.4 setup. Workspace suite passes on the new tip. The replay's base was the key-rework-memory tip 81cef74 this branch sat on, not the release squash, so only its own commits were replayed. |
Summary
Adds
bouncycastle-ec(constant-time custom-curve field/point/scalar arithmetic for P-256/384/521, secp256k1, Brainpool 256/384/512, and SM2),bouncycastle-ecdsa(ECDSA over all of those curves, RFC 6979 deterministic nonces), andbouncycastle-sm2(the SM2 signature scheme). This is a port of Bouncy Castle Java's custom-curve (org.bouncycastle.math.ec.custom), Brainpool, and SM2 implementations.Description
BigInteger-style variable-width arithmetic on secret data), Jacobian point arithmetic, and a fixed-base comb for[k]Gplus wNAF for variable-base multiplication.k, DER and rawr || sencodings, CAVP + Wycheproof vectors per curve.ZAuser-identity hash.Scope and Risk
New crates only, layered on top of
feature/xof-cshake.Validation
cargo test --workspace: all passing (CAVP P-256/384/521, Wycheproof per curve, RFC 6979 vectors, SM2 KAT vectors).cargo mutants -p bouncycastle-sm2(the whole crate): 98 mutants, 52 caught, 46 unviable, 0 missed.cargo mutants -p bouncycastle-ecdsa(full crate, completed): 921 mutants, 538 caught, 371 unviable, 3 missed, 9 timeouts, 0 failures. The 3 missed are XOR/OR equivalences; the 9 timeouts are allhmac_k -> Secret::new()stub mutations across the seven curves' RFC 6979 files, which create a genuine non-terminating reseed loop rather than a fast failure -- effectively kills. See the PR comment for the breakdown.cargo mutants -p bouncycastle-ec(77 files, full crate): built up incrementally per curve/file during development, same as above, with findings fixed along the way; this 3318-mutant, 23h full-crate run was to confirm the final aggregate for the record: 2551 caught, 551 unviable, 151 missed, 65 timeouts.Note on the failing "Tests" CI check
bouncycastle-ecdsa's CAVP/wycheproof suites deliberately fail rather than skip when their datais absent (unlike the rest of the workspace). Both data sets are public on GitHub
(
bcgit/bc-test-data,C2SP/wycheproof), so this workflow now checks them out into the workspaceand symlinks them to the sibling paths the tests expect (
actions/checkout'spathcan't pointoutside the workspace root directly, hence the symlink step). That gets most of the way there, but
the public
bcgit/bc-test-datamirror is missing at least one file the private origin has(
crypto/cavp/ECDSA_SigGen.txt), socavp_siggenfor P-256 still fails -- a gap in that mirror,not in this PR. Validated locally instead, where the private
bc-test-datais complete (seeValidation below);
cargo fmt,BuildandDocumentationall pass.AI Usage Statement
Assisted-by: Claude:claude-sonnet-5
🤖 Generated with Claude Code