ssh-key: allow ECDSA keys shorter than the modulus size

ssh-key/src/private/ecdsa.rs

@@ -24,6 +24,10 @@ pub struct EcdsaPrivateKey<const SIZE: usize> {
 }
 
 impl<const SIZE: usize> EcdsaPrivateKey<SIZE> {
+    /// Minimum allowed size of a private scalar for ECDSA: 192-bits.
+    // NOTE: this matches `elliptic_curve::SecretKey::MIN_SIZE` while avoiding a hard dependency
+    pub const MIN_SIZE: usize = 24;
+
     /// Borrow the inner byte array as a slice.
     #[must_use]
     pub fn as_slice(&self) -> &[u8] {
@@ -47,22 +51,31 @@ impl<const SIZE: usize> Decode for EcdsaPrivateKey<SIZE> {
 
     fn decode(reader: &mut Decoder<impl Reader>) -> Result<Self> {
         reader.read_prefixed(|reader| {
-            let mut len = reader.remaining_len();
-
-            // Strip leading zero if necessary:
-            // `mpint` is signed and may need a leading zero for unsigned integers
-            if len == SIZE.checked_add(1).ok_or(encoding::Error::Length)? {
-                // TODO(tarcieri): make sure leading zero was necessary
-                if u8::decode(reader)? != 0 {
-                    return Err(Error::FormatEncoding);
-                }
+            // Bound the wire length before stripping any leading zeros.
+            match reader.remaining_len().checked_sub(1) {
+                Some(len) if len <= SIZE => (),
+                _ => return Err(encoding::Error::Length.into()),
+            }
+
+            let mut first = u8::decode(reader)?;
+            if first >= 0x80 {
+                return Err(Error::FormatEncoding);
+            }
 
-                len = SIZE;
+            // Accept redundant leading zeros
+            while first == 0 {
+                if reader.is_finished() {
+                    return Err(encoding::Error::Length.into());
+                }
+                first = u8::decode(reader)?;
             }
 
-            // Minimum allowed key size: may be smaller than modulus size
-            const MIN_SIZE: usize = 32;
-            if len < MIN_SIZE || len > SIZE {
+            let len = reader
+                .remaining_len()
+                .checked_add(1)
+                .ok_or(encoding::Error::Length)?;
+
+            if len < Self::MIN_SIZE || len > SIZE {
                 return Err(encoding::Error::Length.into());
             }
 
@@ -71,7 +84,12 @@ impl<const SIZE: usize> Decode for EcdsaPrivateKey<SIZE> {
             let leading_zeros = SIZE.checked_sub(len).ok_or(encoding::Error::Length)?;
 
             let mut bytes = [0u8; SIZE];
-            reader.read(&mut bytes[leading_zeros..])?;
+            let (first_byte, remaining) = bytes[leading_zeros..]
+                .split_first_mut()
+                .ok_or(encoding::Error::Length)?;
+
+            *first_byte = first;
+            reader.read(remaining)?;
             Ok(Self { bytes })
         })
     }

ssh-key/tests/dot_ssh.rs

@@ -20,7 +20,7 @@ fn path_round_trip() {
 #[test]
 fn private_keys() {
     let dot_ssh = dot_ssh();
-    assert_eq!(dot_ssh.private_keys().unwrap().count(), 22);
+    assert_eq!(dot_ssh.private_keys().unwrap().count(), 24);
 }
 
 #[test]

ssh-key/tests/ecdsa_private_key.rs

@@ -0,0 +1,149 @@
+//! ECDSA scalar parsing and validation tests.
+
+#![cfg(feature = "ecdsa")]
+#![allow(clippy::unwrap_used, reason = "tests")]
+
+use encoding::{Base64Reader, Base64Writer, Decode, Encode};
+use hex_literal::hex;
+use ssh_key::{PrivateKey, private::EcdsaPrivateKey};
+
+// Throwaway OpenSSH key from https://github.com/RustCrypto/SSH/issues/620.
+const SHORT_P256_KEY: &str = include_str!("examples/id_ecdsa_p256_short");
+
+#[test]
+fn decode_short_p256_openssh() {
+    let key = PrivateKey::from_openssh(SHORT_P256_KEY).unwrap();
+    let ssh_key::private::EcdsaKeypair::NistP256 { private, .. } = key.key_data().ecdsa().unwrap()
+    else {
+        panic!("unexpected ECDSA curve");
+    };
+    assert_eq!(
+        private.as_slice(),
+        &hex!("0069aaed302f8421543fffa7b5eecb6f17a2bf72ab2df53b548aeab9e42d9106")
+    );
+
+    #[cfg(feature = "alloc")]
+    {
+        let encoded = key.to_openssh(ssh_key::LineEnding::LF).unwrap();
+        assert_eq!(PrivateKey::from_openssh(encoded.as_str()).unwrap(), key);
+    }
+}
+
+#[cfg(feature = "encryption")]
+#[test]
+fn encrypt_decrypt_short_p256() {
+    let key = PrivateKey::from_openssh(SHORT_P256_KEY).unwrap();
+    let openssh_encrypted =
+        PrivateKey::from_openssh(include_str!("examples/id_ecdsa_p256_short.enc")).unwrap();
+    assert_eq!(openssh_encrypted.decrypt(b"password").unwrap(), key);
+
+    let encrypted = key.encrypt(&mut getrandom::SysRng, b"password").unwrap();
+    let encoded = encrypted.to_openssh(ssh_key::LineEnding::LF).unwrap();
+    let decoded = PrivateKey::from_openssh(encoded.as_str()).unwrap();
+    assert_eq!(decoded.decrypt(b"password").unwrap(), key);
+}
+
+#[cfg(all(feature = "alloc", feature = "p256"))]
+#[test]
+fn sign_with_short_p256() {
+    let key = PrivateKey::from_openssh(SHORT_P256_KEY).unwrap();
+    let signature = key
+        .sign("test", ssh_key::HashAlg::Sha256, b"message")
+        .unwrap();
+    key.public_key()
+        .verify("test", b"message", &signature)
+        .unwrap();
+}
+
+/// Decode a length-prefixed scalar without requiring `ssh-key`'s `alloc` feature.
+fn decode_scalar<const SIZE: usize>(bytes: &[u8]) -> ssh_key::Result<EcdsaPrivateKey<SIZE>> {
+    let mut encoded = u32::try_from(bytes.len()).unwrap().to_be_bytes().to_vec();
+    encoded.extend_from_slice(bytes);
+    EcdsaPrivateKey::decode(&mut encoded.as_slice().into())
+}
+
+fn encode_positive(bytes: &[u8]) -> Vec<u8> {
+    let mut encoded = Vec::new();
+    if bytes.first().is_some_and(|byte| *byte >= 0x80) {
+        encoded.push(0);
+    }
+    encoded.extend_from_slice(bytes);
+    encoded
+}
+
+fn check_scalar_encodings<const SIZE: usize>() {
+    // Cover every accepted length, including the 24-byte floor and 31-byte P-256 keys.
+    for len in 24..=SIZE {
+        let mut bytes = vec![0x42; len];
+        bytes[0] = 1;
+        let private = decode_scalar::<SIZE>(&bytes).unwrap();
+        assert_eq!(&private.as_slice()[SIZE - len..], bytes);
+        assert!(
+            private.as_slice()[..SIZE - len]
+                .iter()
+                .all(|byte| *byte == 0)
+        );
+
+        // Keep accepting fixed-width encodings, including redundant leading zeros.
+        let mut encoded = [0u8; 96];
+        let mut writer = Base64Writer::new(&mut encoded).unwrap();
+        private.encode(&mut writer).unwrap();
+        let encoded = writer.finish().unwrap();
+        let mut reader = encoding::Decoder::from(Base64Reader::new(encoded.as_bytes()).unwrap());
+        assert_eq!(private.encoded_len().unwrap(), reader.remaining_len());
+        let decoded = EcdsaPrivateKey::<SIZE>::decode(&mut reader).unwrap();
+        assert_eq!(private, reader.finish(decoded).unwrap());
+
+        // A set high bit requires a leading sign byte, regardless of magnitude length.
+        bytes[0] = 0x80;
+        assert_eq!(
+            decode_scalar::<SIZE>(&bytes),
+            Err(ssh_key::Error::FormatEncoding)
+        );
+        let padded = decode_scalar::<SIZE>(&encode_positive(&bytes)).unwrap();
+        assert_eq!(&padded.as_slice()[SIZE - len..], bytes);
+
+        // Additional leading zeros are accepted within the wire-length bound.
+        let mut redundant = vec![0; SIZE + 1 - len];
+        redundant.extend_from_slice(&bytes);
+        assert_eq!(decode_scalar::<SIZE>(&redundant).unwrap(), padded);
+        redundant.insert(0, 0);
+        assert_eq!(
+            decode_scalar::<SIZE>(&redundant),
+            Err(encoding::Error::Length.into())
+        );
+    }
+
+    // Zero and magnitudes below 24 bytes are rejected, even with enough padding
+    // to fill the whole scalar or an additional sign byte.
+    for len in 0..24 {
+        let bytes = vec![0x80; len];
+        assert!(decode_scalar::<SIZE>(&encode_positive(&bytes)).is_err());
+        let mut padded = vec![0; SIZE + 1 - len];
+        padded.extend_from_slice(&bytes);
+        assert_eq!(
+            decode_scalar::<SIZE>(&padded),
+            Err(encoding::Error::Length.into())
+        );
+    }
+
+    assert_eq!(
+        decode_scalar::<SIZE>(&vec![1; SIZE + 1]),
+        Err(encoding::Error::Length.into())
+    );
+}
+
+#[test]
+fn p256_scalar_encodings() {
+    check_scalar_encodings::<32>();
+}
+
+#[test]
+fn p384_scalar_encodings() {
+    check_scalar_encodings::<48>();
+}
+
+#[test]
+fn p521_scalar_encodings() {
+    check_scalar_encodings::<66>();
+}

ssh-key/tests/examples/id_ecdsa_p256_short

@@ -0,0 +1,9 @@
+-----BEGIN OPENSSH PRIVATE KEY-----
+b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAaAAAABNlY2RzYS
+1zaGEyLW5pc3RwMjU2AAAACG5pc3RwMjU2AAAAQQSE3hgowl7Stx5bXtynenFwiU2ltfsH
+2hs5E7ZCmTnDf6MuGBrSnoFSDmk/nmGGuafKJJjLpJtUmjDiU8Pb65CIAAAAsCogP8MqID
+/DAAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBITeGCjCXtK3Hlte
+3Kd6cXCJTaW1+wfaGzkTtkKZOcN/oy4YGtKegVIOaT+eYYa5p8okmMukm1SaMOJTw9vrkI
+gAAAAfaartMC+EIVQ//6e17stvF6K/cqst9TtUiuq55C2RBgAAABhvcGVud2FyZC10ZXN0
+LXAyNTYtc2hvcnQB
+-----END OPENSSH PRIVATE KEY-----

ssh-key/tests/examples/id_ecdsa_p256_short.enc

@@ -0,0 +1,9 @@
+-----BEGIN OPENSSH PRIVATE KEY-----
+b3BlbnNzaC1rZXktdjEAAAAACmFlczI1Ni1jdHIAAAAGYmNyeXB0AAAAGAAAABCj3lUT68
+c45aBcupOBifWLAAAAGAAAAAEAAABoAAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlz
+dHAyNTYAAABBBITeGCjCXtK3Hlte3Kd6cXCJTaW1+wfaGzkTtkKZOcN/oy4YGtKegVIOaT
++eYYa5p8okmMukm1SaMOJTw9vrkIgAAACwgT4N1S4rlzWWfoNUZx8eh7SZDFQM9ewPqXXY
+vcSOlQ/PadCyLtyKjO00m6K+nRDvbhw1J5h3gtrGP+qYbpEJ7ce8CGXIrknNa9K75LPGUZ
+yLO7qoHG+gB4GoiKvyN/oJvLXOCW8YwD0f+zV2FFLz3tFJZ4rkRhwWOs5a6IpQsi/S5DpE
+xbqa9oxkD120j/l9UGHZhoe6BuzTCk6FL246MtQP2I62BG4xchWvc9YD/KY=
+-----END OPENSSH PRIVATE KEY-----