fixed #316 - correctly parse OpenSSH keys generated by PuTTYgen (#321)

Author: Eugeny

ssh-key/src/private.rs

@@ -176,7 +176,7 @@ const DEFAULT_RSA_KEY_SIZE: usize = 4096;
 const MAX_BLOCK_SIZE: usize = 16;
 
 /// Padding bytes to use.
-const PADDING_BYTES: [u8; MAX_BLOCK_SIZE - 1] = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15];
+const PADDING_BYTES: [u8; MAX_BLOCK_SIZE] = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16];
 
 /// Unix file permissions for SSH private keys.
 #[cfg(all(unix, feature = "std"))]
@@ -377,10 +377,12 @@ impl PrivateKey {
         let mut buffer = Zeroizing::new(ciphertext.to_vec());
         self.cipher.decrypt(&key, &iv, &mut buffer, self.auth_tag)?;
 
+        #[allow(clippy::arithmetic_side_effects)] // block sizes are constants
         Self::decode_privatekey_comment_pair(
             &mut &**buffer,
             self.public_key.key_data.clone(),
             self.cipher.block_size(),
+            self.cipher.block_size() - 1,
         )
     }
 
@@ -571,8 +573,10 @@ impl PrivateKey {
         reader: &mut impl Reader,
         public_key: public::KeyData,
         block_size: usize,
+        max_padding_size: usize,
     ) -> Result<Self> {
         debug_assert!(block_size <= MAX_BLOCK_SIZE);
+        debug_assert!(max_padding_size <= MAX_BLOCK_SIZE);
 
         // Ensure input data is padding-aligned
         if reader.remaining_len().checked_rem(block_size) != Some(0) {
@@ -598,7 +602,7 @@ impl PrivateKey {
 
         let padding_len = reader.remaining_len();
 
-        if padding_len >= block_size {
+        if padding_len > max_padding_size {
             return Err(encoding::Error::Length.into());
         }
 
@@ -756,7 +760,25 @@ impl Decode for PrivateKey {
         }
 
         reader.read_prefixed(|reader| {
-            Self::decode_privatekey_comment_pair(reader, public_key, cipher.block_size())
+            // PuTTYgen uses a non-standard block size of 16
+            // and _always_ adds a padding even if data length
+            // is divisible by 16 - for unencrypted keys
+            // in the OpenSSH format.
+            // We're only relaxing the exact length check, but will
+            // still validate that the contents of the padding area.
+            // In all other cases there can be up to (but not including)
+            // `block_size` padding bytes as per `PROTOCOL.key`.
+            let max_padding_size = match cipher {
+                Cipher::None => 16,
+                #[allow(clippy::arithmetic_side_effects)] // block sizes are constants
+                _ => cipher.block_size() - 1,
+            };
+            Self::decode_privatekey_comment_pair(
+                reader,
+                public_key,
+                cipher.block_size(),
+                max_padding_size,
+            )
         })
     }
 }

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(), 20);
+    assert_eq!(dot_ssh.private_keys().unwrap().count(), 21);
 }
 
 #[test]

ssh-key/tests/examples/puttygen_overpadded

@@ -0,0 +1,8 @@
+-----BEGIN OPENSSH PRIVATE KEY-----
+b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtz
+c2gtZWQyNTUxOQAAACA+af4QkC9p+OQHgC8EQ1xT+4Ykkf0SYPmEF85tb57WMwAA
+ALBRB2JGUQdiRgAAAAtzc2gtZWQyNTUxOQAAACA+af4QkC9p+OQHgC8EQ1xT+4Yk
+kf0SYPmEF85tb57WMwAAAEBGxdSjfrbFQ17/N6WcP1EmN6ymf3qRR3NGSGh6zCtm
+JD5p/hCQL2n45AeALwRDXFP7hiSR/RJg+YQXzm1vntYzAAAAHWVkZHNhLWtleS0y
+MDI0MTIyN2ExMjM0NTY3ODkwAQIDBAUGBwgJCgsMDQ4PEA==
+-----END OPENSSH PRIVATE KEY-----

ssh-key/tests/private_key.rs

@@ -94,6 +94,11 @@ const OPENSSH_OPAQUE_EXAMPLE: &str = include_str!("examples/id_opaque");
 #[cfg(feature = "p384")]
 const OPENSSH_PADLESS_WONDER_EXAMPLE: &str = include_str!("examples/padless_wonder");
 
+/// OpenSSH-formatted private key generated by PuTTYgen that showcases its
+/// incorrect 16-byte "block size"
+#[cfg(feature = "ed25519")]
+const OPENSSH_OVERPADDED_PUTTYGEN_EXAMPLE: &str = include_str!("examples/puttygen_overpadded");
+
 /// Get a path into the `tests/scratch` directory.
 #[cfg(feature = "std")]
 pub fn scratch_path(filename: &str) -> PathBuf {
@@ -229,6 +234,19 @@ fn decode_padless_wonder_openssh() {
     assert_eq!("", key.comment());
 }
 
+#[cfg(feature = "ed25519")]
+#[test]
+fn decode_overpadded_puttygen_openssh() {
+    let key = PrivateKey::from_openssh(OPENSSH_OVERPADDED_PUTTYGEN_EXAMPLE).unwrap();
+    assert_eq!(Algorithm::Ed25519, key.algorithm());
+    assert_eq!(Cipher::None, key.cipher());
+    assert_eq!(KdfAlg::None, key.kdf().algorithm());
+    assert!(key.kdf().is_none());
+
+    #[cfg(feature = "alloc")]
+    assert_eq!("eddsa-key-20241227a1234567890", key.comment());
+}
+
 #[cfg(feature = "p384")]
 #[test]
 fn decode_ecdsa_p384_openssh() {