Fixes for constant time Greater-Than

curve25519-dalek/src/backend/serial/fiat_u32/field.rs

@@ -270,22 +270,27 @@ impl FieldElement2625 {
     }
 
     /// Returns 1 if self is greater than the other and 0 otherwise
-    // implementation based on C libgmp -> mpn_sub_n
-    pub(crate) fn gt(&self, other: &Self) -> Choice {
+    // strategy: check if b-a overflows. if it does not overflow, then a was larger
+    pub(crate) fn gt_direct(&self, other: &Self) -> Choice {
         let mut _ul = 0_u32;
         let mut _vl = 0_u32;
-        let mut _rl = 0_u32;
 
-        let mut cy = 0_u32;
+        // carry through gt
+        let mut c_gt = false;
+        let mut gt_i: bool;
+        let mut eq_i: bool;
+
+        // start from least significant go to most significant
         for i in 0..10 {
             _ul = self.0[i];
             _vl = other.0[i];
 
-            let (_sl, _cy1) = _ul.overflowing_sub(_vl);
-            let (_rl, _cy2) = _sl.overflowing_sub(cy);
-            cy = _cy1 as u32 | _cy2 as u32;
+            gt_i = _ul > _vl;
+            eq_i = _ul == _vl;
+
+            c_gt = gt_i || (eq_i & c_gt);
         }
 
-        Choice::from((cy != 0_u32) as u8)
+        Choice::from(c_gt as u8)
     }
 }

curve25519-dalek/src/backend/serial/fiat_u64/field.rs

@@ -261,22 +261,27 @@ impl FieldElement51 {
     }
 
     /// Returns 1 if self is greater than the other and 0 otherwise
-    // implementation based on C libgmp -> mpn_sub_n
-    pub(crate) fn gt(&self, other: &Self) -> Choice {
+    // strategy: check if b-a overflows. if it does not overflow, then a was larger
+    pub(crate) fn gt_direct(&self, other: &Self) -> Choice {
         let mut _ul = 0_u64;
         let mut _vl = 0_u64;
-        let mut _rl = 0_u64;
 
-        let mut cy = 0_u64;
+        // carry through gt
+        let mut c_gt = false;
+        let mut gt_i: bool;
+        let mut eq_i: bool;
+
+        // start from least significant go to most significant
         for i in 0..5 {
             _ul = self.0[i];
             _vl = other.0[i];
 
-            let (_sl, _cy1) = _ul.overflowing_sub(_vl);
-            let (_rl, _cy2) = _sl.overflowing_sub(cy);
-            cy = _cy1 as u64 | _cy2 as u64;
+            gt_i = _ul > _vl;
+            eq_i = _ul == _vl;
+
+            c_gt = gt_i || (eq_i & c_gt);
         }
 
-        Choice::from((cy != 0_u64) as u8)
+        Choice::from(c_gt as u8)
     }
 }

curve25519-dalek/src/backend/serial/u32/field.rs

@@ -603,22 +603,27 @@ impl FieldElement2625 {
     }
 
     /// Returns 1 if self is greater than the other and 0 otherwise
-    // implementation based on C libgmp -> mpn_sub_n
-    pub(crate) fn gt(&self, other: &Self) -> Choice {
+    // strategy: check if b-a overflows. if it does not overflow, then a was larger
+    pub(crate) fn gt_direct(&self, other: &Self) -> Choice {
         let mut _ul = 0_u32;
         let mut _vl = 0_u32;
-        let mut _rl = 0_u32;
 
-        let mut cy = 0_u32;
+        // carry through gt
+        let mut c_gt = false;
+        let mut gt_i: bool;
+        let mut eq_i: bool;
+
+        // start from least significant go to most significant
         for i in 0..10 {
             _ul = self.0[i];
             _vl = other.0[i];
 
-            let (_sl, _cy1) = _ul.overflowing_sub(_vl);
-            let (_rl, _cy2) = _sl.overflowing_sub(cy);
-            cy = _cy1 as u32 | _cy2 as u32;
+            gt_i = _ul > _vl;
+            eq_i = _ul == _vl;
+
+            c_gt = gt_i || (eq_i & c_gt);
         }
 
-        Choice::from((cy != 0_u32) as u8)
+        Choice::from(c_gt as u8)
     }
 }

curve25519-dalek/src/backend/serial/u64/field.rs

@@ -574,8 +574,33 @@ impl FieldElement51 {
     }
 
     /// Returns 1 if self is greater than the other and 0 otherwise
