Repository: facebook/opaque-ke (language: rust)

Issue:
P256's `is_square` bug
This is just checking if `result.number` is one twice and might leak timing. Assuming it's not optimized out which I feel like it would be.

https://github.com/novifinancial/opaque-ke/blob/d1dfee9a6545d7dfddc6dd94f985f31b3221c54e/src/group/p256.rs#L308-L320

Line 319 should be something like this:
```
result.number.is_zero() || result.number.is_one()
```
But this also leaks the timing of whether `self.number` is zero which really doesn't matter too much. Note testing `is_one` first will leak the timing of whether `self.number` is a non-zero square which is way worse. The correct fix is a function that tests for zero or one. It would work by converting it to bytes then masking out the lowest bit and checking if it's zero. Unless you have a constant time less than compare function. Also that `is_zero` function is no longer used.


Discussion / hints:

Fix patch:
diff --git a/src/group/p256.rs b/src/group/p256.rs
index 7399d16c..a358fa5d 100644
--- a/src/group/p256.rs
+++ b/src/group/p256.rs
@@ -17,7 +17,7 @@ use generic_array::typenum::{U32, U33};
 use generic_array::{ArrayLength, GenericArray};
 use num_bigint::{BigInt, Sign};
 use num_integer::Integer;
-use num_traits::{One, ToPrimitive};
+use num_traits::{One, ToPrimitive, Zero};
 use once_cell::unsync::Lazy;
 use p256_::elliptic_curve::group::prime::PrimeCurveAffine;
 use p256_::elliptic_curve::group::GroupEncoding;
@@ -306,7 +306,7 @@ fn map_to_curve_simple_swu<N: ArrayLength<u8>>(
         }
 
         fn is_zero(&self) -> bool {
-            self.number.is_one()
+            self.number.is_zero()
         }
 
         /// Corresponds to the is_square() function defined in
@@ -316,7 +316,7 @@ fn map_to_curve_simple_swu<N: ArrayLength<u8>>(
             let exponent = (self.f.0 - 1) >> 1;
 
             let result = self.pow_internal(&exponent);
-            result.number.is_one() || result.is_zero()
+            result.is_zero() || result.number.is_one()
         }
 
         fn to_bytes<N: ArrayLength<u8>>(&self) -> GenericArray<u8, N> {


Test patch: