From f27e59e4a5f0fc25e8c462b69d85b9faa9de5e81 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Tue, 10 Nov 2026 15:33:29 -0700 Subject: [PATCH] ssh-key: allow ECDSA keys shorter than the modulus size Allows private scalars as small as 192-bits (following `elliptic_curve::SecretKey::MIN_SIZE`, padding them with zeros if they're smaller than the modulus size. Also includes regression tests. Closes #620 --- ssh-key/src/private/ecdsa.rs | 46 ++++-- ssh-key/tests/dot_ssh.rs | 2 +- ssh-key/tests/ecdsa_private_key.rs | 149 ++++++++++++++++++ ssh-key/tests/examples/id_ecdsa_p256_short | 9 ++ .../tests/examples/id_ecdsa_p256_short.enc | 9 ++ 5 files changed, 200 insertions(+), 15 deletions(-) create mode 100644 ssh-key/tests/ecdsa_private_key.rs create mode 100644 ssh-key/tests/examples/id_ecdsa_p256_short create mode 100644 ssh-key/tests/examples/id_ecdsa_p256_short.enc diff --git a/ssh-key/src/private/ecdsa.rs b/ssh-key/src/private/ecdsa.rs index 563168bb..f60aed16 100644 --- a/ssh-key/src/private/ecdsa.rs +++ b/ssh-key/src/private/ecdsa.rs @@ -24,6 +24,10 @@ pub struct EcdsaPrivateKey { } impl EcdsaPrivateKey { + /// 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 Decode for EcdsaPrivateKey { fn decode(reader: &mut Decoder) -> Result { 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 Decode for EcdsaPrivateKey { 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 }) }) } diff --git a/ssh-key/tests/dot_ssh.rs b/ssh-key/tests/dot_ssh.rs index 9e517aab..8ed3ce08 100644 --- a/ssh-key/tests/dot_ssh.rs +++ b/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] diff --git a/ssh-key/tests/ecdsa_private_key.rs b/ssh-key/tests/ecdsa_private_key.rs new file mode 100644 index 00000000..57158700 --- /dev/null +++ b/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(bytes: &[u8]) -> ssh_key::Result> { + 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 { + 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() { + // 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::(&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::::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::(&bytes), + Err(ssh_key::Error::FormatEncoding) + ); + let padded = decode_scalar::(&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::(&redundant).unwrap(), padded); + redundant.insert(0, 0); + assert_eq!( + decode_scalar::(&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::(&encode_positive(&bytes)).is_err()); + let mut padded = vec![0; SIZE + 1 - len]; + padded.extend_from_slice(&bytes); + assert_eq!( + decode_scalar::(&padded), + Err(encoding::Error::Length.into()) + ); + } + + assert_eq!( + decode_scalar::(&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>(); +} diff --git a/ssh-key/tests/examples/id_ecdsa_p256_short b/ssh-key/tests/examples/id_ecdsa_p256_short new file mode 100644 index 00000000..dc443a7f --- /dev/null +++ b/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----- diff --git a/ssh-key/tests/examples/id_ecdsa_p256_short.enc b/ssh-key/tests/examples/id_ecdsa_p256_short.enc new file mode 100644 index 00000000..4601bec4 --- /dev/null +++ b/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-----