-    // implementation based on C libgmp -> mpn_sub_n
-    pub(crate) fn gt(&self, other: &Self) -> Choice {
+    // strategy: check if b-a overflows. if it does not overflow, then a was larger
+    pub(crate) fn gt_direct(&self, other: &Self) -> Choice {
+        let mut _ul = 0_u64;
+        let mut _vl = 0_u64;
+
+        // carry through gt
+        let mut c_gt = false;
+        let mut gt_i: bool;
+        let mut eq_i: bool;
+
+        // start from least significant go to most significant
+        for i in 0..5 {
+            _ul = self.0[i];
+            _vl = other.0[i];
+
+            gt_i = _ul > _vl;
+            eq_i = _ul == _vl;
+
+            c_gt = gt_i || (eq_i & c_gt);
+        }
+
+        Choice::from(c_gt as u8)
+    }
+
+    /// Returns 1 if self is greater than the other and 0 otherwise
+    // strategy: check if b-a overflows. if it does not overflow, then a was larger
+    pub(crate) fn mpn_sub_n(&self, other: &Self) -> Choice {
         let mut _ul = 0_u64;
         let mut _vl = 0_u64;
         let mut _rl = 0_u64;

curve25519-dalek/src/elligator2.rs

@@ -231,6 +231,9 @@ impl MapToPointVariant for Randomized {
 }
 
 #[cfg(feature = "digest")]
+/// In general this mode should **NEVER** be used unless there is a very specific
+/// reason to do so as it has multiple serious known flaws.
+///
 /// Converts between a point on elliptic curve E (Curve25519) and an element of
 /// the finite field F over which E is defined. Supports older implementations
 /// with a common implementation bug (Signal, Kleshni-C).
@@ -244,9 +247,6 @@ impl MapToPointVariant for Randomized {
 /// high-order two bits set. The kleshni C and Signal implementations are examples
 /// of libraries that don't always use the least square root.
 ///
-/// In general this mode should NOT be used unless there is a very specific
-/// reason to do so.
-///
 // We return the LSR for to_representative values. This is here purely for testing
 // compatability and ensuring that we understand the subtle differences that can
 // influence proper implementation.
@@ -266,10 +266,28 @@ impl MapToPointVariant for Legacy {
         CtOption::new(point, Choice::from(1))
     }
 
+    // There is a bug in the kleshni implementation where it
+    // takes a sortcut when computng greater than for field elemements.
+    // For the purpose of making tests pass matching the bugged implementation
+    // I am adding the bug here intentionally. Legacy is not exposed and
+    // should not be exposed as it is obviously flawed in multiple ways.
+    //
+    // What we want is:
+    //      If root - (p - 1) / 2 < 0, root := -root
+    // This is not equivalent to:
+    //      if root > (p - 1)/2 root := -root
+    //
     fn to_representative(point: &[u8; 32], _tweak: u8) -> CtOption<[u8; 32]> {
         let pubkey = EdwardsPoint::mul_base_clamped(*point);
         let v_in_sqrt = v_in_sqrt_pubkey_edwards(&pubkey);
+
         point_to_representative(&MontgomeryPoint(*point), v_in_sqrt.into())
+
+        // let divide_minus_p_1_2 = FieldElement::from_bytes(&DIVIDE_MINUS_P_1_2_BYTES);
+        // let did_negate = divide_minus_p_1_2.ct_gt(&b);
+        // let should_negate = Self::gt(&b, &divide_minus_p_1_2);
+        // FieldElement::conditional_negate(&mut b, did_negate ^ should_negate);
+        // CtOption::new(b.as_bytes(), c)
     }
 }
 
@@ -583,7 +601,8 @@ pub(crate) fn point_to_representative(
     let mut b = FieldElement::conditional_select(&r1, &r0, Choice::from(v_in_sqrt as u8));
 
     // If root > (p - 1) / 2, root := -root
-    let negate = divide_minus_p_1_2.ct_gt(&b);
+    // let negate = divide_minus_p_1_2.ct_gt(&b);
+    let negate = divide_minus_p_1_2.mpn_sub_n(&b);
     FieldElement::conditional_negate(&mut b, negate);
 
     CtOption::new(b.as_bytes(), is_encodable)
@@ -677,7 +696,9 @@ pub(crate) fn v_in_sqrt_pubkey_edwards(pubkey: &EdwardsPoint) -> Choice {
     let v = &(&t0 * &inv1) * &(&pubkey.Z * &sqrt_minus_a_plus_2);
 
     // is   v <= (q-1)/2  ?
-    divide_minus_p_1_2.ct_gt(&v)
+    // divide_minus_p_1_2.ct_gt(&v)
+    // v.is_negative()
+    divide_minus_p_1_2.mpn_sub_n(&v)
 }
 
 // ============================================================================

curve25519-dalek/src/field.rs

@@ -313,13 +313,22 @@ impl ConstantTimeGreater for FieldElement {
     /// If self > other return Choice(1), otherwise return Choice(0)
     ///
     fn ct_gt(&self, other: &FieldElement) -> Choice {
-        self.gt(other)
+        // One possible failure for is if self or other falls in 0..18
+        // as s+p in 2^255-19..2^255-1.  We can check this by
+        // converting to bytes and then back to FieldElement,
+        // since our encoding routine is canonical the returned value
+        // will always be compared properly.
+        let a = FieldElement::from_bytes(&self.as_bytes());
+        let b = FieldElement::from_bytes(&other.as_bytes());
+
+        a.gt_direct(&b)
     }
 }
 
 #[cfg(test)]
 mod test {
     use crate::field::*;
+    use hex::FromHex;
 
     /// Random element a of GF(2^255-19), from Sage
     /// a = 1070314506888354081329385823235218444233221\
@@ -504,4 +513,39 @@ mod test {
     fn batch_invert_empty() {
         FieldElement::batch_invert(&mut []);
     }
+
+    #[test]
+    fn greater_than() {
+        // 2^255 - 1 = 18
+        let low_high_val = <[u8; 32]>::from_hex(
+            "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
+        )
+        .expect("should never fail");
+        // 32
+        let higher_low_val = <[u8; 32]>::from_hex(
+            "0000000000000000000000000000000000000000000000000000000000000020",
+        )
+        .expect("should never fail");
+
+        let cases = [
+            (FieldElement::ONE, FieldElement::ZERO, true),
+            (FieldElement::ZERO, FieldElement::ONE, false),
+            (FieldElement::ONE, FieldElement::ONE, false),
+            (
+                FieldElement::from_bytes(&higher_low_val),
+                FieldElement::from_bytes(&low_high_val),
+                true,
+            ),
+            (
+                FieldElement::from_bytes(&low_high_val),
+                FieldElement::from_bytes(&higher_low_val),
+                false,
+            ),
+        ];
+
+        for (i, (a, b, expected)) in cases.into_iter().enumerate() {
+            let actual: bool = a.ct_gt(&b).into();
+            assert_eq!(expected, actual, "failed case ({i}) {actual}");
+        }
+    }
 }