fix: wipe private key bytes and passphrases when they're freed

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]

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<PickyException>(() => 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());
+    }
+}

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<PickyException>(() => 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]

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);
+        }
+    }
+}

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<PickyError>> {
-            writeable.write_str(&self.0.to_string()?)?;
+            let repr = zeroize::Zeroizing::new(self.0.to_string()?);
+            writeable.write_str(&repr)?;
             writeable.flush();
             Ok(())
         }

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<PickyError>> {
-            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);
+        }
+    }
+}

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<R: Rng>(mut message: Vec<u8>, mut rng: R) -> Vec<u8> {
-    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<R: Rng>(message: &[u8], mut rng: R) -> Zeroizing<Vec<u8>> {
+    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.

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::<PpkPrivateLines>(Base64PpkValue::from(self.private_key.clone()));
+        writer.write_multiline_value::<PpkPrivateLines>(Base64PpkValue::from(self.private_key.to_vec()));
         writer.write_value::<PpkPrivateMac>(HexPpkValue::from(self.mac.clone()));
 
         Ok(writer.finish())

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())?;

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<PpkEncryptionKind>,
     comment: String,
     public_key: Vec<u8>,
-    private_key: Vec<u8>,
+    private_key: Zeroizing<Vec<u8>>,
     mac: Vec<u8>,
 }
 

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<u8>,
+    pub(crate) data: Zeroizing<Vec<u8>>,
 }
 
 impl PuttyBasePrivateKey {
     pub fn from_openssh(key: &SshBasePrivateKey) -> Result<Self, PuttyError> {
-        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 { .. } => {