Repository navigation
Feature/ecdsa fix/ct review and sm2 - #182
Merged
dghgit merged 19 commits intoNov 2, 2026
Merged
Conversation
…re it lives [For humans] - reduce()'s third fold had no test, and its bound proof pointed at P-384 text that no longer exists - reduce() now derives its own bounds: one bit left after two folds, and folding it in cannot carry - p256k1_field_tests.rs pins a product that needs the fold; without it the result is short by C - comments and a test only; no code change in reduce() ====================================================================== [For agents] p256k1::reduce folds a 512-bit product twice with 2^256 ≡ C (mod p), then folds the single remaining bit above 2^256 in under a mask. Both the module docs and the in-function comment deferred the proof that two folds leave at most one bit -- and that the third fold cannot carry -- to crate::p384::reduce, "identical shape". Since 57d7417 P-384's reduce is the SP 800-186 G.1.3 term sum, which has no second fold and no such argument: secp256k1's is the only two-fold reduction left in the crate, and its correctness argument pointed at text that no longer exists. The argument is now written out in reduce() itself: hi_1 <= C after fold 1, acc_2 < 2^256 + C^2 < 2^256 + 2^66 after fold 2, and when the bit is set lo_2 < C^2, so lo_2 + C < p. That last bound also shows `carry1`, the carry out of the third fold, is identically zero; its comment said the opposite. The third fold had no test. The old comment noted that 2,000,000 random trials and (p-1)^2 never reached it and that an adversarial suite would -- but none was added for this curve, and the seven reduction-bound products in p256k1_field_tests.rs do not reach it either: (p-1)^2 gives fold 1 a high half of C - 1, one short of the bound, and is still ~2^190 away, because what matters is the low half after fold 1 landing within hi_1*C <= 2^66 of 2^256. One product is now pinned: a = 2^255 and b = 2 * ((2^256 - 1) * C^-1 mod 2^256), so that a*b = hi_0 * 2^256 with hi_0 * C ≡ 2^256 - 1; the two folds leave 2^256 + 0x3642e89815569618 and the third adds C. With the third fold dropped (`let extra_bit = 0`) reduce returns a value short by exactly C and every other test in the ec crate still passes; the new test fails. (In the workspace, wycheproof's ecdsa_secp256k1_sha256 tcId 135 / 364, "smallish r and s^-1", also fails on that mutant -- the P-384 history repeating -- so until now the fold was covered only from another crate.) No code change in reduce(): comments and a test only. Assisted-by: Claude:claude-fable-5-1
… pin it
[For humans]
- the brainpool rfc6979 docs called the step h.3 rejection loop near-dead, by reference to P-256
- on these curves 34 % / 45 % / 33 % of signatures reject a candidate: a visible timing signal
- each module now carries its own argument: the rejection count is independent of k and d (PRF)
- the vectors annotate the rejecting cases and add a CASE_5 per curve with five or six rejections
- all 18 vectors re-derived and reproduced by python-ecdsa 0.19.2 and OpenSSL 4.0.2
- docs and tests only
======================================================================
[For agents]
The three brainpool rfc6979 modules inherited P-256's rejection-loop text in three places, and on
these curves all three say the opposite of what happens. generate_k's doc deferred to
crate::rfc6979::generate_k's argument "which applies unchanged here": its quantitative half
("rejected with negligible probability") assumes n within 2^-32 of 2^256, where the brainpool
moduli give P[candidate >= n] = 1 - n/2^qlen of 0.336 (bp256r1), 0.4503 (bp384r1) and 0.3326
(bp512r1) -- so 34 % / 45 % / 34 % of signatures run step h.3's "otherwise" branch at least once
(mean extra iterations 0.51 / 0.82 / 0.50, each two more HMAC calls), a timing signal a remote
observer can see. Its structural half ("a rejected candidate is discarded entirely and nothing
derived from it is ever used") is not true of RFC 6979 on any curve: the accepted k is an HMAC of
the rejected V. The test-module comment claimed the reject path is never reached by realistic
inputs and took the QUALITY_AND_STYLE.md private-function exception on that basis -- on these
curves a third to a half of real messages reach it, and the checked-in vectors already did
(bp256r1 CASE_0 once; bp384r1 CASE_0 twice, CASE_4 once; bp512r1 CASE_0 and CASE_1 once each), by
accident of which (d, msg) the generator drew. And the module docs did not say the loop is a
normal control path at all.
Each module now carries its own argument instead of the reference: 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 -- which, conditioned on the count,
is uniform on [1, n-1] -- and reveals nothing about d; the count is deterministic per (d, msg), so
the timing pattern over chosen messages is a fingerprint worth about a bit per message, usable for
confirming a candidate d an attacker already holds but not for recovering one; and what must be
constant time is everything done with the accepted k. The module docs state the rejection rate and
that RFC 6979 §3.4's "utterly improbable" assumes n close to 2^qlen, which qlen == hlen alone does
not give; the test-module comment now states the coverage instead of denying it.
The vectors files pin the loop deliberately rather than by accident: the rejecting cases are
annotated with their rejection counts, and each curve gains a CASE_5 searched to need several
rejections in a row -- five (bp256r1), six (bp384r1), five (bp512r1) -- so the K = HMAC_K(V ||
0x00) re-key is exercised repeatedly and a mutant that drops it is killed by these files' own
generate_k and sign tests. The bp384r1 and bp512r1 pins are the PR 143 review's; bp256r1's is new:
d is SHA-512("bc-rust brainpoolP256r1 h.3 rejection pin") reduced into [1, n-1] and the message is
the first index of the form "brainpoolP256r1 rejection test {i}" whose rejection count reaches
five. All 18 vectors (15 existing, 3 pins) were re-derived this session from an instrumented
RFC 6979 port and reproduced by python-ecdsa 0.19.2 (its own pure-Python RFC 6979) and OpenSSL
4.0.2 (deterministic ECDSA through the cryptography 50.0.1 bindings): k, r, s and Q agree on all
18, which also anchors the h.3 continue-chain against two implementations that share no code with
this crate.
crate::rfc6979 (P-256) is deliberately untouched: its structural sentence misdescribes RFC 6979 on
every curve and its handling of an r = 0 candidate differs from the RFC's step h.3, but that is a
cross-curve item (secp256k1 review F6), not a brainpool one.
Assisted-by: Claude:claude-fable-5
…fix two doc numbers [For humans] - the ecdsa crate docs claimed d and k are never handled in variable time; the rfc6979 loop is - the crate docs now name two exceptions: the rfc6979 candidate loop and the r = 0 / s = 0 error - brainpool generate_k docs: a rejection costs three more HMAC calls, not two - secp256k1 third-fold test doc: (p-1)^2 misses the fold by C - 1, not by about 2^190 - docs and comments only ====================================================================== [For agents] The ecdsa crate docs' Security Considerations opened with "d and k are never handled in variable time ... every operation performed on it or on k is branch-free". e8acb3b documents, in the three brainpool rfc6979 modules, a timing signal that 34 % to 45 % of signatures on those curves emit: the RFC 6979 step h.3 loop iterates a data-dependent number of times. The crate-level sentence contradicted that, and was not literally true on any curve, since the loop exists everywhere. The bullet now states what holds, that everything done with d and the accepted k is branch-free, and lists the two exceptions this crate adds to the ec crate's: the candidate loop (the rates, the PRF-assumption argument by reference to rfc6979_bp256r1::generate_k, and the per-message timing fingerprint it leaves) and the r = 0 / s = 0 error return, a branch on a public outcome. Two numbers from the last two commits were wrong. The brainpool generate_k docs said each extra iteration costs two more HMAC calls; it costs three: the re-key is two calls and the next candidate's HMAC_K(V) is the third. And the secp256k1 third-fold test's doc said (p-1)^2 "still falls about 2^190 short" of the third fold; it misses by C - 1, since its two folds leave exactly p + 1 (hi_0 = 2^256 - 2C - 2, lo_0 = (C+1)^2, acc_1 = C * 2^256 - C^2 + 1, acc_2 = 2^256 - C + 1). The 2^-190 figure is the probability for a pseudo-random product, which reduce()'s comment states correctly; that comment no longer files (p-1)^2 under the same probability either. Docs and comments only; no code change. Assisted-by: Claude:claude-fable-5-1
…ptimisation barrier [For humans] - to_bool is `self.0 != 0`: LLVM can fold the mask away and branch on the inputs that built it - to_bool_opaque runs the mask through the volatile barrier first, so the one branch is on the mask - added to all four widths (u64, u32, i64, i32), with a test per width in ct_tests.rs - to_bool is unchanged: it is const and has about 100 callers ====================================================================== [For agents] Condition's consumers that do masked arithmetic (select, mov, swap, conditional_select) all pass the mask through barrier(), a volatile store and load, before using it. to_bool does not: it is 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 folds the mask away and emits one branch per input comparison. Observed in release asm of bouncycastle_ecdsa's rfc6979::generate_k: a mask built from nat::is_zero and from_lsb(borrow) and converted with to_bool() still produced two conditional jumps, one on the all-zero test and one on the borrow; the same mask converted with select(1, 0) == 1, which passes the barrier, produced exactly one jump on the barrier-loaded value. to_bool_opaque is that second form as a method: `self.barrier().0 != 0`. It is for the case where the outcome of a decision is public but the inputs are secret, so the decision must be made on the whole mask and not on its constituents. It is not const because barrier() is not. to_bool is left as it is: const, about 100 call sites across the workspace, and whether it should itself pass the barrier is a separate question that needs a census of those callers first. Per CLAUDE.md the four `impl Condition<W>` blocks are hand-written copies and a change to one width is applied to every width; the method is added to all four, and ct_tests.rs gains one test per width module that checks TRUE and FALSE and agreement with to_bool on masks built from the boundary values and their negations. cargo test -p bouncycastle-utils: all pass. Assisted-by: Claude:claude-fable-5-1
…n all seven curves
[For humans]
- candidate_in_range returned a bool from `limbs == [0; L]` and `nat::sub(..).1 == 1`
- release asm: `limbs == [0; L]` became a bcmp libcall over the candidate, then limb compares and jumps
- it is now two masks (nat::is_zero, from_lsb(borrow)) joined and converted with to_bool_opaque
- every generate_k now has one data-dependent conditional jump, on the barrier-loaded mask, and no bcmp
- cargo test -p bouncycastle-ecdsa, all 33 test binaries pass (CAVP and wycheproof included)
======================================================================
[For agents]
RFC 6979 step h.3 decides whether a candidate is in [1, n-1], and this crate's constant-time rules
(plan §7, and the crate docs' Security Considerations) permit exactly one branch on that decision:
the loop's. The function that made the decision computed `limbs == [0; L]` with an early return and
then `borrow == 1`, both plain bools. In release asm (cargo rustc --release --lib --emit=asm, with
codegen-units=1) that compiled, in every one of the seven generate_k bodies, to a call to bcmp over
the candidate's L*8 bytes (`movq bcmp@GOTPCREL(%rip), %rbx ; callq *%rbx ; testl %eax, %eax ; je`,
P-521 calling `*bcmp@GOTPCREL` directly), i.e. a variable-time memcmp with early exit, followed by
compares of the candidate's limbs against n's. For P-256:
movabsq $-4294967296, %rdx ; cmpq %rdx, 88(%rsp) ; sete %dl ; jb .LBB75_19
orb %al, %cl ; andb %cl, %dl ; jne .LBB75_19
a compare of the top limb against n's top limb 0xFFFFFFFF00000000 with a jump taken when it is
smaller, and a second jump on the combined zero test and borrow. secp256k1, P-384 and P-521 had the
same shape with `cmpq $-1, <limb>` on the all-ones limbs of their n; the brainpool curves had the
top-limb compare and the combined jump. The leaked information is small (how far the memcmp got,
which limb decided the comparison), but it is a branch on k's bits that the rules and the docs
said did not exist.
The body is now
let is_nonzero = !nat::is_zero(&limbs);
let (_, borrow) = nat::sub(&limbs, &N_LIMBS);
let is_below_n = Condition::<u64>::from_lsb(borrow);
(is_nonzero & is_below_n).to_bool_opaque()
to_bool_opaque (added to Condition in the previous commit) passes the mask through the volatile
barrier before testing it; with plain to_bool() LLVM still split the test into two jumps, one on
the OR-chain and one on the borrow. Conditional jumps per generate_k, before -> after, from the
census script over the demangled release asm:
rfc6979 (P-256) 3 -> 1 rfc6979_bp256r1 3 -> 1
rfc6979_p256k1 4 -> 1 rfc6979_bp384r1 3 -> 1
rfc6979_p384 4 -> 1 rfc6979_bp512r1 3 -> 1
rfc6979_p521 4 -> 2 (the second is the `cmpq $66` byte-copy loop counter)
After: the bcmp call is gone (one call fewer per function) and the one data-dependent jump is
`movq %rax, <slot> ; cmpq $0, <slot> ; jne`, the mask reloaded through the barrier. hmac_k is a
separate symbol in both builds, so its expect on do_final_out's Result never appears here.
rfc6979.rs's doc comment on candidate_in_range gains one sentence saying the comparison is done as
masks and why; the other six copies are not given the same sentence (QUALITY_AND_STYLE.md
"Proportion": one home per fact). The seven files differ only in limb count and sec1 module name;
the body edit was applied to all seven with one regex and the diff checked by eye.
Tests: the existing candidate_in_range_boundary_cases unit tests (0, 1, n-1, n, 0xff..ff) in each
module kill the `!` and `&`->`|` mutants; the brainpool vectors suites also exercise the reject path
(five and six rejections in a row since b11afd6). Full suite with /tmp/bc-test-data and
/tmp/wycheproof symlinked: 33 binaries, all pass.
Assisted-by: Claude:claude-fable-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
ounsworth
reviewed
Oct 26, 2026
| pub fn to_bool_opaque(self) -> bool { | ||
| // The barrier (section comment above) is what stops LLVM from folding the mask away and | ||
| // branching on its constituents; it is volatile, hence this is not `const`. | ||
| self.barrier().0 != 0 |
Contributor
There was a problem hiding this comment.
Questions:
self.barrier().0is of type a i32 or similar. Are we sure that rust's!=for i32 is constant-time, and doesn't quit early? (it might be fine, and I see that the existingto_bool()already uses!=, so if this is a problem, then we already have this problem).- If you need to add this in order to prevent LLVM from folding away the mask, then does that mean that the existing
to_bool()is actually broken and should just be deleted? IE should this just replace the existingto_bool()?
Collaborator
Author
There was a problem hiding this comment.
So, this is my understanding:
- The issue arises when the compiler is able to look into the values that go into
!=. If it can guess how the mask looks like, then it can fold it away if it pleases. If it's forced to evaluate the!=in full (by using the barrier), then it seems we're in a better position to have constant time. - Yeah, we'll need to switch to the barrier variant for the private data at least. Claude ran some experiments with the two, and the difference is noticeable... so it convinced me
to_boolkinda broken.
If we change the existingto_boolto use the barrier, the only con I see is that we might lose some performance in the public operations (though Claude disagrees with this too).
Let me know and I'll make that change.
Collaborator
Author
There was a problem hiding this comment.
to_bool_opaque eliminated and to_bool switched to use the barrier.
… 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
… (RFC 6979), both reached through Signer::sign Until now one type per curve (ECDSAP256 and the other six) implemented Signer with the deterministic per-message secret of FIPS 186-5 §6.3.2 / Appendix A.3.3 (RFC 6979), and the randomised method of §6.3.1 / Appendix A.3.1 was only reachable through an inherent sign_randomized that took the caller's RNG -- so the two algorithms could not be chosen through the trait. FIPS 186-5 §6 treats them as two variants with one verifier: Deterministic ECDSA "does not impact the signature verification process". Each ecdsa_<curve> module now has two signer types, both implementing Signer and SignatureVerifier over the same key types: - ECDSA<curve> (the existing name) is ECDSA proper: Signer::sign and do_sign_final draw k by Appendix A.3.1 from the library's default RNG (DefaultRNG, instantiated per signature, as bouncycastle-mldsa and bouncycastle-sm2 already do inside their sign), and the inherent sign_rng(sk, msg, ctx, rng) / do_sign_final_rng(self, rng) take the caller's RNG instead -- the `_rng` twins QUALITY_AND_STYLE.md's suffix rules call for, replacing sign_randomized (which also gains the trait's ctx parameter, ignored as everywhere in this crate). - ECDetDSA<curve> is Deterministic ECDSA: Signer::sign and do_sign_final derive k by Appendix A.3.3 through the existing rfc6979*::generate_k, and signing draws no randomness. Verification is one private verify_final per module that both types' do_verify_final call, and sign_der / verify_der exist on both. The signing core (sign_with_k, e_from_hash, x_affine_of_signing_point) is unchanged; the new random_k collects the A.3.1 draw-and-reduce that sign_randomized did inline. The seven modules keep their identical shape; ecdsa_p256.rs was written by hand and the other six were derived from it mechanically, then formatted. SM2 is untouched apart from a doc sentence that pointed at sign_randomized. Tests: each curve's conformance suite now runs twice, with TestFrameworkSignature::new(false, ..) over ECDSA* (two signatures of one message must differ) and new(true, ..) over ECDetDSA* (must match), plus a cross-verification test (either type's signature verifies under the other), a DER round trip through either verify_der, and the streaming-misuse errors on the deterministic type. The RFC 6979 vector suites pin ECDetDSA*, the CAVP SigGen suites inject k through sign_rng, and the sign_der-equals-sign test moved onto ECDetDSA*, the only type for which that equality holds. cargo test -p bouncycastle-ecdsa -p bouncycastle-sm2 -p cli: all green, including the crate's doctests. The bc-rust CLI reproduces RFC 6979 A.2.5's P-256 "sample" signature with `ecdsa-p256 sign --deterministic`. CLI: the seven ecdsa-* subcommands take --deterministic, which instantiates the generic ecdsa_cmd over ECDetDSA* instead of ECDSA*; verification accepts either flag setting. Benches: the criterion series "sign_randomized" becomes "sign_rng" and gains a sibling "sign_deterministic" (ECDetDSA*::sign); the existing "sign" series now measures ECDSA*::sign, i.e. the randomised path including the DefaultRNG instantiation, where it measured RFC 6979 before, so its gh-pages history breaks at this commit. The stack harness gains bench_<curve>_sign_deterministic beside each bench_<curve>_sign. ./dev_scripts/quality_stats.sh ./crypto/ecdsa: unwraps in core code 28 -> 21 (sign_randomized's one-shot hash `try_into().unwrap()` per curve is gone, the streaming path having no such conversion), Err() in core code unchanged at 49. Assisted-by: Claude:claude-mythos-5-1
Contributor
|
@laruizlo heads-up: I merged feature/ecdsa into this branch and pushed it (735fae3), so the PR picks up what landed on the base today:
One thing you may want to adjust in your wording: the new timing-contract bullet in the ecdsa crate docs describes the RFC 6979 candidate loop, which now only applies to the |
dghgit
approved these changes
Oct 31, 2026
dghgit
left a comment
dghgit
left a comment
Contributor
There was a problem hiding this comment.
There's some minor documentation issues which I've forwarded in a report. Once they're dealt with I can merge this - technically it stands up well though.
[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
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
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
Three code fixes, plus the tests and docs that go with them:
candidate_in_rangereturned a bool fromlimbs == [0; L]andnat::sub(..).1 == 1. In release asmthe first became a
bcmplibcall over the candidate, followed by per-limb compares and jumps.It now joins two masks (
nat::is_zero,Condition::from_lsb(borrow)) and converts them withCondition::to_bool, which now passes the mask through the volatile barrier before the single branch(fix 3). Each
generate_know has one data-dependent conditional jump, on the barrier-loaded mask,and no
bcmp.d = n-1 could be loaded, and keygen could draw it (probability 2^-256), but no signature exists for it,
because (1 + d)^-1 is the inverse of 0. Keygen now reduces mod n-2, and
from_bytesrejects n-1 and above.Condition::to_booltested the mask without the optimisation barrier.It was
self.0 != 0. When a mask is built from values the compiler can see, LLVM may test each valuethat built it instead of the mask (on x86-64 when the second test is costly enough; on AArch64, ARM and
RISC-V whenever it can). The decision is the same, but its timing shows which condition decided it: a
rejection loop shaped like
generate_kcompiles to two jumps with the oldto_bool, one per condition.to_boolnow loads the mask through the same volatile barrier asselectandswap, on all fourwidths, so every caller gets this without a source change.
0e2bb42first added it as a separateCondition::to_bool_opaque;73e42eefolds it intoto_booland removesto_bool_opaque.The other commits are tests that pin paths no test reached before, and docs that make each crate's timing
contract name the branches it actually has.
Review follow-ups (2026-11-01): four commits on top of the feature/ecdsa merge (
735fae3) addressthe follow-up review of this branch — one behaviour-preserving code change (secp256k1's
reducecarrieda dead fourth-fold guard its own bound proof rules out) and three commits of docs and strings, including
an RFC 6979 misattribution: four docs said §3.4 calls candidate rejection "utterly improbable", but the
RFC says that of r = 0, and step h.3 attaches no probability to its loop (re-read first-hand, not from
recall). One item is deferred to its own PR: a generic helper to replace the seven per-curve
candidate_in_rangebodies.Commits
ec / ecdsa: secp256k1 and RFC 6979
def9b6bec: pin secp256k1's third reduction fold. Adds a test for the fold and writes the bound proofinto
reduce(), which used to point at P-384 text that no longer exists. Comments and a test only.b11afd6ecdsa: brainpool RFC 6979 step h.3 rejection is a normal path (34 % / 45 % / 33 % ofsignatures). Updates the docs, annotates the rejecting vectors and adds a CASE_5 per curve. All 18
vectors re-derived and reproduced with python-ecdsa 0.19.2 and OpenSSL 4.0.2.
e955a8fecdsa, ec: the crate's timing contract names its two exceptions, the RFC 6979 candidate loop andthe r = 0 / s = 0 error. Also fixes two doc numbers (three HMAC calls per rejection, not two; (p-1)^2 misses
the fold by C - 1).
0e2bb42utils: addsCondition::to_bool_opaquefor all four widths, with a test per width.to_boolis unchanged. Superseded by73e42ee.040f1b2ecdsa: the maskedcandidate_in_rangeon all seven curves (fix 1 above).432ee74ecdsa: the rejection-loop argument now lives in one place (P-256), and every curve states itsrejection rate. Corrects "discarded entirely" (RFC 6979 chains, so a rejected candidate feeds the next)
and gives bp512r1's rate as 33 %. Docs only.
sm2 / ec: findings F1 to F5 from the PR 143 review
868e28bsm2: generate and accept the private key in [1, n-2] (fix 2 above). Addsextra_bits::reduce_wide_bits_mod_n_minus_2; k keeps the n-1 reduction. Seven n-2 known answers from Python.4b445c0ec: pins the final subtraction in sm2 scalar REDC with a known answer. Before this, thecargo-mutants mutant "select forced to high" survived both crates' test suites, which leaves
u = c + n non-canonical. The test checks for canonical output, not just the value. Tests only.
5584724sm2, ec: cites GB/T 32918.1/.2-2016 throughout, with the draft's sections in parentheses.Verification steps use GB/T S7.1 numbering (B1 to B7), and the
eccrate'ssm2_*modules now describeSM2's own equations instead of leftover ECDSA ones. Docs only.
eb0bca4sm2: the timing contract names the step A5 (r = 0 or r + k = n) and step A6 (s = 0) retrybranches. The heading moves to the
# 🚨 Security Considerations 🚨form. Docs only.utils / ecdsa:
to_boolpasses the barrier73e42eeutils, ecdsa:Condition::to_booltests the mask after the volatile barrier, on all fourwidths (fix 3 above), and is no longer
const; nothing in the workspace calls it in a const context.to_bool_opaqueand its four tests are removed, and the sevencandidate_in_rangesites go back toto_bool. No test is added:assert_canonicaland the operator tests already coverto_bool, andwhether the barrier holds is checked in the asm, as for
selectandswap.Review follow-ups, on top of the feature/ecdsa merge (
735fae3)b9d3e56ec: drop the dead carry select in secp256k1's third reduction fold. Bound (3) inreduce'sown comment proves the masked fold's carry-out is identically zero, so the second
+ Cand the selecton that carry could never contribute a result; they become a
debug_assertin the file's existingstyle, and the borrow select is returned directly. No behaviour change. The pinned product's
construction now lives only in the test file, next to the KAT; the bounds stay in
reduce(). The onefollow-up commit that touches code.
60bff45ecdsa: fix the RFC 6979 attribution and give the brainpool rejection text one home. Fourdocs said §3.4 calls candidate rejection "utterly improbable"; the RFC says that of r = 0 (§2.4 and
§3.4, where it is justified by preimage resistance), and §3.2 step h.3 attaches no probability to its
loop. The brainpool rate/cost/fingerprint text now lives once, in
rfc6979_bp256r1; bp384r1 andbp512r1 state their own rates and point at it; the crate docs' timing bullet now names
ECDetDSA*(
ECDSA*draws k per FIPS 186-5 A.3.1, which has no rejection loop). Docs only.510c99dec, sm2: correct two SM2 doc strings. Anexpectstill said [1, n-2]'s predecessor range,and the REDC final-subtraction KAT's doc named the canonical
ab mod n(254 to 256 bits in its owncases) where the quantity that must sit below
2^256 - nis the Montgomery representativeabR mod n(220 to 224 bits); both recomputed from the test's constants. A string and a doc comment only.a770496ec, ecdsa: the two crate docs' headings move to the# 🚨 Security Considerations 🚨form(sm2's already had it). Two lines.
Verification
cargo fmt --all --check,cargo test --workspace --all-targets --no-runandcargo test --workspacepass on the combined branch: 251 test binaries, 1923 passed, 0 failed, 1 ignored, and no vector suite
skipped for missing data (1927 before
73e42ee; the fourto_bool_opaquetests are gone).cargo docfor utils and ecdsa adds no warnings.generate_k: one data-dependent conditional jump, on the barrier-loaded mask, andno
bcmp, the same withto_bool_opaqueand with the newto_bool. P-521 has one more jump, thecounter of its public 66-byte copy loop.
73e42ee: no function inbouncycastle_ecdsachanges its conditional-jumpcount. The only bodies that grow are the seven
sign_with_k(+10 or +11 instructions) and the sevendo_verify_final(+12), where a mask now goes through the barrier.generate_k: a zerocandidate and an all-ones candidate, rejected by different checks, sit 9.14 ticks apart with the old
to_bool(|t| = 492; control pair 0.01 ticks) and 0.00 ticks apart with the new one (|t| = 0.1).73e42ee: differences runfrom -9 % to +3 % in both directions, and the largest are speedups (P-384 sign -9 %, P-521 verify -8 %)
where the change only adds instructions, so the run resolves nothing below the VM's drift; no slowdown
is attributable to the barrier.
./dev_scripts/quality_stats.sh: fallibility counts unchanged on the SM2 key-range commit and on73e42ee.--re to_boolon utils, 12 mutants, 12 caught. SM2 commits: no new survivors on changedlines. On sm2 there is 1 pre-existing miss. On the five ec
sm2*.rsfiles, 480 mutants: 418 caught,16 missed, 3 timeouts, 43 unviable. Each of the 16 misses is an equivalent mutant, none of them on a
changed line, and the 3 timeouts are non-terminating loops (kills).
bc-rust sm2 signandpk-from-skrefuse d = n-1 and accept d = n-2.The brainpool vectors match python-ecdsa and OpenSSL.
Review follow-ups (each commit message carries its full numbers):
p256k1.rsatb9d3e56: 97 mutants — the same list before and after thechange, since the deleted statements contain nothing cargo-mutants rewrites, which is why reasoning
rather than a surviving mutant had to flag them — 79 caught, 15 unviable, 2 missed (the documented
shift/XOR equivalences, on unchanged lines), 1 timeout (an infinite-loop mutant, a kill).
a770496(rustc 1.97.0, bc-test-data and wycheproof cloned in full):cargo fmt --all --checkclean; ec, ecdsa, sm2 and utils suites: 96 test binaries, 934 passed,0 failed, with CAVP, wycheproof, the RFC 6979 vectors and the brainpool rejection pins running, not
skipped, and
known_answer_mul_reaching_the_third_foldgreen on the newreduce;cargo test --workspace --all-targets --no-runcompiles;quality_stats.shmoves docstring linecounts only (fallibility identical);
cargo docfor utils, ec, ecdsa and sm2 emits the samewarning counts at
735fae31and ata770496(ec 63, ecdsa 14, sm2 2, utils 0), none of them onthe new cross-module links, so
60bff45's intra-doc links all resolve.60bff45were checked against the RFC's text downloaded on 2026-10-31, and510c99d's bit-lengths recomputed from the test's own constants, independently of the crate.For the reviewer
API:
Condition::to_boolis no longerconst fn, on all four widths. Nothing in the workspace calls itin a const context, but outside code that did would stop compiling.
Additions:
sm2::extra_bits::reduce_wide_bits_mod_n_minus_2.Condition::to_bool_opaqueis added in0e2bb42and removed in73e42ee, so it isn't in the squashed diff.Behaviour changes:
to_boolcall now passes the barrier, one volatile store and load. That includes the 100existing calls, whose source doesn't change, among them ec's point code and
PartialEqimpls, theecdsa and sm2 verifiers, and cipher's PKCS#7
unpad.from_bytesnow use [1, n-2].loop's accept/reject, on the combined mask (its output is the same).
Review follow-ups: the only one that changes code is
b9d3e56, and its outputs are bit-identical onevery input (the removed select's condition is identically zero by bound (3)); release builds lose one
4-limb add and one masked select per secp256k1 field mul/square and gain no branch; debug builds gain
a carry assert like the two already in the file. The report listed three sites for the RFC 6979
misattribution; a fourth (the P-256 test-module comment) had the same problem and is fixed with them.
@ounsworth:
to_boolgets the barrier, though it's no longerconst.to_bool_opaqueis gone.Assisted-by: Claude:claude-opus-5-5