diff --git a/frost-ed25519/Cargo.toml b/frost-ed25519/Cargo.toml index e8c63e15..636badfc 100644 --- a/frost-ed25519/Cargo.toml +++ b/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" diff --git a/frost-ed25519/src/lib.rs b/frost-ed25519/src/lib.rs index 28b47f50..b76e0718 100644 --- a/frost-ed25519/src/lib.rs +++ b/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) diff --git a/frost-ed25519/tests/frost.rs b/frost-ed25519/tests/frost.rs index d6e397d6..d57b9873 100644 --- a/frost-ed25519/tests/frost.rs +++ b/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 = <::Group as Group>::deserialize(&encoded_identity); + let r = ::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 = ::Group::deserialize(&encoded_point); + assert_eq!(r, Err(GroupError::InvalidNonPrimeOrderElement)); +} diff --git a/frost-ed448/Cargo.toml b/frost-ed448/Cargo.toml index 9e282ebe..aea64f42 100644 --- a/frost-ed448/Cargo.toml +++ b/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" diff --git a/frost-ed448/src/lib.rs b/frost-ed448/src/lib.rs index f18298a4..306d281a 100644 --- a/frost-ed448/src/lib.rs +++ b/frost-ed448/src/lib.rs @@ -94,12 +94,19 @@ impl Group for Ed448Group { } fn deserialize(buf: &Self::Serialization) -> Result { - 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) } diff --git a/frost-ed448/tests/frost.rs b/frost-ed448/tests/frost.rs index 22baf8d3..5420ca29 100644 --- a/frost-ed448/tests/frost.rs +++ b/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 = <::Group as Group>::deserialize(&encoded_identity); + let r = ::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 = ::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 = ::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 = ::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 = ::Group::deserialize(&encoded_point); + assert_eq!(r, Err(GroupError::InvalidNonPrimeOrderElement)); +} diff --git a/frost-p256/Cargo.toml b/frost-p256/Cargo.toml index a62f6ff3..2dc907df 100644 --- a/frost-p256/Cargo.toml +++ b/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" diff --git a/frost-p256/tests/frost.rs b/frost-p256/tests/frost.rs index 6472946c..6bda8897 100644 --- a/frost-p256/tests/frost.rs +++ b/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 = <::Group as Group>::deserialize(&encoded_identity); + let r = ::Group::deserialize(&encoded_identity); + assert_eq!(r, Err(GroupError::MalformedElement)); +} + +#[test] +fn check_deserialize_non_canonical() { + let mut encoded_generator = ::Group::serialize( + &::Group::generator(), + ); + + let r = ::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 = ::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 = ::Group::deserialize(&encoded_point); assert_eq!(r, Err(GroupError::MalformedElement)); } diff --git a/frost-ristretto255/tests/frost.rs b/frost-ristretto255/tests/frost.rs index 4f57d027..c20f266c 100644 --- a/frost-ristretto255/tests/frost.rs +++ b/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 = <::Group as Group>::deserialize(&encoded_identity); + let r = ::Group::deserialize(&encoded_identity); assert_eq!(r, Err(GroupError::InvalidIdentityElement)); } diff --git a/frost-secp256k1/Cargo.toml b/frost-secp256k1/Cargo.toml index 5bb98a45..35338e5c 100644 --- a/frost-secp256k1/Cargo.toml +++ b/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" diff --git a/frost-secp256k1/tests/frost.rs b/frost-secp256k1/tests/frost.rs index 8fc88f06..67ca00e2 100644 --- a/frost-secp256k1/tests/frost.rs +++ b/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 = <::Group as Group>::deserialize(&encoded_identity); + let r = ::Group::deserialize(&encoded_identity); + assert_eq!(r, Err(GroupError::MalformedElement)); +} + +#[test] +fn check_deserialize_non_canonical() { + let mut encoded_generator = ::Group::serialize( + &::Group::generator(), + ); + + let r = ::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 = ::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 = ::Group::deserialize(&encoded_point); assert_eq!(r, Err(GroupError::MalformedElement)); }