Skip to content

Commit

Permalink
Test private key parsing for commentless edge case
Browse files Browse the repository at this point in the history
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` fails to successfully parse the key.

The root cause is `base64ct` (prior to RustCrypto/formats#1387) incorrectly rejecting zero-length reads when the base64-encoded buffer was empty.
This 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 is with an ECDSA P-256 key without a comment (easily generated with `ssh-keygen -t ecdsa -b 256 -C '' -N '' -f <file>`),
because they *usually* (but not *always*) are of an appropriate length to trigger the bug.
Amusingly (cough), the existing ECDSA P-256 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.
  • Loading branch information
mpalmer committed Apr 22, 2024
1 parent d9179b8 commit 45083f0
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 2 deletions.
3 changes: 1 addition & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ opt-level = 2
# When you remove the last patch.crates-io entry, please re-enable `minimal-versions`
# in `.github/workflows/ssh-key.yml` and `.github/workflows/ssh-cipher.yml`

base64ct = { git = "https://github.com/mpalmer/rust-crypto-formats.git", branch = "b64ct-decode-empty-at-end" }

p256 = { git = "https://github.com/RustCrypto/elliptic-curves.git" }
p384 = { git = "https://github.com/RustCrypto/elliptic-curves.git" }
Expand Down
8 changes: 8 additions & 0 deletions ssh-key/tests/examples/padless_wonder
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
-----BEGIN OPENSSH PRIVATE KEY-----
b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAaAAAABNlY2RzYS
1zaGEyLW5pc3RwMjU2AAAACG5pc3RwMjU2AAAAQQRx3l5o/ZI7bNGXguxVI/VmDd/SIwUo
nlZHbHSmwBSeHPT7RisjBbiXnS829RrZ2o+Ix34GFtLN7z+SBHViPRVuAAAAmOCDDMLggw
zCAAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBHHeXmj9kjts0ZeC
7FUj9WYN39IjBSieVkdsdKbAFJ4c9PtGKyMFuJedLzb1Gtnaj4jHfgYW0s3vP5IEdWI9FW
4AAAAgNEF96jnfIuhkq4ECZqNPe98Fv1SFb5evUQAq3/MtkKEAAAAA
-----END OPENSSH PRIVATE KEY-----
22 changes: 22 additions & 0 deletions ssh-key/tests/private_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ 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
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 {
Expand Down Expand Up @@ -129,6 +133,24 @@ fn decode_ecdsa_p256_openssh() {
assert_eq!("[email protected]", 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() {
Expand Down

0 comments on commit 45083f0

Please sign in to comment.