diff --git a/k256/src/arithmetic/affine.rs b/k256/src/arithmetic/affine.rs index fc5a4b2e..bd0ca171 100644 --- a/k256/src/arithmetic/affine.rs +++ b/k256/src/arithmetic/affine.rs @@ -151,6 +151,7 @@ impl DecompressPoint 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, diff --git a/k256/src/arithmetic/field.rs b/k256/src/arithmetic/field.rs index c2ebc87e..a1298909 100644 --- a/k256/src/arithmetic/field.rs +++ b/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); diff --git a/k256/src/arithmetic/field/field_impl.rs b/k256/src/arithmetic/field/field_impl.rs index e502d379..f64b3f8f 100644 --- a/k256/src/arithmetic/field/field_impl.rs +++ b/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 { diff --git a/k256/src/ecdsa/sign.rs b/k256/src/ecdsa/sign.rs index e1d3dd92..46a7be49 100644 --- a/k256/src/ecdsa/sign.rs +++ b/k256/src/ecdsa/sign.rs @@ -183,7 +183,7 @@ impl SignPrimitive 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);