Repository navigation
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()?
This branch has not been deployed
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
Two 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 with a newCondition::to_bool_opaque, which passes the mask through the volatile barrier before the singlebranch. 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.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.
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.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.Verification
cargo fmt --all --check,cargo test --workspace --all-targets --no-runandcargo test --workspacepass on the combined branch: 251 test binaries, 1927 passed, 0 failed, 1 ignored, and no vector suite skipped for missing data.
generate_k: one data-dependent conditional jump, on the barrier-loaded mask, and nobcmp../dev_scripts/quality_stats.sh: fallibility counts unchanged on the SM2 key-range commit.sm2*.rsfiles, 480 mutants: 418 caught, 16 missed, 3 timeouts, 43 unviable. Each of the 16 misses is anequivalent 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.
For the reviewer
ct.rs(see below)Condition::to_bool_opaqueandsm2::extra_bits::reduce_wide_bits_mod_n_minus_2. @ounsworth:Condition::to_bool_opaqueis an addition to/utils/src/ct/rs. The "opaque" suffix doesn't seem to appear anywhere else. Is the name ok, or would you rather it be named something else?from_bytesnow use [1, n-2], and the RFC 6979 range check is nowbranch-free (its output is the same).
Assisted-by: Claude:claude-opus-5-5