add check for canonical point encodings where needed, and tests (#193)

* add check for canonical point encodings where needed, and tests

* remove unneeded 'as' keywords

* fix after syncing with main

* pin curve25519-dalek for now due to breaking changes

Co-authored-by: Deirdre Connolly <durumcrustulum@gmail.com>

frost-ed25519/Cargo.toml

@@ -33,6 +33,7 @@ bincode = "1"
 criterion = "0.4"
 ed25519-dalek = "1.0.1"
 ed25519-zebra = "3.0.0"
+hex = "0.4.3"
 lazy_static = "1.4"
 proptest = "1.0"
 proptest-derive = "0.3"

frost-ed25519/src/lib.rs

@@ -55,6 +55,15 @@ impl Group for Ed25519Group {
                 if point == Self::identity() {
                     Err(GroupError::InvalidIdentityElement)
                 } else if point.is_torsion_free() {
+                    // At this point we should reject points which were not
+                    // encoded canonically (i.e. Y coordinate >= p).
+                    // However, we don't allow non-prime order elements,
+                    // and that suffices to also reject non-canonical encodings
+                    // per https://eprint.iacr.org/2020/1244.pdf:
+                    //
+                    // > There are 19 elliptic curve points that can be encoded in a non-canonical form.
+                    // > (...) Among these points there are 2 points of small order and from the
+                    // > remaining 17 y-coordinates only 10 decode to valid curve points all of mixed order.
                     Ok(point)
                 } else {
                     Err(GroupError::InvalidNonPrimeOrderElement)

frost-ed25519/tests/frost.rs

@@ -35,6 +35,17 @@ fn check_bad_batch_verify() {
 fn check_deserialize_identity() {
     let encoded_identity = EdwardsPoint::identity().compress().to_bytes();
 
-    let r = <<Ed25519Sha512 as Ciphersuite>::Group as Group>::deserialize(&encoded_identity);
+    let r = <Ed25519Sha512 as Ciphersuite>::Group::deserialize(&encoded_identity);
     assert_eq!(r, Err(GroupError::InvalidIdentityElement));
 }
+
+#[test]
+fn check_deserialize_non_prime_order() {
+    let encoded_point =
+        hex::decode("0300000000000000000000000000000000000000000000000000000000000000")
+            .unwrap()
+            .try_into()
+            .unwrap();
+    let r = <Ed25519Sha512 as Ciphersuite>::Group::deserialize(&encoded_point);
+    assert_eq!(r, Err(GroupError::InvalidNonPrimeOrderElement));
+}

frost-ed448/Cargo.toml

@@ -32,6 +32,7 @@ sha3 = "0.10.6"
 bincode = "1"
 criterion = "0.4"
 lazy_static = "1.4"
+hex = "0.4.3"
 proptest = "1.0"
 proptest-derive = "0.3"
 rand = "0.8"

frost-ed448/src/lib.rs

@@ -94,12 +94,19 @@ impl Group for Ed448Group {
     }
 
     fn deserialize(buf: &Self::Serialization) -> Result<Self::Element, GroupError> {
-        match CompressedEdwardsY(*buf).decompress() {
+        let compressed = CompressedEdwardsY(*buf);
+        match compressed.decompress() {
             Some(point) => {
                 if point == Self::identity() {
                     Err(GroupError::InvalidIdentityElement)
                 } else if point.is_torsion_free() {
-                    Ok(point)
+                    // decompress() does not check for canonicality, so we
+                    // check by recompressing and comparing
+                    if point.compress().0 != compressed.0 {
+                        Err(GroupError::MalformedElement)
+                    } else {
+                        Ok(point)
+                    }
                 } else {
                     Err(GroupError::InvalidNonPrimeOrderElement)
                 }

frost-ed448/tests/frost.rs

@@ -39,6 +39,37 @@ fn check_bad_batch_verify() {
 fn check_deserialize_identity() {
     let encoded_identity = ExtendedPoint::identity().compress().0;
 
-    let r = <<Ed448Shake256 as Ciphersuite>::Group as Group>::deserialize(&encoded_identity);
+    let r = <Ed448Shake256 as Ciphersuite>::Group::deserialize(&encoded_identity);
     assert_eq!(r, Err(GroupError::InvalidIdentityElement));
 }
+
+#[test]
+fn check_deserialize_non_canonical() {
+    let mut encoded_generator = ExtendedPoint::generator().compress().0;
+
+    let r = <Ed448Shake256 as Ciphersuite>::Group::deserialize(&encoded_generator);
+    assert!(r.is_ok());
+
+    // The last byte only should have the sign bit. Set all other bits to
+    // create a non-canonical encoding.
+    encoded_generator[56] |= 0x7f;
+    let r = <Ed448Shake256 as Ciphersuite>::Group::deserialize(&encoded_generator);
+    assert_eq!(r, Err(GroupError::MalformedElement));
+
+    // Besides the last byte, it is still possible to get non-canonical encodings.
+    // This is y = p + 19 which is non-canonical and maps to a valid prime-order point.
+    let encoded_point = hex::decode("12000000000000000000000000000000000000000000000000000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffff00").unwrap().try_into().unwrap();
+    let r = <Ed448Shake256 as Ciphersuite>::Group::deserialize(&encoded_point);
+    assert_eq!(r, Err(GroupError::MalformedElement));
+}
+
+#[test]
+fn check_deserialize_non_prime_order() {
+    let encoded_point =
+        hex::decode("030000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000")
+            .unwrap()
+            .try_into()
+            .unwrap();
+    let r = <Ed448Shake256 as Ciphersuite>::Group::deserialize(&encoded_point);
+    assert_eq!(r, Err(GroupError::InvalidNonPrimeOrderElement));
+}

frost-p256/Cargo.toml

@@ -32,6 +32,7 @@ bincode = "1"
 criterion = "0.4"
 ed25519-dalek = "1.0.1"
 ed25519-zebra = "3.0.0"
+hex = "0.4.3"
 lazy_static = "1.4"
 proptest = "1.0"
 proptest-derive = "0.3"

frost-p256/tests/frost.rs

@@ -36,6 +36,32 @@ fn check_deserialize_identity() {
     // allow us to change that. Try to send something similar.
     let encoded_identity = [0u8; 33];
 
-    let r = <<P256Sha256 as Ciphersuite>::Group as Group>::deserialize(&encoded_identity);
+    let r = <P256Sha256 as Ciphersuite>::Group::deserialize(&encoded_identity);
+    assert_eq!(r, Err(GroupError::MalformedElement));
+}
+
+#[test]
+fn check_deserialize_non_canonical() {
+    let mut encoded_generator = <P256Sha256 as Ciphersuite>::Group::serialize(
+        &<P256Sha256 as Ciphersuite>::Group::generator(),
+    );
+
+    let r = <P256Sha256 as Ciphersuite>::Group::deserialize(&encoded_generator);
+    assert!(r.is_ok());
+
+    // The first byte should be 0x02 or 0x03. Set other value to
+    // create a non-canonical encoding.
+    encoded_generator[0] = 0xFF;
+    let r = <P256Sha256 as Ciphersuite>::Group::deserialize(&encoded_generator);
+    assert_eq!(r, Err(GroupError::MalformedElement));
+
+    // Besides the first byte, it is still possible to get non-canonical encodings.
+    // This is x = p + 5 which is non-canonical and maps to a valid prime-order point.
+    let encoded_point =
+        hex::decode("02ffffffff00000001000000000000000000000001000000000000000000000004")
+            .unwrap()
+            .try_into()
+            .unwrap();
+    let r = <P256Sha256 as Ciphersuite>::Group::deserialize(&encoded_point);
     assert_eq!(r, Err(GroupError::MalformedElement));
 }

frost-ristretto255/tests/frost.rs

@@ -35,6 +35,6 @@ fn check_bad_batch_verify() {
 fn check_deserialize_identity() {
     let encoded_identity = RistrettoPoint::identity().compress().to_bytes();
 
-    let r = <<Ristretto255Sha512 as Ciphersuite>::Group as Group>::deserialize(&encoded_identity);
+    let r = <Ristretto255Sha512 as Ciphersuite>::Group::deserialize(&encoded_identity);
     assert_eq!(r, Err(GroupError::InvalidIdentityElement));
 }

frost-secp256k1/Cargo.toml

@@ -32,6 +32,7 @@ bincode = "1"
 criterion = "0.4"
 ed25519-dalek = "1.0.1"
 ed25519-zebra = "3.0.0"
+hex = "0.4.3"
 lazy_static = "1.4"
 proptest = "1.0"
 proptest-derive = "0.3"

frost-secp256k1/tests/frost.rs

@@ -36,6 +36,32 @@ fn check_deserialize_identity() {
     // allow us to change that. Try to send something similar.
     let encoded_identity = [0u8; 33];
 
-    let r = <<Secp256K1Sha256 as Ciphersuite>::Group as Group>::deserialize(&encoded_identity);
+    let r = <Secp256K1Sha256 as Ciphersuite>::Group::deserialize(&encoded_identity);
+    assert_eq!(r, Err(GroupError::MalformedElement));
+}
+
+#[test]
+fn check_deserialize_non_canonical() {
+    let mut encoded_generator = <Secp256K1Sha256 as Ciphersuite>::Group::serialize(
+        &<Secp256K1Sha256 as Ciphersuite>::Group::generator(),
+    );
+
+    let r = <Secp256K1Sha256 as Ciphersuite>::Group::deserialize(&encoded_generator);
+    assert!(r.is_ok());
+
+    // The first byte should be 0x02 or 0x03. Set other value to
+    // create a non-canonical encoding.
+    encoded_generator[0] = 0xFF;
+    let r = <Secp256K1Sha256 as Ciphersuite>::Group::deserialize(&encoded_generator);
+    assert_eq!(r, Err(GroupError::MalformedElement));
+
+    // Besides the first byte, it is still possible to get non-canonical encodings.
+    // This is x = p + 2 which is non-canonical and maps to a valid prime-order point.
+    let encoded_point =
+        hex::decode("02fffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc31")
+            .unwrap()
+            .try_into()
+            .unwrap();
+    let r = <Secp256K1Sha256 as Ciphersuite>::Group::deserialize(&encoded_point);
     assert_eq!(r, Err(GroupError::MalformedElement));
 }