diff --git a/Cargo.lock b/Cargo.lock index 1647ae45..73049883 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1370,6 +1370,7 @@ dependencies = [ "rand_core", "serde_json", "time", + "zeroize", ] [[package]] diff --git a/ffi/Cargo.toml b/ffi/Cargo.toml index 4c7c2501..a3360054 100644 --- a/ffi/Cargo.toml +++ b/ffi/Cargo.toml @@ -31,6 +31,7 @@ diplomat-runtime = { git = "https://github.com/Devolutions/diplomat", tag = "0.1 time = "0.3" hex = "0.4" serde_json = "1" +zeroize = "1.8" # WASM support [target.'cfg(target_arch = "wasm32")'.dependencies] diff --git a/ffi/dotnet/Devolutions.Picky.Tests/PuttyTests.cs b/ffi/dotnet/Devolutions.Picky.Tests/PuttyTests.cs new file mode 100644 index 00000000..d83b5ae9 --- /dev/null +++ b/ffi/dotnet/Devolutions.Picky.Tests/PuttyTests.cs @@ -0,0 +1,31 @@ +using Xunit; + +namespace Devolutions.Picky.Tests; + +public class PuttyTests +{ + [Fact] + public void EncryptedPpkRoundTrips() + { + string publicKey; + string encryptedRepr; + using (PuttyPpk ppk = PuttyPpk.GenerateEd25519("test@picky.com")) + using (PuttyPpkEncryptionConfig config = PuttyPpkEncryptionConfig.Default()) + using (PuttyPpk encrypted = ppk.Encrypt("hunter2", config)) + using (PuttyPublicKey ppkPublicKey = ppk.ExtractPuttyPublicKey()) + { + Assert.True(encrypted.IsEncrypted()); + publicKey = ppkPublicKey.ToRepr(); + encryptedRepr = encrypted.ToRepr(); + } + + using PuttyPpk parsed = PuttyPpk.Parse(encryptedRepr); + Assert.Throws(() => parsed.Decrypt("wrong")); + + using PuttyPpk decrypted = parsed.Decrypt("hunter2"); + using PuttyPublicKey decryptedPublicKey = decrypted.ExtractPuttyPublicKey(); + Assert.False(decrypted.IsEncrypted()); + Assert.Contains("Encryption: none", decrypted.ToRepr()); + Assert.Equal(publicKey, decryptedPublicKey.ToRepr()); + } +} diff --git a/ffi/dotnet/Devolutions.Picky.Tests/SshTests.cs b/ffi/dotnet/Devolutions.Picky.Tests/SshTests.cs index 8db8353f..62b7d57c 100644 --- a/ffi/dotnet/Devolutions.Picky.Tests/SshTests.cs +++ b/ffi/dotnet/Devolutions.Picky.Tests/SshTests.cs @@ -69,6 +69,23 @@ public void PrivateKeyParse() Assert.Equal("none", key.CipherName); } + [Fact] + public void PassphraseProtectedKeyRoundTrips() + { + string repr; + using (SshPrivateKey key = SshPrivateKey.GenerateEd25519("hunter2", "test@picky.com")) + { + Assert.NotEqual("none", key.CipherName); + repr = key.ToRepr(); + } + + using Pem pem = Pem.Parse(repr); + Assert.Throws(() => SshPrivateKey.FromPem(pem, "wrong")); + + using SshPrivateKey parsed = SshPrivateKey.FromPem(pem, "hunter2"); + Assert.Equal("test@picky.com", parsed.Comment); + } + private static readonly string hostCertStrRepr = "ssh-rsa-cert-v01@openssh.com AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgxrum49LfnPQE9T+xcClCKuEzSrwNh3M5P6f4uwda6CsAAAADAQABAAACAQCxxwZypEyoP3lq2HfeGiyO7fenoj1txaF4UodcPMMRAyatme6BRy3gobY59IStkhN/oA1QZPVb+uOBpgepZgNPDOMrsODgU0ZxbbYwH/cdGWRoXMYlRZhw1y4KJB5ZVg+pRwrkeNpgP5yrAYuAzjg3GGovEHRDhNGuvANgje/Mr+Ye/YGASUaUaXouPMn4BxoVHM5h7SpWQSXWvy7pszsYAMadGmSnik9Xilrio3I0Z4I51vyxkePwZhKrLUW7tlJES/r3Ezurjz1FW2CniivWtTHDsuM6hLeFPdLZ/Y7yeRpUwmS+21SH/abaxqKvU5dQr1rFs2anXBnPgH2RGXS7a3TznZe0BBccy2uRrvta4eN1pjIL7Olxe8yuea1rygjAn+wb6BFLekYu/GvIPzpf+bw9yVtE51eIkQy5QyqBNJTdRXdKSU5bm8Z4XZcgX5osDG+dpL2SewgLlrxXrAsrSjAeycLKwO+VOUFLMmFO040ZjuAs4Sbw8ptkePdCveU1BFHpWyvf/WG/BmdUzrSwjjVOJT2kguBLiOiH8YAOncCFMLDcHBfd5hFU6jQ5U7CU8HM2wYV8uq1kXtXqmfJ4QJV1D9he8MOJ+u3G4KZR0uNREe5gX7WjvQGT3kql5c8LanDb3rY0Auj9pJd639f7XGN+UYGROuycqvB7BvgQ1wAAAAAAAAAAAAAAAgAAAAVwaWNreQAAACsAAAARZmlyc3QuZXhhbXBsZS5jb20AAAASc2Vjb25kLmV4YW1wbGUuY29tAAAAAGFlVGwAAAAAY0U22QAAAAAAAAAAAAAAAAAAAhcAAAAHc3NoLXJzYQAAAAMBAAEAAAIBAMwDtw6lA1R20MaWSHCB/23LYMQvKjiXv2mh3YjsHZZYj9mzoeWmhOF4jjDTB2r6//BuwPIyq+We4AQqbZladmXo1CVPZqtgCa2zCMRfWukj+OvluglSFqgc4fpFyEvbC1o7HA+OGzCcWS7fg2VKNyWnXuVxvPNJhgCo+fzXf3CQyWJ9rO5H6QGKaTtczW7IlZ7WfA1KP/NtCg57QWQzghH2hxTHK+DQN6uGzdIMmddJBklJXkialS+FhSJuWNKAkeN/gwfQ7qgItDUG9hRYvOO7aQbf1u/UQpXtV9jH+KAZrDlRS4/DdSta6G9bHjPfX/sqJYchIdbjLwPvu07Q2Gu6BRVj5qiKxH5VJ1eoHuw6PyV/EJP0nseUK8bspcxZ2ooIxmXbetpBdv5r4Piztw4CPZAap1ZXUhivc8hR/1Q5DhXAHKjtZVQ6nUTqALB27b6lkCUoaOgN/BW//O9Yh/g1uW8le8pzO7y8KsQL1pO9DkutJYQh9dEhVJvYkAHeQVWLTKOIUgGCzaVwh6i9VgwdVgibgqrJPxqJPhA1AEk2Wl+390cU/BfqyDM7/S0ezNoBKSY9dtAOBFE5uBd8PwwdhhnQKbHl+FVyco2A5ncN9bkpQgPlF1Cp+Pi/xQUyrJ3oOxuIszmN7Mhg+b2DiDygqbQ0U/IPpa3AY8QlMnL3AAACFAAAAAxyc2Etc2hhMi01MTIAAAIAaUKPXTKkIouWmHjfhSqV97D3Sh/airfktqVeZTAwjvVkwDcNSswJROfNr8r1Y3RlcFzGI/iFFBjfdoq4kdhMyh+wQs12lkqywj+S96Um9ox846OZwVa43eGuI+aH8D1jUiaFiLJG6+NK0yj4y/i+fHQpS9xveF1T+MsxCnhZ8AMLp0dkokfM1QowXpHHoTJeyg5g2GngxWYZcKogLYo/bVNcL5OoWQwrPDLQeJ+Oumv6HxNb1EOR6QpdQBvrw4mnpfyR1Z8pMNCACFHPCKimvEhfV5xlTtp6N1GH2rDyT8L1iuluMBMBVYmS9MLt2xbY4MJSf2wpvjgyQhhlOlMWjC1/dmaIri+V2qozG5S8Z/Yc0hgigJ8YQl747j7KDA6fSSYzSNogt7x1DLE8Vg6eSHEw05QDPZwBDh7sV+9MKgsZZX0Yb/dXGMEAttDs63YmLL2IqIRFgcJLlsD3fkNxnZvgkppKSw2KVic5PpONwD3DgvRyneVKLUICbh/WhOev90J+UKU/vyHEjrNX4XcJ9uhTc14sWxS5JyRRU48MjrLLQYK1ods6aAIqmOGc6YW3Q4pZFDuwO0dFpNnJPlzeytOObVSk+9ybFF45tJdViU1H7i832o4ifVFVV+jicLB8uy4ov6XG1h4kCeaUzIil90yosg9+qmBzDktkqbocPKc= sasha@kubuntu\r\n"; [Fact] diff --git a/ffi/src/pem.rs b/ffi/src/pem.rs index 4f2f2ecb..1588bc75 100644 --- a/ffi/src/pem.rs +++ b/ffi/src/pem.rs @@ -96,3 +96,13 @@ pub unsafe extern "C" fn Pem_peek_data(pem: Option<&ffi::Pem>, len: *mut usize) core::ptr::null() } } + +// A PEM can carry a private key, so wipe the decoded bytes before they're freed. +impl Drop for ffi::Pem { + fn drop(&mut self) { + let pem = core::mem::replace(&mut self.0, picky::pem::Pem::new(String::new(), Vec::new())); + if let std::borrow::Cow::Owned(mut data) = pem.into_data() { + zeroize::Zeroize::zeroize(&mut data); + } + } +} diff --git a/ffi/src/putty.rs b/ffi/src/putty.rs index 668cfd02..ec7bb266 100644 --- a/ffi/src/putty.rs +++ b/ffi/src/putty.rs @@ -139,7 +139,8 @@ pub mod ffi { /// Encode PPK key file to a string. pub fn to_repr(&self, writeable: &mut DiplomatWrite) -> Result<(), Box> { - writeable.write_str(&self.0.to_string()?)?; + let repr = zeroize::Zeroizing::new(self.0.to_string()?); + writeable.write_str(&repr)?; writeable.flush(); Ok(()) } diff --git a/ffi/src/ssh.rs b/ffi/src/ssh.rs index 9b73f449..9f59dbf6 100644 --- a/ffi/src/ssh.rs +++ b/ffi/src/ssh.rs @@ -225,7 +225,7 @@ pub mod ffi { /// Returns the SSH Private Key string representation. pub fn to_repr(&self, writeable: &mut DiplomatWrite) -> Result<(), Box> { - let repr = self.0.to_string()?; + let repr = zeroize::Zeroizing::new(self.0.to_string()?); writeable.write_str(&repr)?; writeable.flush(); Ok(()) @@ -415,3 +415,12 @@ pub mod ffi { } } } + +// picky wipes the key material itself, but keeps the passphrase in a plain `String`. +impl Drop for ffi::SshPrivateKey { + fn drop(&mut self) { + if let Some(passphrase) = self.0.passphrase.as_mut() { + zeroize::Zeroize::zeroize(passphrase); + } + } +} diff --git a/picky/src/putty/ppk/aes.rs b/picky/src/putty/ppk/aes.rs index dac24791..f3e1c4c0 100644 --- a/picky/src/putty/ppk/aes.rs +++ b/picky/src/putty/ppk/aes.rs @@ -6,21 +6,26 @@ use aes::cipher::block_padding::NoPadding; use cbc::cipher::{BlockModeDecrypt, BlockModeEncrypt}; use inout::InOutBufReserved; use rand_core::Rng; +use zeroize::Zeroizing; pub const KEY_SIZE: usize = 32; pub const BLOCK_SIZE: usize = 16; -/// Adds padding to the message if it is not a multiple of the AES block size. -pub fn make_padding(mut message: Vec, mut rng: R) -> Vec { - if message.len() % BLOCK_SIZE != 0 { - let unpadded_size = message.len(); - let padding_size = BLOCK_SIZE - (unpadded_size % BLOCK_SIZE); +/// Returns a copy of the message, padded with random bytes to a multiple of the AES block size. +pub fn make_padding(message: &[u8], mut rng: R) -> Zeroizing> { + let unpadded_size = message.len(); + let padded_size = unpadded_size.next_multiple_of(BLOCK_SIZE); - message.resize(unpadded_size + padding_size, 0); - rng.fill_bytes(&mut message[unpadded_size..]); + // Allocate the final size up front so growing the buffer can't leave a copy of the key behind. + let mut padded = Zeroizing::new(Vec::with_capacity(padded_size)); + padded.extend_from_slice(message); + + if padded_size != unpadded_size { + padded.resize(padded_size, 0); + rng.fill_bytes(&mut padded[unpadded_size..]); } - message + padded } /// Encrypts the message in-place using AES-256 in CBC mode. diff --git a/picky/src/putty/ppk/encoding.rs b/picky/src/putty/ppk/encoding.rs index 6d28abed..ba3c3984 100644 --- a/picky/src/putty/ppk/encoding.rs +++ b/picky/src/putty/ppk/encoding.rs @@ -9,6 +9,7 @@ use crate::putty::key_value::{ use crate::putty::ppk::encryption::PpkEncryptionKind; use crate::putty::{Argon2Params, Ppk, PuttyError}; use std::str::FromStr; +use zeroize::Zeroizing; impl FromStr for Ppk { type Err = PuttyError; @@ -50,7 +51,7 @@ impl FromStr for Ppk { encryption, comment: comment.into(), public_key: public_key.into(), - private_key: private_key.into(), + private_key: Zeroizing::new(private_key.into()), mac: mac.into(), }; @@ -93,7 +94,7 @@ impl Ppk { None | Some(PpkEncryptionKind::Aes256CbcV2) => {} } - writer.write_multiline_value::(Base64PpkValue::from(self.private_key.clone())); + writer.write_multiline_value::(Base64PpkValue::from(self.private_key.to_vec())); writer.write_value::(HexPpkValue::from(self.mac.clone())); Ok(writer.finish()) diff --git a/picky/src/putty/ppk/encryption.rs b/picky/src/putty/ppk/encryption.rs index 50f66933..0bc58f07 100644 --- a/picky/src/putty/ppk/encryption.rs +++ b/picky/src/putty/ppk/encryption.rs @@ -131,7 +131,7 @@ impl Ppk { PpkVersionKey::V2 => { let key_material = kdf::derive_key_material_v2(passphrase)?; - let mut private_key = ppk_aes::make_padding(self.private_key.clone(), rng); + let mut private_key = ppk_aes::make_padding(&self.private_key, rng); let mac = self.calculate_mac_v2(passphrase, &private_key, PpkEncryptionValue::Aes256Cbc)?; ppk_aes::encrypt(&mut private_key, key_material.key(), KeyMaterialV2::iv())?; @@ -158,7 +158,7 @@ impl Ppk { let key_material = kdf::derive_key_material_v3(&argon2_params, passphrase)?; - let mut private_key = ppk_aes::make_padding(self.private_key.clone(), rng); + let mut private_key = ppk_aes::make_padding(&self.private_key, rng); let mac = self.calculate_mac_v3(key_material.hmac_key(), &private_key, PpkEncryptionValue::Aes256Cbc)?; ppk_aes::encrypt(&mut private_key, key_material.key(), key_material.iv())?; diff --git a/picky/src/putty/ppk/mod.rs b/picky/src/putty/ppk/mod.rs index c367000e..a07ff740 100644 --- a/picky/src/putty/ppk/mod.rs +++ b/picky/src/putty/ppk/mod.rs @@ -10,6 +10,7 @@ use crate::putty::key_value::{PpkKeyAlgorithmValue, PpkVersionKey}; use crate::putty::private_key::{PuttyBasePrivateKey, PuttyPrivateKey}; use crate::putty::public_key::{PuttyBasePublicKey, PuttyPublicKey}; use crate::ssh::SshPrivateKey; +use zeroize::Zeroizing; use self::encryption::PpkEncryptionKind; @@ -37,7 +38,7 @@ pub struct Ppk { encryption: Option, comment: String, public_key: Vec, - private_key: Vec, + private_key: Zeroizing>, mac: Vec, } diff --git a/picky/src/putty/private_key.rs b/picky/src/putty/private_key.rs index 1ff8a1fd..fbe169b1 100644 --- a/picky/src/putty/private_key.rs +++ b/picky/src/putty/private_key.rs @@ -12,6 +12,7 @@ use crate::ssh::public_key::SshBasePublicKey; use crypto_bigint::BoxedUint; use rsa::traits::{PrivateKeyParts, PublicKeyParts}; use rsa::{RsaPrivateKey, RsaPublicKey}; +use zeroize::Zeroizing; /// PuTTY private key wrapper pub(crate) struct PuttyPrivateKey { @@ -56,13 +57,13 @@ impl PuttyPrivateKey { pub(crate) struct PuttyBasePrivateKey { pub(crate) algorithm: PpkKeyAlgorithmValue, pub(crate) public_key: PuttyBasePublicKey, - pub(crate) data: Vec, + pub(crate) data: Zeroizing>, } impl PuttyBasePrivateKey { pub fn from_openssh(key: &SshBasePrivateKey) -> Result { - let mut data = Vec::new(); - let cursor = &mut data; + let mut data = Zeroizing::new(Vec::new()); + let cursor = &mut *data; match key { SshBasePrivateKey::SkEcdsaSha2NistP256 { .. } | SshBasePrivateKey::SkEd25519 { .. } => {