From 1e2aaeb0846b4bd93443fe1f07cb83aaa5686266 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Sat, 21 Jan 2023 08:25:34 -0700 Subject: [PATCH 1/2] ed25519: infallible signature parsing We've had a couple request to make signature parsing infallible. Presently it checks that the `s` scalar component of the signature has its three highest bits clear, which will reject some but not all signatures where `s` overflows the curve's order. However, this check will not reject *all* such invalid signatures (at least as presently implemented), and the same check will be performed at the time the signature is verified (which *will* reject all such invalid signatures). Though we already shipped `ed25519` v2.0.0 and this is a breaking change, that release has been yanked so we can get this change in last minute. --- ed25519/src/lib.rs | 59 ++++++++++++++++++++++---------------------- ed25519/src/serde.rs | 18 ++++++-------- ed25519/tests/hex.rs | 12 +++------ 3 files changed, 39 insertions(+), 50 deletions(-) diff --git a/ed25519/src/lib.rs b/ed25519/src/lib.rs index c6fe10bf..b7f23fd4 100644 --- a/ed25519/src/lib.rs +++ b/ed25519/src/lib.rs @@ -301,30 +301,31 @@ impl Signature { pub const BYTE_SIZE: usize = COMPONENT_SIZE * 2; /// Parse an Ed25519 signature from a byte slice. - pub fn from_bytes(bytes: &SignatureBytes) -> signature::Result { - let (R, s) = bytes.split_at(COMPONENT_SIZE); + pub fn from_bytes(bytes: &SignatureBytes) -> Self { + let mut R = ComponentBytes::default(); + let mut s = ComponentBytes::default(); - Self::from_components( - R.try_into().map_err(|_| Error::new())?, - s.try_into().map_err(|_| Error::new())?, - ) + let components = bytes.split_at(COMPONENT_SIZE); + R.copy_from_slice(components.0); + s.copy_from_slice(components.1); + + Self { R, s } } /// Parse an Ed25519 signature from its `R` and `s` components. - pub fn from_components(R: ComponentBytes, s: ComponentBytes) -> signature::Result { - // Perform a partial reduction check on the signature's `s` scalar. - // When properly reduced, at least the three highest bits of the scalar - // will be unset so as to fit within the order of ~2^(252.5). - // - // This doesn't ensure that `s` is fully reduced (which would require a - // full reduction check in the event that the 4th most significant bit - // is set), however it will catch a number of invalid signatures - // relatively inexpensively. - if s[COMPONENT_SIZE - 1] & 0b1110_0000 != 0 { - return Err(Error::new()); - } - - Ok(Self { R, s }) + pub fn from_components(R: ComponentBytes, s: ComponentBytes) -> Self { + Self { R, s } + } + + /// Parse an Ed25519 signature from a byte slice. + /// + /// # Returns + /// - `Ok` on success + /// - `Err` if the input byte slice is not 64-bytes + pub fn from_slice(bytes: &[u8]) -> signature::Result { + SignatureBytes::try_from(bytes) + .map(Into::into) + .map_err(|_| Error::new()) } /// Bytes for the `R` component of a signature. @@ -373,18 +374,14 @@ impl From<&Signature> for SignatureBytes { } } -impl TryFrom for Signature { - type Error = Error; - - fn try_from(bytes: SignatureBytes) -> signature::Result { - Signature::try_from(&bytes) +impl From for Signature { + fn from(bytes: SignatureBytes) -> Self { + Signature::from_bytes(&bytes) } } -impl TryFrom<&SignatureBytes> for Signature { - type Error = Error; - - fn try_from(bytes: &SignatureBytes) -> signature::Result { +impl From<&SignatureBytes> for Signature { + fn from(bytes: &SignatureBytes) -> Self { Signature::from_bytes(bytes) } } @@ -393,7 +390,9 @@ impl TryFrom<&[u8]> for Signature { type Error = Error; fn try_from(bytes: &[u8]) -> signature::Result { - Self::from_bytes(bytes.try_into().map_err(|_| Error::new())?) + SignatureBytes::try_from(bytes) + .map(Into::into) + .map_err(|_| Error::new()) } } diff --git a/ed25519/src/serde.rs b/ed25519/src/serde.rs index 2dbdc3f6..79783f77 100644 --- a/ed25519/src/serde.rs +++ b/ed25519/src/serde.rs @@ -51,8 +51,9 @@ impl<'de> Deserialize<'de> for Signature { } } - let bytes = deserializer.deserialize_tuple(Signature::BYTE_SIZE, ByteArrayVisitor)?; - Self::from_bytes(&bytes).map_err(de::Error::custom) + deserializer + .deserialize_tuple(Signature::BYTE_SIZE, ByteArrayVisitor) + .map(Into::into) } } @@ -93,8 +94,9 @@ impl<'de> serde_bytes::Deserialize<'de> for Signature { } } - let bytes = deserializer.deserialize_bytes(ByteArrayVisitor)?; - Self::from_bytes(&bytes).map_err(de::Error::custom) + deserializer + .deserialize_bytes(ByteArrayVisitor) + .map(Into::into) } } @@ -114,15 +116,9 @@ mod tests { #[test] fn round_trip() { - let signature = Signature::from_bytes(&SIGNATURE_BYTES).unwrap(); + let signature = Signature::from_bytes(&SIGNATURE_BYTES); let serialized = bincode::serialize(&signature).unwrap(); let deserialized = bincode::deserialize(&serialized).unwrap(); assert_eq!(signature, deserialized); } - - #[test] - fn overflow() { - let bytes = hex!("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"); - assert!(bincode::deserialize::(&bytes).is_err()); - } } diff --git a/ed25519/tests/hex.rs b/ed25519/tests/hex.rs index 96edc1bc..99214343 100644 --- a/ed25519/tests/hex.rs +++ b/ed25519/tests/hex.rs @@ -15,19 +15,19 @@ const TEST_1_SIGNATURE: [u8; Signature::BYTE_SIZE] = hex!( #[test] fn display() { - let sig = Signature::from_bytes(&TEST_1_SIGNATURE).unwrap(); + let sig = Signature::from_bytes(&TEST_1_SIGNATURE); assert_eq!(sig.to_string(), "E5564300C360AC729086E2CC806E828A84877F1EB8E5D974D873E065224901555FB8821590A33BACC61E39701CF9B46BD25BF5F0595BBE24655141438E7A100B") } #[test] fn lower_hex() { - let sig = Signature::from_bytes(&TEST_1_SIGNATURE).unwrap(); + let sig = Signature::from_bytes(&TEST_1_SIGNATURE); assert_eq!(format!("{:x}", sig), "e5564300c360ac729086e2cc806e828a84877f1eb8e5d974d873e065224901555fb8821590a33bacc61e39701cf9b46bd25bf5f0595bbe24655141438e7a100b") } #[test] fn upper_hex() { - let sig = Signature::from_bytes(&TEST_1_SIGNATURE).unwrap(); + let sig = Signature::from_bytes(&TEST_1_SIGNATURE); assert_eq!(format!("{:X}", sig), "E5564300C360AC729086E2CC806E828A84877F1EB8E5D974D873E065224901555FB8821590A33BACC61E39701CF9B46BD25BF5F0595BBE24655141438E7A100B") } @@ -48,9 +48,3 @@ fn from_str_rejects_mixed_case() { let result = Signature::from_str("E5564300c360ac729086e2cc806e828a84877f1eb8e5d974d873e065224901555fb8821590a33bacc61e39701cf9b46bd25bf5f0595bbe24655141438e7a100b"); assert!(result.is_err()); } - -#[test] -fn from_str_rejects_invalid_signature() { - let result = Signature::from_str("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"); - assert!(result.is_err()); -} From a5512a075cfac4bca8ec89d8e5c668270d4e5b1c Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Sat, 21 Jan 2023 09:37:09 -0700 Subject: [PATCH 2/2] Add comment about signature not verifying R and s components --- ed25519/src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ed25519/src/lib.rs b/ed25519/src/lib.rs index b7f23fd4..4ac1e6fe 100644 --- a/ed25519/src/lib.rs +++ b/ed25519/src/lib.rs @@ -289,6 +289,13 @@ pub type ComponentBytes = [u8; COMPONENT_SIZE]; pub type SignatureBytes = [u8; Signature::BYTE_SIZE]; /// Ed25519 signature. +/// +/// This type represents a container for the byte serialization of an Ed25519 +/// signature, and does not necessarily represent well-formed elements of the +/// respective elliptic curve fields. +/// +/// Signature verification libraries are expected to reject invalid field +/// elements at the time a signature is verified. #[derive(Copy, Clone, Eq, PartialEq)] #[repr(C)] pub struct Signature {