From 125343b66cbb419566a59bae7d49951ea026c2ef Mon Sep 17 00:00:00 2001 From: jmwample <8297368+jmwample@users.noreply.github.com> Date: Sat, 27 Jul 2024 21:46:44 -0600 Subject: [PATCH 1/3] seemingly working greater than. I believe in constant time --- .../src/backend/serial/fiat_u32/field.rs | 25 ++++++----- .../src/backend/serial/fiat_u64/field.rs | 26 ++++++----- .../src/backend/serial/u32/field.rs | 25 ++++++----- .../src/backend/serial/u64/field.rs | 25 ++++++----- curve25519-dalek/src/field.rs | 45 ++++++++++++++++++- 5 files changed, 105 insertions(+), 41 deletions(-) diff --git a/curve25519-dalek/src/backend/serial/fiat_u32/field.rs b/curve25519-dalek/src/backend/serial/fiat_u32/field.rs index 755c8c60..7b95add6 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]; + _ul = self.0[i]; + _vl = other.0[i]; + + gt_i = _ul > _vl; + eq_i = _ul == _vl; - let (_sl, _cy1) = _ul.overflowing_sub(_vl); - let (_rl, _cy2) = _sl.overflowing_sub(cy); - cy = _cy1 as u32 | _cy2 as u32; + 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 0e4eedc7..d1fee439 100644 --- a/curve25519-dalek/src/backend/serial/fiat_u64/field.rs +++ b/curve25519-dalek/src/backend/serial/fiat_u64/field.rs @@ -260,23 +260,29 @@ impl FieldElement51 { output } + /// 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]; + _ul = self.0[i]; + _vl = other.0[i]; + + gt_i = _ul > _vl; + eq_i = _ul == _vl; - let (_sl, _cy1) = _ul.overflowing_sub(_vl); - let (_rl, _cy2) = _sl.overflowing_sub(cy); - cy = _cy1 as u64 | _cy2 as u64; + 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 a2ffe264..7ebe4cf2 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]; + _ul = self.0[i]; + _vl = other.0[i]; + + gt_i = _ul > _vl; + eq_i = _ul == _vl; - let (_sl, _cy1) = _ul.overflowing_sub(_vl); - let (_rl, _cy2) = _sl.overflowing_sub(cy); - cy = _cy1 as u32 | _cy2 as u32; + 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 673f5f92..cde51842 100644 --- a/curve25519-dalek/src/backend/serial/u64/field.rs +++ b/curve25519-dalek/src/backend/serial/u64/field.rs @@ -574,22 +574,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]; + _ul = self.0[i]; + _vl = other.0[i]; + + gt_i = _ul > _vl; + eq_i = _ul == _vl; - let (_sl, _cy1) = _ul.overflowing_sub(_vl); - let (_rl, _cy2) = _sl.overflowing_sub(cy); - cy = _cy1 as u64 | _cy2 as u64; + 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/field.rs b/curve25519-dalek/src/field.rs index 0a4a428b..b95c77e2 100644 --- a/curve25519-dalek/src/field.rs +++ b/curve25519-dalek/src/field.rs @@ -313,13 +313,23 @@ 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 +514,37 @@ 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}"); + } + } } From a3c2ba0a0d1d24a7f4aa43a6fb388e053165b8a1 Mon Sep 17 00:00:00 2001 From: jmwample <8297368+jmwample@users.noreply.github.com> Date: Sat, 27 Jul 2024 21:54:33 -0600 Subject: [PATCH 2/3] fmt --- curve25519-dalek/src/backend/serial/fiat_u32/field.rs | 10 +++++----- curve25519-dalek/src/backend/serial/fiat_u64/field.rs | 11 +++++------ curve25519-dalek/src/backend/serial/u32/field.rs | 10 +++++----- curve25519-dalek/src/backend/serial/u64/field.rs | 10 +++++----- curve25519-dalek/src/field.rs | 11 ++++++----- 5 files changed, 26 insertions(+), 26 deletions(-) diff --git a/curve25519-dalek/src/backend/serial/fiat_u32/field.rs b/curve25519-dalek/src/backend/serial/fiat_u32/field.rs index 7b95add6..d121928e 100644 --- a/curve25519-dalek/src/backend/serial/fiat_u32/field.rs +++ b/curve25519-dalek/src/backend/serial/fiat_u32/field.rs @@ -282,13 +282,13 @@ impl FieldElement2625 { // start from least significant go to most significant for i in 0..10 { - _ul = self.0[i]; - _vl = other.0[i]; + _ul = self.0[i]; + _vl = other.0[i]; - gt_i = _ul > _vl; - eq_i = _ul == _vl; + gt_i = _ul > _vl; + eq_i = _ul == _vl; - c_gt = gt_i || (eq_i & c_gt); + c_gt = gt_i || (eq_i & c_gt); } 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 d1fee439..1b28b2bc 100644 --- a/curve25519-dalek/src/backend/serial/fiat_u64/field.rs +++ b/curve25519-dalek/src/backend/serial/fiat_u64/field.rs @@ -260,7 +260,6 @@ impl FieldElement51 { output } - /// 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 gt_direct(&self, other: &Self) -> Choice { @@ -274,13 +273,13 @@ impl FieldElement51 { // start from least significant go to most significant for i in 0..5 { - _ul = self.0[i]; - _vl = other.0[i]; + _ul = self.0[i]; + _vl = other.0[i]; - gt_i = _ul > _vl; - eq_i = _ul == _vl; + gt_i = _ul > _vl; + eq_i = _ul == _vl; - c_gt = gt_i || (eq_i & c_gt); + c_gt = gt_i || (eq_i & c_gt); } 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 7ebe4cf2..a7471285 100644 --- a/curve25519-dalek/src/backend/serial/u32/field.rs +++ b/curve25519-dalek/src/backend/serial/u32/field.rs @@ -615,13 +615,13 @@ impl FieldElement2625 { // start from least significant go to most significant for i in 0..10 { - _ul = self.0[i]; - _vl = other.0[i]; + _ul = self.0[i]; + _vl = other.0[i]; - gt_i = _ul > _vl; - eq_i = _ul == _vl; + gt_i = _ul > _vl; + eq_i = _ul == _vl; - c_gt = gt_i || (eq_i & c_gt); + c_gt = gt_i || (eq_i & c_gt); } 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 cde51842..ada14204 100644 --- a/curve25519-dalek/src/backend/serial/u64/field.rs +++ b/curve25519-dalek/src/backend/serial/u64/field.rs @@ -586,13 +586,13 @@ impl FieldElement51 { // start from least significant go to most significant for i in 0..5 { - _ul = self.0[i]; - _vl = other.0[i]; + _ul = self.0[i]; + _vl = other.0[i]; - gt_i = _ul > _vl; - eq_i = _ul == _vl; + gt_i = _ul > _vl; + eq_i = _ul == _vl; - c_gt = gt_i || (eq_i & c_gt); + c_gt = gt_i || (eq_i & c_gt); } Choice::from(c_gt as u8) diff --git a/curve25519-dalek/src/field.rs b/curve25519-dalek/src/field.rs index b95c77e2..4b99ff53 100644 --- a/curve25519-dalek/src/field.rs +++ b/curve25519-dalek/src/field.rs @@ -313,10 +313,9 @@ impl ConstantTimeGreater for FieldElement { /// If self > other return Choice(1), otherwise return Choice(0) /// fn ct_gt(&self, other: &FieldElement) -> Choice { - - // One possible failure for is if self.or other falls in 0..18 + // 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, + // 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()); @@ -520,11 +519,13 @@ mod test { // 2^255 - 1 = 18 let low_high_val = <[u8; 32]>::from_hex( "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", - ).expect("should never fail"); + ) + .expect("should never fail"); // 32 let higher_low_val = <[u8; 32]>::from_hex( "0000000000000000000000000000000000000000000000000000000000000020", - ).expect("should never fail"); + ) + .expect("should never fail"); let cases = [ (FieldElement::ONE, FieldElement::ZERO, true), From 10f829abf65e6c1e1b1849c39ac122a9a9e521c4 Mon Sep 17 00:00:00 2001 From: jmwample <8297368+jmwample@users.noreply.github.com> Date: Tue, 30 Jul 2024 22:51:03 -0600 Subject: [PATCH 3/3] solved the mystery of ct_gt, but at what cost [skip ci] --- .../src/backend/serial/u64/field.rs | 20 ++++++++++++ curve25519-dalek/src/elligator2.rs | 31 ++++++++++++++++--- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/curve25519-dalek/src/backend/serial/u64/field.rs b/curve25519-dalek/src/backend/serial/u64/field.rs index ada14204..d16d2bd3 100644 --- a/curve25519-dalek/src/backend/serial/u64/field.rs +++ b/curve25519-dalek/src/backend/serial/u64/field.rs @@ -597,4 +597,24 @@ impl FieldElement51 { 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; + + let mut cy = 0_u64; + 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; + } + + Choice::from((cy != 0_u64) as u8) + } } diff --git a/curve25519-dalek/src/elligator2.rs b/curve25519-dalek/src/elligator2.rs index f8540fce..960287ea 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) } // ============================================================================