Constant time cmov (#241)

* Constant time `cmov`

* Adjust byte size

* Move from `to_signed_bytes_le` to `to_bytes_le`

* Add experimental p256 warning

* Remove redundant sign handling in `cmov`

* Remove unnecessary `Sub` implementation

* Improve note on missing constant-time implementations

* Move `inv0()` to `FieldElement`

* Fix Rust 1.51 compilation

* Remove unnecessary reference

Co-authored-by: Kevin Lewi <[email protected]>

src/group/p256.rs

@@ -3,6 +3,9 @@
 // This source code is licensed under the MIT license found in the
 // LICENSE file in the root directory of this source tree.
 
+// Note: This group implementation of p256 is experimental for now,
+// until hash-to-curve or crypto-bigint are fully supported.
+
 #![allow(
     clippy::borrow_interior_mutable_const,
     clippy::declare_interior_mutable_const
@@ -11,7 +14,7 @@
 use super::Group;
 use crate::errors::{InternalError, ProtocolError};
 use crate::hash::Hash;
-use core::ops::{Add, Div, Mul, Neg, Sub};
+use core::ops::{Add, Div, Mul, Neg};
 use core::str::FromStr;
 use generic_array::typenum::{U32, U33};
 use generic_array::{ArrayLength, GenericArray};
@@ -26,6 +29,7 @@ use p256_::elliptic_curve::subtle::ConstantTimeEq;
 use p256_::elliptic_curve::Field;
 use p256_::{AffinePoint, EncodedPoint, ProjectivePoint};
 use rand::{CryptoRng, RngCore};
+use subtle::{Choice, ConditionallySelectable};
 
 // `L: 48`
 pub const L: usize = 48;
@@ -160,6 +164,10 @@ impl Group for ProjectivePoint {
 
 /// Corresponds to the map_to_curve_simple_swu() function defined in
 /// <https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-hash-to-curve-11#appendix-F.2>
+// `cmov`, `mod_floor` and `modpow` needs to be made constant-time, which
+// will be supported after crypto-bigint is no longer experimental. See
+// https://github.com/novifinancial/opaque-ke/issues/239 for more context.
+
 #[allow(clippy::many_single_char_names)]
 fn map_to_curve_simple_swu<N: ArrayLength<u8>>(
     u: &[u8],
@@ -186,11 +194,6 @@ fn map_to_curve_simple_swu<N: ArrayLength<u8>>(
         fn one(&'a self) -> FieldElement<'a> {
             self.element(&BigInt::one())
         }
-
-        /// See <https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-hash-to-curve-11#section-4>
-        fn inv0(&'a self, number: &FieldElement<'a>) -> FieldElement<'a> {
-            number.pow_internal(&(self.0 - 2))
-        }
     }
 
     /// Finite field arithmetic
@@ -216,14 +219,6 @@ fn map_to_curve_simple_swu<N: ArrayLength<u8>>(
         }
     }
 
-    impl<'a> Sub for &FieldElement<'a> {
-        type Output = FieldElement<'a>;
-
-        fn sub(self, rhs: Self) -> Self::Output {
-            self.f.element(&(&self.number - &rhs.number))
-        }
-    }
-
     impl<'a> Neg for FieldElement<'a> {
         type Output = FieldElement<'a>;
 
@@ -277,7 +272,7 @@ fn map_to_curve_simple_swu<N: ArrayLength<u8>>(
 
         #[allow(clippy::suspicious_arithmetic_impl)]
         fn div(self, rhs: &Self) -> Self::Output {
-            self * rhs.f.inv0(rhs)
+            self * rhs.inv0()
         }
     }
 
@@ -309,6 +304,11 @@ fn map_to_curve_simple_swu<N: ArrayLength<u8>>(
             (&self.number % 2_usize).to_i32().unwrap()
         }
 
+        /// See <https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-hash-to-curve-11#section-4>
+        fn inv0(&self) -> Self {
+            self.pow_internal(&(self.f.0 - 2))
+        }
+
         fn is_zero(&self) -> bool {
             self.number.is_zero()
         }
@@ -332,10 +332,27 @@ fn map_to_curve_simple_swu<N: ArrayLength<u8>>(
     }
 
     fn cmov<'a>(x: &FieldElement<'a>, y: &FieldElement<'a>, b: bool) -> FieldElement<'a> {
-        if b {
-            y.clone()
-        } else {
-            x.clone()
+        let f = x.f;
+
+        let x_bytes = x.number.to_bytes_le().1;
+        let mut x = [0; 32];
+        x[..x_bytes.len()].copy_from_slice(&x_bytes);
+
+        let y_bytes = y.number.to_bytes_le().1;
+        let mut y = [0; 32];
+        y[..y_bytes.len()].copy_from_slice(&y_bytes);
+
+        let mut bytes = [0; 32];
+
+        let choice = Choice::from(u8::from(b));
+
+        for ((byte, x), y) in bytes.iter_mut().zip(&x).zip(&y) {
+            *byte = u8::conditional_select(x, y, choice);
+        }
+
+        FieldElement {
+            f,
+            number: BigInt::from_bytes_le(Sign::Plus, &bytes),
         }
     }
 
@@ -359,7 +376,7 @@ fn map_to_curve_simple_swu<N: ArrayLength<u8>>(
     // 3.   x1 = tv1 + tv2
     let mut x1 = &tv1 + &tv2;
     // 4.   x1 = inv0(x1)
-    x1 = f.inv0(&x1);
+    x1 = x1.inv0();
     // 5.   e1 = x1 == 0
     let e1 = x1.is_zero();
     // 6.   x1 = x1 + 1

src/lib.rs

@@ -819,7 +819,8 @@
 //! [curve25519-dalek](https://doc.dalek.rs/curve25519_dalek/index.html#backends-and-features) and allow for selecting
 //! the corresponding backend for the curve arithmetic used. The `u64_backend` feature is included as the default.
 //!
-//! - The `p256` feature enables the use of `p256::ProjectivePoint` as a `Group` for `CipherSuite`.
+//! - The `p256` feature enables the use of `p256::ProjectivePoint` as a `Group` for `CipherSuite`. Note that this
+//! is currently an experimental feature ⚠️, and is not yet ready for production use.
 //!
 //! - The `bench` feature is used only for running performance benchmarks for this implementation.
 //!