Feature/ecdsa fix/ct review and sm2

crypto/ec/src/lib.rs

@@ -142,7 +142,7 @@
 //! of `G` its width-7 window over `G` indexes (2 KiB for a 256-bit curve, 4.5 KiB for P-521),
 //! read by public digit and so with no scan.
 //!
-//! # Security Considerations
+//! # 🚨 Security Considerations 🚨
 //!
 //! Every arithmetic primitive here is written to be branch-free and to avoid indexing by a
 //! secret value, per this workspace's constant-time rules. See [`p256`]'s module docs for the

crypto/ec/src/p256k1.rs

@@ -9,12 +9,19 @@
 //!
 //! # Reduction algorithm
 //!
-//! Same fold as [`crate::p256`]/[`crate::p384`]: `2^256 = p + C` where `C = 2^32 + 2^9 + 2^8 + 2^7
-//! + 2^6 + 2^4 + 1` (33 bits -- even smaller relative to the modulus than P-384's `C < 2^129`).
-//! Two folds bound the result to within a single extra bit of `2^256`, exactly like
-//! [`crate::p384::reduce`]: that function's own doc comment has the full derivation and the
-//! wycheproof-caught bug an earlier, `debug_assert`-based version of this shape produced by
-//! assuming that extra bit was always `0`. This module folds it in explicitly from the start.
+//! `2^256 = p + C` with `C = 2^32 + 2^9 + 2^8 + 2^7 + 2^6 + 2^4 + 1` (33 bits), so a 512-bit
+//! product `t = hi * 2^256 + lo` is congruent mod `p` to `lo + hi * C`, a value only about 33 bits
+//! wider than `lo`. The private `reduce` applies that fold twice, which leaves at most a single
+//! bit above `2^256`, then folds that bit in under a mask; the bounds that make exactly those
+//! steps sufficient are derived in `reduce`'s own comment. This is the only reduction of its
+//! shape in the crate -- [`crate::p256`], [`crate::p384`] and [`crate::sm2`] evaluate the term
+//! tables of their respective standards, and the Brainpool curves use [`crate::montgomery`] --
+//! so the argument is written out here rather than by reference to another curve.
+//!
+//! The masked third fold is not a formality. An earlier P-384 version of this same two-fold shape
+//! `debug_assert`ed that the bit was always `0`, which held for every random trial and was wrong:
+//! wycheproof's `ecdsa_secp384r1` suite found a product that sets it. `p256k1_field_tests.rs`
+//! constructs and pins a product that sets it on this curve.
 
 use crate::nat;
 use bouncycastle_utils::ct;
@@ -287,13 +294,23 @@ fn reduce(t: &[u64; 8]) -> [u64; 4] {
         debug_assert_eq!(carry, 0, "secp256k1 reduction fold overflowed 512 bits");
         acc = sum;
     }
-    // After exactly 2 folds, `acc`'s high 4 limbs are at most a single bit -- see
-    // crate::p384::reduce's doc comment for the general proof (identical shape: fold 1 bounds
-    // `hi < 2^34ish` since `C < 2^33` here, fold 2 then bounds the result to within a single bit
-    // of `2^256`). Folded in explicitly rather than assumed zero, per that function's own
-    // wycheproof-caught lesson (verified here too, not checked in, against 2,000,000 random
-    // trials plus the `(p-1)*(p-1)` worst case, none of which happened to hit the rare case --
-    // exactly the kind of gap a real-world adversarial test suite, not random sampling, catches).
+    // After exactly two folds the high half of `acc` is a single bit. The bounds, with `C < 2^33`
+    // and `acc_i = hi_i * 2^256 + lo_i` the value after fold `i`:
+    //
+    //   (1) `t < 2^512` splits as `hi_0 * 2^256 + lo_0` with `hi_0, lo_0 < 2^256`, so fold 1's
+    //       `acc_1 = lo_0 + hi_0 * C < 2^256 + 2^256 * C < 2^290`: it fits in 8 limbs (the
+    //       `carry == 0` asserted in the loop) and its high half is `hi_1 <= C`.
+    //   (2) fold 2's `acc_2 = lo_1 + hi_1 * C < 2^256 + C^2 < 2^256 + 2^66`, so `acc[4]` is `0` or
+    //       `1` and `acc[5..8]` are `0`.
+    //   (3) when `acc[4]` is `1`, `lo_2 = acc_2 - 2^256 < C^2`, so `lo_2 + C < 2^66 + 2^33 < p`:
+    //       folding the bit in can neither carry out of 256 bits nor produce a value `>= p`.
+    //
+    // The bit is folded in under a mask rather than assumed to be `0`: it is reachable from
+    // canonical operands, and `p256k1_field_tests.rs` constructs a product that sets it (the
+    // operands, and why sampling never finds them, are derived there). Without this fold that
+    // product's result is short by exactly `C`, the P-384 bug the module docs describe. By (3)
+    // the fold itself cannot carry, which the `debug_assert` pins, so `folded < 2^256 < 2p` and
+    // one conditional subtraction of `p` makes it canonical.
     let extra_bit = acc[4];
     let low: [u64; 4] = [acc[0], acc[1], acc[2], acc[3]];
     let mut c_or_zero = [0u64; 4];
