P-256 ECDSA verification: Clarify multiplication.

Move more of the logic for the nistz256 multiplication into Rust.
briansmith · Oct 17, 2023 · 86f4976 · 86f4976
1 parent 4fa9905
commit 86f4976
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 31 deletions.
diff --git a/build.rs b/build.rs
@@ -958,7 +958,7 @@ fn prefix_all_symbols(pp: char, prefix_prefix: &str, prefix: &str) -> String {
         "p256_point_double",
         "p256_point_mul",
         "p256_point_mul_base",
-        "p256_points_mul_public",
+        "p256_point_mul_base_vartime",
         "p256_scalar_mul_mont",
         "p256_scalar_sqr_rep_mont",
         "p256_sqr_mont",

diff --git a/crypto/fipsmodule/ec/p256-nistz.c b/crypto/fipsmodule/ec/p256-nistz.c
@@ -284,11 +284,8 @@ void p256_point_mul_base(P256_POINT *r, const Limb scalar[P256_LIMBS]) {
   limbs_copy(r->Z, p.Z, P256_LIMBS);
 }
 
-void p256_points_mul_public(P256_POINT *r,
-                            const Limb g_scalar[P256_LIMBS],
-                            const Limb p_scalar[P256_LIMBS],
-                            const Limb p_x[P256_LIMBS],
-                            const Limb p_y[P256_LIMBS]) {
+void p256_point_mul_base_vartime(P256_POINT *r,
+                                 const Limb g_scalar[P256_LIMBS]) {
   alignas(32) P256_POINT p;
   uint8_t p_str[33];
   OPENSSL_memcpy(p_str, g_scalar, 32);
@@ -336,10 +333,6 @@ void p256_points_mul_public(P256_POINT *r,
     ecp_nistz256_point_add_affine(&p, &p, &t);
   }
 
-  alignas(32) P256_POINT tmp;
-  ecp_nistz256_windowed_mul(&tmp, p_scalar, p_x, p_y);
-  ecp_nistz256_point_add(&p, &p, &tmp);
-
   OPENSSL_memcpy(r->X, p.X, P256_LIMBS * sizeof(BN_ULONG));
   OPENSSL_memcpy(r->Y, p.Y, P256_LIMBS * sizeof(BN_ULONG));
   OPENSSL_memcpy(r->Z, p.Z, P256_LIMBS * sizeof(BN_ULONG));

diff --git a/src/ec/suite_b/ops.rs b/src/ec/suite_b/ops.rs
@@ -979,6 +979,7 @@ mod tests {
     fn p256_point_mul_base_test() {
         point_mul_base_tests(
             &p256::PRIVATE_KEY_OPS,
+            |s| p256::PRIVATE_KEY_OPS.point_mul_base(s),
             test_file!("ops/p256_point_mul_base_tests.txt"),
         );
     }
@@ -987,16 +988,21 @@ mod tests {
     fn p384_point_mul_base_test() {
         point_mul_base_tests(
             &p384::PRIVATE_KEY_OPS,
+            |s| p384::PRIVATE_KEY_OPS.point_mul_base(s),
             test_file!("ops/p384_point_mul_base_tests.txt"),
         );
     }
 
-    fn point_mul_base_tests(ops: &PrivateKeyOps, test_file: test::File) {
+    pub(super) fn point_mul_base_tests(
+        ops: &PrivateKeyOps,
+        f: impl Fn(&Scalar) -> Point,
+        test_file: test::File,
+    ) {
         test::run(test_file, |section, test_case| {
             assert_eq!(section, "");
             let g_scalar = consume_scalar(ops.common, test_case, "g_scalar");
             let expected_result = consume_point(ops, test_case, "r");
-            let actual_result = ops.point_mul_base(&g_scalar);
+            let actual_result = f(&g_scalar);
             assert_point_actual_equals_expected(ops, &actual_result, &expected_result);
             Ok(())
         })

diff --git a/src/ec/suite_b/ops/p256.rs b/src/ec/suite_b/ops/p256.rs
@@ -127,30 +127,24 @@ pub static PUBLIC_SCALAR_OPS: PublicScalarOps = PublicScalarOps {
 };
 
 #[cfg(any(target_arch = "aarch64", target_arch = "x86_64"))]
-fn twin_mul_nistz256(
-    g_scalar: &Scalar,
-    p_scalar: &Scalar,
-    (p_x, p_y): &(Elem<R>, Elem<R>),
-) -> Point {
+fn twin_mul_nistz256(g_scalar: &Scalar, p_scalar: &Scalar, p_xy: &(Elem<R>, Elem<R>)) -> Point {
+    let scaled_g = point_mul_base_vartime(g_scalar);
+    let scaled_p = PRIVATE_KEY_OPS.point_mul(p_scalar, p_xy);
+    PRIVATE_KEY_OPS.common.point_sum(&scaled_g, &scaled_p)
+}
+
+#[cfg(any(target_arch = "aarch64", target_arch = "x86_64"))]
+fn point_mul_base_vartime(g_scalar: &Scalar) -> Point {
     prefixed_extern! {
-        fn p256_points_mul_public(r: *mut Limb,          // [3][COMMON_OPS.num_limbs]
-                                  g_scalar: *const Limb, // [COMMON_OPS.num_limbs]
-                                  p_scalar: *const Limb, // [COMMON_OPS.num_limbs]
-                                  p_x: *const Limb,      // [COMMON_OPS.num_limbs]
-                                  p_y: *const Limb,      // [COMMON_OPS.num_limbs]
+        fn p256_point_mul_base_vartime(r: *mut Limb,          // [3][COMMON_OPS.num_limbs]
+                                       g_scalar: *const Limb, // [COMMON_OPS.num_limbs]
         );
     }
-    let mut r = Point::new_at_infinity();
+    let mut scaled_g = Point::new_at_infinity();
     unsafe {
-        p256_points_mul_public(
-            r.xyz.as_mut_ptr(),
-            g_scalar.limbs.as_ptr(),
-            p_scalar.limbs.as_ptr(),
-            p_x.limbs.as_ptr(),
-            p_y.limbs.as_ptr(),
-        );
+        p256_point_mul_base_vartime(scaled_g.xyz.as_mut_ptr(), g_scalar.limbs.as_ptr());
     }
-    r
+    scaled_g
 }
 
 pub static PRIVATE_SCALAR_OPS: PrivateScalarOps = PrivateScalarOps {
@@ -316,3 +310,17 @@ prefixed_extern! {
         rep: Limb,
     );
 }
+
+#[cfg(test)]
+mod tests {
+    #[cfg(any(target_arch = "aarch64", target_arch = "x86_64"))]
+    #[test]
+    fn p256_point_mul_base_vartime_test() {
+        use super::{super::tests::point_mul_base_tests, *};
+        point_mul_base_tests(
+            &PRIVATE_KEY_OPS,
+            point_mul_base_vartime,
+            test_file!("p256_point_mul_base_tests.txt"),
+        );
+    }
+}