diff --git a/curve25519-dalek/src/backend/serial/fiat_u32/field.rs b/curve25519-dalek/src/backend/serial/fiat_u32/field.rs index 755c8c60b..d121928ea 100644 --- a/curve25519-dalek/src/backend/serial/fiat_u32/field.rs +++ b/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) } } diff --git a/curve25519-dalek/src/backend/serial/fiat_u64/field.rs b/curve25519-dalek/src/backend/serial/fiat_u64/field.rs index 0e4eedc70..1b28b2bc9 100644 --- a/curve25519-dalek/src/backend/serial/fiat_u64/field.rs +++ b/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) } } diff --git a/curve25519-dalek/src/backend/serial/u32/field.rs b/curve25519-dalek/src/backend/serial/u32/field.rs index a2ffe264b..a7471285d 100644 --- a/curve25519-dalek/src/backend/serial/u32/field.rs +++ b/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) } } diff --git a/curve25519-dalek/src/backend/serial/u64/field.rs b/curve25519-dalek/src/backend/serial/u64/field.rs index 673f5f92b..d16d2bd36 100644 --- a/curve25519-dalek/src/backend/serial/u64/field.rs +++ b/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; diff --git a/curve25519-dalek/src/elligator2.rs b/curve25519-dalek/src/elligator2.rs index f8540fce2..960287eac 100644 --- a/curve25519-dalek/src/elligator2.rs +++ b/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, ÷_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) } // ============================================================================ diff --git a/curve25519-dalek/src/field.rs b/curve25519-dalek/src/field.rs index 0a4a428bd..4b99ff536 100644 --- a/curve25519-dalek/src/field.rs +++ b/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}"); + } + } }