@@ -303,17 +320,10 @@ fn reduce(t: &[u64; 8]) -> [u64; 4] {
         &[0u64; 4],
         &mut c_or_zero,
     );
-    let (folded, carry1) = nat::add(&low, &c_or_zero);
-    let (folded_plus_c, _) = nat::add(&folded, &C_LIMBS); // valid when carry1 == 1: folded < 2^33 there
-    let (diff, borrow) = nat::sub(&folded, &P_LIMBS); // valid when carry1 == 0
-    let mut when_no_carry = [0u64; 4];
-    ct::conditional_select(Condition::<u64>::from_lsb(borrow), &folded, &diff, &mut when_no_carry);
+    let (folded, carry) = nat::add(&low, &c_or_zero);
+    debug_assert_eq!(carry, 0, "secp256k1 third fold carried out of 256 bits");
+    let (diff, borrow) = nat::sub(&folded, &P_LIMBS);
     let mut result = [0u64; 4];
-    ct::conditional_select(
-        Condition::<u64>::from_lsb(carry1),
-        &folded_plus_c,
-        &when_no_carry,
-        &mut result,
-    );
+    ct::conditional_select(Condition::<u64>::from_lsb(borrow), &folded, &diff, &mut result);
     result
 }

crypto/ec/src/sm2.rs

@@ -274,8 +274,9 @@ fn widen(v: &[u64; 4]) -> [u64; 5] {
     [v[0], v[1], v[2], v[3], 0]
 }
 
-/// `small * v`, one limb times a 4-limb value, as 5 limbs. Only ever called with a `small` that is
-/// a bounded, operand-independent count (see [`reduce`]), never a secret.
+/// `small * v`, one limb times a 4-limb value, as 5 limbs. `small` is [`reduce`]'s accumulator top
+/// limb: a function of the operands, bounded in `[4, 18]`, and only ever a multiplicand here --
+/// the loop's trip count and memory accesses do not depend on it.
 fn mul_small(small: u64, v: &[u64; 4]) -> [u64; 5] {
     let mut out = [0u64; 5];
     let mut carry: u128 = 0;
@@ -369,17 +370,19 @@ fn widening_square(a: &[u64; 4]) -> [u64; 8] {
 ///   D4 = ( 0   || 0   || 0   || 0   || 0   || A14 || 0   || 0   )
 /// ```
 ///
-/// **Precondition: `t < p^2`**, matching SP 800-186 §G.1's own precondition for the analogous NIST
-/// tables ("given an integer A less than m^2") and holding for every call site, since `mul` is the
-/// only one and it passes the product of two canonical (`< p`) field elements. The bounds below
-/// rest on it; a raw 512-bit value near `2^512` is *not* reduced correctly by this function.
+/// Both call sites, `mul` and `square`, pass the product of two canonical (`< p`) field elements,
+/// so `t < p^2` as in SP 800-186 §G.1's analogous NIST tables ("given an integer A less than
+/// m^2"); the bounds below do not need that, and hold for any 8-limb `t`.
 ///
-/// Bounds. Each term is a 256-bit value, so `T + S1 + ... + S13 < 14*2^256` and `D1 + ... + D4 <
-/// 4*2^256`. Starting the accumulator at `5p` keeps it non-negative throughout (`5p > 4*2^256`,
-/// since `5p - 4*2^256 = 2^256 - 5*2^224 - ... > 0`) and bounded by `5p + 14*2^256 < 19*2^256`, so
-/// five limbs always suffice and the top limb is at most 18. Folding that limb back in via
-/// `2^256 ≡ C` adds `u_hi*C < 19*2^225 < 2^230`, leaving `V < 2^256 + 2^230`; since `p > 2^256 -
-/// 2^225`, `V - p < 2^230 + 2^225 < p`, so exactly one conditional subtraction finishes the job.
+/// Bounds. Each term is a 256-bit value, so `T + S1 + ... + S13 < 14*2^256`; the four `D` words
+/// all sit at bits 64-95, so `D1 + ... + D4 < 4*2^96`. Any bias of `2^98` or more therefore keeps
+/// the accumulator non-negative; `5p` is used because it also keeps the top-limb bound a clean
+/// 18: `5p + 14*2^256 < 19*2^256`, so five limbs always suffice. (`sm2_field_tests.rs` drives the
+/// limb through 4 to 17 on products; only non-product input, such as all ones, reaches 18.)
+/// Folding that limb
+/// back in via `2^256 ≡ C` adds `u_hi*C < 19*2^225 < 2^230`, leaving `V < 2^256 + 2^230`; since
+/// `p > 2^256 - 2^225`, `V - p < 2^230 + 2^225 < p`, so exactly one conditional subtraction
+/// finishes the job.
 fn reduce(t: &[u64; 8]) -> [u64; 4] {
     let a = |i: usize| word(t, i) as i64;
 
@@ -410,8 +413,8 @@ fn reduce(t: &[u64; 8]) -> [u64; 4] {
     // `V = sum(columns[w] * 2^(32 w))`, assembled as a two's-complement 5-limb value with one
     // signed carry pass: `cur >> 32` is an arithmetic shift, so a negative running sum carries a
     // negative amount into the next word, and the final carry (which can be negative) becomes the
-    // sign-extended top limb. `V` itself may be negative here -- the `D` terms can exceed the
-    // `S` terms -- which is what the `5p` bias is for.
+    // sign-extended top limb. `V` itself may be negative here -- the `D` words can exceed the
+    // `S` words in column 2 -- which is what the `5p` bias is for.
     let mut v = [0u64; 5];
     let mut carry: i64 = 0;
     for w in 0..8 {
@@ -420,7 +423,7 @@ fn reduce(t: &[u64; 8]) -> [u64; 4] {
         carry = cur >> 32;
     }
     v[4] = carry as u64;
-    // `V + 5p`, wrapping: the docs prove `V > -4 * 2^256`, so the true sum is non-negative and
+    // `V + 5p`, wrapping: the docs prove `V > -4 * 2^96`, so the true sum is non-negative and
     // below `19 * 2^256`, and the wrapping add of two 5-limb values recovers it exactly.
     let (acc, _) = nat::add(&v, &FIVE_P_LIMBS);
 

crypto/ec/src/sm2_scalar.rs

@@ -1,5 +1,7 @@
 //! The SM2 scalar field, arithmetic mod the curve order `n` (`draft-shen-sm2-ecdsa-02` Appendix
-//! D), needed for `k⁻¹` and `s = k⁻¹(e + rd) mod n` in ECDSA-shaped signing.
+//! D), for SM2 signing's `r = e + x1` and `s = (1 + dA)^-1 (k - r dA) mod n` (GB/T 32918.2-2016
+//! §6.1 steps A5-A6, with `(1 + dA)^-1` inverted once per key) and verification's `t = r' + s'`
+//! (§7.1 step B5). SM2 has no `k^-1`.
 //!
 //! Unlike `p`, `n` is not a Solinas prime -- there is no special shape to exploit for reduction --
 //! so this uses Montgomery arithmetic instead: [`Sm2ScalarField`] holds its value as `x*R mod n`
@@ -126,13 +128,13 @@ impl Sm2ScalarField {
 
     /// Overwrites this value with zero, through a write the compiler may not elide.
     ///
-    /// For the signing path, where a value of this type holds `d`, `k` or `k^-1` and would
+    /// For the signing path, where a value of this type holds `d`, `k` or `(1 + d)^-1` and would
     /// otherwise stay legible on the stack after the signature is returned. This type is `Copy`
-    /// and carries public values too (verification's `u`, `v`, `e`, `r`, `s`), so it is not
+    /// and carries public values too (verification's `e`, `r'`, `s'`, `t`), so it is not
     /// wrapped in [`bouncycastle_utils::secret::Secret`] -- that would cost its `Copy`, its `const
     /// ZERO`/`ONE`, and a scrub on every intermediate including the public ones. The trade is that
     /// scrubbing is the caller's job, on every path out of the function; see
-    /// `bouncycastle_ecdsa`'s `sign_with_k` for the intended use.
+    /// `bouncycastle_sm2`'s `sign_with_k` for the intended use.
     ///
     /// This does not reach values the compiler kept in registers or spilled itself, and is
     /// defence in depth rather than a guarantee.
@@ -143,19 +145,16 @@ impl Sm2ScalarField {
     /// `self^2`, via a dedicated squaring rather than `self.mul(self)` -- see
     /// [`widening_square`].
     ///
-    /// Used by the exponentiation loops ([`Self::invert`] and friends), which call it hundreds of
-    /// times in a row with nothing else competing for registers. Deliberately *not* used by this
-    /// curve's point arithmetic, which squares by calling `mul` with equal arguments: that was
-    /// tried and measured on P-256, and despite `square` being ~9% cheaper than `mul` in
-    /// isolation it made the constant-time comb multiplier consistently slower, because inlining
-    /// a third wide-multiply routine into that loop costs more than the saved partial products
-    /// return. See [`crate::p256::P256FieldElement::square`] for the numbers.
+    /// Used only by [`Self::invert`]'s exponentiation loop, which calls it hundreds of times in
+    /// a row with nothing else competing for registers. (The point arithmetic is on the base
+    /// field, whose own `square` is likewise kept out of it; see
+    /// [`crate::p256::P256FieldElement::square`] for the measurements behind that.)
     pub fn square(&self) -> Self {
         Self(redc(&widening_square(&self.0)))
     }
 
-    /// Converts a secret scalar (`d` or `k`) into Montgomery form for field arithmetic (`k⁻¹`, `r*d`,
-    /// ...). Reads the secret's limbs once into a plain value, the same "protects storage, not
+    /// Converts a secret scalar (`d` or `k`) into Montgomery form for field arithmetic (`r*d`,
+    /// `k - r*d`, ...). Reads the secret's limbs once into a plain value, the same "protects storage, not
     /// every derived computation" boundary [`crate::sm2_comb::comb_multiply_base_point`] already
     /// crosses to read `k`'s bits.
     pub fn from_secret(secret: &Sm2Scalar) -> Self {
@@ -386,7 +385,7 @@ impl Sm2Scalar {
         Self::from_limbs(crate::sm2_sec1::limbs_from_be_bytes(bytes))
     }
 
-    /// Encodes to SEC 1 §2.3.7 big-endian octets -- the wire form of an ECDSA private key.
+    /// Encodes to SEC 1 §2.3.7 big-endian octets -- the wire form of an SM2 private key.
     /// Momentarily holds the plain value in the returned array, same as any other `encode()` on a
     /// `SignaturePrivateKey`: the caller asked for the bytes.
     pub fn to_be_bytes(&self) -> [u8; 32] {
@@ -402,9 +401,9 @@ impl Sm2Scalar {
 }
 
 /// A scalar mod `n`, `< n`, held as a plain (non-secret) value -- for a value that is public by
-/// construction: the `u`, `v` scalars ECDSA verification computes from a signature and public
-/// data, or a message-derived value. See [`Sm2Scalar`]'s docs for why there is no conversion the
-/// other way.
+/// construction: the `s'` and `t = r' + s'` scalars SM2 verification computes from a signature
+/// and public data, or a message-derived value. See [`Sm2Scalar`]'s docs for why there is no
+/// conversion the other way.
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub struct Sm2PublicScalar([u64; 4]);
 
@@ -420,10 +419,11 @@ impl Sm2PublicScalar {
     }
 
     /// `self^-1 mod n` in **variable time**, as a [`Sm2ScalarField`] ready for the arithmetic
-    /// that follows, or `0` for `0`. For ECDSA verification's `s` (FIPS 186-5 §6.4.2 step 4),
-    /// which is part of the signature and so already public; nothing derived from a private key
-    /// or a per-message secret may reach this, which is why it lives on the public scalar type
-    /// and not on [`Sm2ScalarField`], whose `invert` is the constant-time one. See
+    /// that follows, or `0` for `0`. SM2 verification (GB/T 32918.2-2016 §7.1) inverts nothing,
+    /// so `bouncycastle-sm2` never calls this; it is kept for parity with the ECDSA curves'
+    /// public scalar types, for a value that is public by construction. Nothing derived from a
+    /// private key or a per-message secret may reach this, which is why it lives on the public
+    /// scalar type and not on [`Sm2ScalarField`], whose `invert` is the constant-time one. See
     /// [`crate::inverse_vartime`].
     pub fn invert_vartime(&self) -> Sm2ScalarField {
         Sm2ScalarField::from_limbs(crate::inverse_vartime::mod_inverse(&self.0, &N_LIMBS))

crypto/ec/src/sm2_sec1.rs

@@ -14,7 +14,7 @@
 //! [`encode_uncompressed`] is SEC 1 §2.3.3 case 3 (`04 || X || Y`) and [`encode_compressed`] is
 //! case 2 (`02 || X` or `03 || X`, tag selected by `y mod 2`). Both take affine coordinates
 //! directly rather than a [`Sm2JacobianPoint`], not a `[u8; 1]` "point at infinity" case: SEC 1
-//! §2.3.3 case 1 encodes `∅` as a single `0x00` octet, but an ECDSA-shaped public key -- the only
+//! §2.3.3 case 1 encodes `∅` as a single `0x00` octet, but an SM2 public key -- the only
 //! caller -- can never legitimately be `∅`, so pushing the infinity check to the type (a caller
 //! must have already gotten `Some(x, y)` out of [`Sm2JacobianPoint::to_affine`]) makes encoding
 //! infinity a compile error instead of a runtime one.
@@ -127,7 +127,7 @@ pub fn encode_compressed(x: &Sm2FieldElement, y: &Sm2FieldElement) -> [u8; 33] {
 /// Decodes and validates a SEC 1 §2.3.4 point encoding: 33 bytes (compressed, tag `0x02`/`0x03`)
 /// or 65 bytes (uncompressed, tag `0x04`). See the module docs for exactly which SP 800-186
 /// Appendix D.1.1.1 checks this performs and why it constitutes full (not merely partial)
-/// validation for P-256. Returns `None` for anything invalid.
+/// validation for SM2. Returns `None` for anything invalid.
 pub fn decode(bytes: &[u8]) -> Option<(Sm2FieldElement, Sm2FieldElement)> {
     match bytes.len() {
         33 => {
@@ -187,7 +187,7 @@ pub fn decode_point(bytes: &[u8]) -> Option<Sm2JacobianPoint> {
 // check, so no integration test through the public API can pin either one's pass/fail boundary
 // independent of that coincidence -- the QUALITY_AND_STYLE.md "private function, known-answer,
 // can't be reached cleanly from outside the crate" exception applies to both. Known answers for
-// `limbs_less_than_p`: SP 800-186 S3.2.1.3 gives `p`; 0 and `p - 1` are its immediate neighbours
+// `limbs_less_than_p`: GB/T 32918.5-2017 (draft Appendix D) gives `p`; 0 and `p - 1` are its immediate neighbours
 // inside `[0, p)`, and `p` itself plus a value with only a non-adjacent limb bumped above `p`'s
 // are its immediate neighbours outside it. `either_out_of_range` is exercised over all four
 // in-range/out-of-range combinations of its two arguments, which is what pins OR against AND.

crypto/ec/src/sm2_wnaf.rs

@@ -1,6 +1,7 @@
-//! Interleaved windowed-NAF (wNAF) Shamir's-trick multiplication: `[u]G + [v]Q`, for ECDSA
-//! verification's `R' = [u]G + [v]Q` (FIPS 186-5 §6.4.2). Both scalars and both points are public
-//! -- `G` is the curve's fixed base point and `Q` a signer's public key -- so unlike
+//! Interleaved windowed-NAF (wNAF) Shamir's-trick multiplication: `[u]G + [v]Q`, for SM2
+//! verification's `(x1', y1') = [s']G + [t]PA` (GB/T 32918.2-2016 §7.1 step B6). Both scalars and
+//! both points are public -- `G` is the curve's fixed base point and `Q` a signer's public key --
+//! so unlike
 //! [`crate::sm2_comb`], this multiplier takes [`Sm2PublicScalar`], not [`crate::sm2_scalar::Sm2Scalar`],
 //! and is written for speed, not constant time: it branches freely on the digits it computes,
 //! and adds points with `add_vartime` rather than the constant-time `add` (see that method's