From 2c6c66a828826957998d62192b48e86a02eb1167 Mon Sep 17 00:00:00 2001 From: Matt Palmer Date: Wed, 24 Apr 2024 14:12:52 +1000 Subject: [PATCH] Avoid zero-length decodes via base64ct Adjust to the changes to `base64ct` that reject zero-length decode() calls as invalid. The underlying trigger for this change was to avoid problems with decoding certain OpenSSH private keys. If a "PEM-like" OpenSSH format private key: 1. has no comment; 2. is of a length such that there is no padding bytes at the end of the "Unencrypted list of N private keys"; ***and*** 3. is of a length such that the base64-encoded form of the key does not require any `=` bytes for padding; then `ssh_key::private::PrivateKey::from_openssh` failed to successfully parse the key. The root cause was `base64ct` (prior to RustCrypto/formats#1387) incorrectly rejecting zero-length reads, *only* when the base64-encoded buffer was empty. This only happened for reading empty comments, because comments are at the end of the key. It *wouldn't* happen if: 1. the key had a comment, because that wouldn't be a zero-length read; 2. there were padding bytes in the "Unencrypted list of N private keys", because those padding bytes would still be in the buffer, so the buffer wasn't empty; *or* 3. there were base64 `=` padding characters, because those would cause the decoder to believe a read would succeed (even though it wouldn't if the read was longer than zero bytes). Debugging this was... an amusement. The most reliable way to demonstrate this was with an ECDSA P-256 key without a comment (easily generated with `ssh-keygen -t ecdsa -b 256 -C '' -N '' -f `), because they're *usually* (but not *always*) an appropriate length to trigger the bug. Amusingly (cough), the existing ECDSA P-256 test key in `ssh-key/tests/examples/id_ecdsa_p256` is one of the outliers, as its `d` is 33 bytes, rather than 32, which means that key (when stripped of its comment) *doesn't* trigger the bug. --- ssh-encoding/src/reader.rs | 4 ++++ ssh-key/tests/examples/padless_wonder | 8 ++++++++ ssh-key/tests/private_key.rs | 23 +++++++++++++++++++++++ 3 files changed, 35 insertions(+) create mode 100644 ssh-key/tests/examples/padless_wonder diff --git a/ssh-encoding/src/reader.rs b/ssh-encoding/src/reader.rs index f669b25..a88450b 100644 --- a/ssh-encoding/src/reader.rs +++ b/ssh-encoding/src/reader.rs @@ -178,6 +178,10 @@ pub struct NestedReader<'r, R: Reader> { impl<'r, R: Reader> Reader for NestedReader<'r, R> { fn read<'o>(&mut self, out: &'o mut [u8]) -> Result<&'o [u8]> { + if out.is_empty() { + return Ok(out); + } + let remaining_len = self .remaining_len .checked_sub(out.len()) diff --git a/ssh-key/tests/examples/padless_wonder b/ssh-key/tests/examples/padless_wonder new file mode 100644 index 0000000..c351b4f --- /dev/null +++ b/ssh-key/tests/examples/padless_wonder @@ -0,0 +1,8 @@ +-----BEGIN OPENSSH PRIVATE KEY----- +b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAaAAAABNlY2RzYS +1zaGEyLW5pc3RwMjU2AAAACG5pc3RwMjU2AAAAQQRx3l5o/ZI7bNGXguxVI/VmDd/SIwUo +nlZHbHSmwBSeHPT7RisjBbiXnS829RrZ2o+Ix34GFtLN7z+SBHViPRVuAAAAmOCDDMLggw +zCAAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBHHeXmj9kjts0ZeC +7FUj9WYN39IjBSieVkdsdKbAFJ4c9PtGKyMFuJedLzb1Gtnaj4jHfgYW0s3vP5IEdWI9FW +4AAAAgNEF96jnfIuhkq4ECZqNPe98Fv1SFb5evUQAq3/MtkKEAAAAA +-----END OPENSSH PRIVATE KEY----- diff --git a/ssh-key/tests/private_key.rs b/ssh-key/tests/private_key.rs index 9e43137..15bafe8 100644 --- a/ssh-key/tests/private_key.rs +++ b/ssh-key/tests/private_key.rs @@ -46,6 +46,11 @@ const OPENSSH_RSA_4096_EXAMPLE: &str = include_str!("examples/id_rsa_4096"); #[cfg(feature = "alloc")] const OPENSSH_OPAQUE_EXAMPLE: &str = include_str!("examples/id_opaque"); +/// OpenSSH-formatted private key with no internal or external padding, and no comment +/// Trips a corner case in base64ct +#[cfg(feature = "ecdsa")] +const OPENSSH_PADLESS_WONDER_EXAMPLE: &str = include_str!("examples/padless_wonder"); + /// Get a path into the `tests/scratch` directory. #[cfg(feature = "std")] pub fn scratch_path(filename: &str) -> PathBuf { @@ -129,6 +134,24 @@ fn decode_ecdsa_p256_openssh() { assert_eq!("user@example.com", key.comment()); } +#[cfg(feature = "ecdsa")] +#[test] +fn decode_padless_wonder_openssh() { + let key = PrivateKey::from_openssh(OPENSSH_PADLESS_WONDER_EXAMPLE).unwrap(); + assert_eq!( + Algorithm::Ecdsa { + curve: EcdsaCurve::NistP256 + }, + 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!("", key.comment()); +} + #[cfg(feature = "ecdsa")] #[test] fn decode_ecdsa_p384_openssh() {