k256: Do not normalize the argument in FieldElementImpl::is_odd() (#…

…530)

- `FieldElementImpl::is_odd()` now checks for normalization instead of performing it
- `AffinePoint::decompress()` normalizes `y` coordinate (`beta` in the code) before checking if it is odd
- `Scalar::try_sign_prehashed` normalizes `R.y` before checking if it is odd

k256/src/arithmetic/affine.rs

@@ -151,6 +151,7 @@ impl DecompressPoint<Secp256k1> for AffinePoint {
             let beta = alpha.sqrt();
 
             beta.map(|beta| {
+                let beta = beta.normalize(); // Need to normalize for is_odd() to be consistent
                 let y = FieldElement::conditional_select(
                     &beta.negate(1),
                     &beta,

k256/src/arithmetic/field.rs

@@ -542,6 +542,33 @@ mod tests {
         assert_eq!(four.sqrt().unwrap().normalize(), two.normalize());
     }
 
+    #[test]
+    #[cfg_attr(
+        debug_assertions,
+        should_panic(expected = "assertion failed: self.normalized")
+    )]
+    fn unnormalized_is_odd() {
+        // This is a regression test for https://github.com/RustCrypto/elliptic-curves/issues/529
+        // where `is_odd()` in debug mode force-normalized its argument
+        // instead of checking that it is already normalized.
+        // As a result, in release (where normalization didn't happen) `is_odd()`
+        // could return an incorrect value.
+
+        let x = FieldElement::from_bytes_unchecked(&[
+            61, 128, 156, 189, 241, 12, 174, 4, 80, 52, 238, 78, 188, 251, 9, 188, 95, 115, 38, 6,
+            212, 168, 175, 174, 211, 232, 208, 14, 182, 45, 59, 122,
+        ]);
+        // Produces an unnormalized FieldElement with magnitude 1
+        // (we cannot create one directly).
+        let y = x.sqrt().unwrap();
+
+        // This is fine.
+        assert!(y.normalize().is_odd().unwrap_u8() == 0);
+
+        // This panics since `y` is not normalized.
+        let _result = y.is_odd().unwrap_u8();
+    }
+
     prop_compose! {
         fn field_element()(bytes in any::<[u8; 32]>()) -> FieldElement {
             let mut res = bytes_to_biguint(&bytes);

k256/src/arithmetic/field/field_impl.rs

@@ -89,7 +89,8 @@ impl FieldElementImpl {
     }
 
     pub fn is_odd(&self) -> Choice {
-        self.normalize().value.is_odd()
+        debug_assert!(self.normalized);
+        self.value.is_odd()
     }
 
     pub fn negate(&self, magnitude: u32) -> Self {

k256/src/ecdsa/sign.rs

@@ -183,7 +183,7 @@ impl SignPrimitive<Secp256k1> for Scalar {
         }
 
         let signature = Signature::from_scalars(r, s)?;
-        let is_r_odd: bool = R.y.is_odd().into();
+        let is_r_odd: bool = R.y.normalize().is_odd().into();
         let is_s_high: bool = signature.s().is_high().into();
         let signature_low = signature.normalize_s().unwrap_or(signature);
         let recovery_id = ecdsa_core::RecoveryId::new(is_r_odd ^ is_s_high, false);