diff --git a/Cargo.lock b/Cargo.lock index 85a6aa1d..2a0a98e0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1126,6 +1126,7 @@ dependencies = [ "tun", "windows-service", "wintun", + "zeroize", ] [[package]] @@ -4288,6 +4289,20 @@ name = "zeroize" version = "1.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e13c156562582aa81c60cb29407084cdb54c4164760106ab78e6c5b0858cf64e" +dependencies = [ + "zeroize_derive", +] + +[[package]] +name = "zeroize_derive" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3c50655cbb0fe3fc43170059e702f1ce5e19b84cec58dc87b037a09935c2f328" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.119", +] [[package]] name = "zerotrie" diff --git a/Cargo.toml b/Cargo.toml index 7d8573b7..d5b1db2b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,6 +17,7 @@ secp256k1 = { version = "0.30", features = ["rand", "global-context"] } sha2 = "0.10" hkdf = "0.12" ring = "0.17" +zeroize = { version = "1.9", features = ["zeroize_derive"] } rand = "0.10.1" crossbeam-channel = "0.5" thiserror = "2.0" diff --git a/src/node/encrypt_worker.rs b/src/node/encrypt_worker.rs index d27ebfb8..c12b27f7 100644 --- a/src/node/encrypt_worker.rs +++ b/src/node/encrypt_worker.rs @@ -93,8 +93,10 @@ use tracing::{debug, trace, warn}; /// inside the worker. That second alloc + ~1.5 KB memcpy per packet at /// line rate cost ~150 MB/sec of memory bandwidth on the hot worker.) pub(crate) struct FmpSendJob { - /// Cloned FMP send cipher. `LessSafeKey` is `Clone` (`ring::aead`) - /// — the clone is just a refcount bump on the inner key material. + /// Cloned FMP send cipher. `LessSafeKey` is `Clone` (`ring::aead`), but + /// the clone is not a refcount bump: `ring` stores the ChaCha20 key + /// inline as `[u32; 8]`, so cloning copies the key material outright and + /// leaves a second copy that nothing outside `ring` can clear. pub cipher: LessSafeKey, /// Pre-reserved monotonic counter (via `take_send_counter`). pub counter: u64, diff --git a/src/noise/handshake.rs b/src/noise/handshake.rs index db9f4000..b7dbb881 100644 --- a/src/noise/handshake.rs +++ b/src/noise/handshake.rs @@ -9,6 +9,7 @@ use rand::Rng; use secp256k1::{Keypair, PublicKey, Secp256k1, SecretKey, ecdh::shared_secret_point}; use sha2::{Digest, Sha256}; use std::fmt; +use zeroize::{Zeroize, ZeroizeOnDrop}; /// Symmetric state during handshake. /// @@ -17,7 +18,11 @@ use std::fmt; /// `Clone` exists for [`HandshakeState::try_read_xk_message_2`], which has to /// put the pre-read state back after a message that mixed material in before /// failing to authenticate. -#[derive(Clone)] +/// +/// `ck` and `h` are cleared on drop, including on the clone above once it +/// goes out of scope. `cipher` is skipped because [`CipherState`] clears its +/// own retained key. +#[derive(Clone, Zeroize, ZeroizeOnDrop)] struct SymmetricState { /// Chaining key for key derivation. ck: [u8; 32], @@ -30,6 +35,7 @@ struct SymmetricState { /// cookie binding) will silently not work until the AAD carries `h`. h: [u8; 32], /// Current cipher state for encrypting handshake payloads. + #[zeroize(skip)] cipher: CipherState, } @@ -76,6 +82,9 @@ impl SymmetricState { let mut key = [0u8; 32]; key.copy_from_slice(&output[32..64]); self.cipher.initialize_key(key); + key.zeroize(); + + output.zeroize(); } /// Encrypt and mix into hash. @@ -104,7 +113,13 @@ impl SymmetricState { k1.copy_from_slice(&output[..32]); k2.copy_from_slice(&output[32..64]); - (CipherState::new(k1), CipherState::new(k2)) + let ciphers = (CipherState::new(k1), CipherState::new(k2)); + + output.zeroize(); + k1.zeroize(); + k2.zeroize(); + + ciphers } /// Get the handshake hash. diff --git a/src/noise/mod.rs b/src/noise/mod.rs index a3d656c4..e2c21be0 100644 --- a/src/noise/mod.rs +++ b/src/noise/mod.rs @@ -42,6 +42,7 @@ mod session; use ring::aead::{Aad, CHACHA20_POLY1305, LessSafeKey, Nonce, UnboundKey}; use std::fmt; use thiserror::Error; +use zeroize::{Zeroize, ZeroizeOnDrop}; pub use handshake::HandshakeState; pub use replay::ReplayWindow; @@ -181,21 +182,32 @@ impl fmt::Display for HandshakeProgress { /// AEAD is `ring`'s ChaCha20-Poly1305 (BoringSSL backend), which dispatches /// to NEON on aarch64 and AVX2/AVX-512 on x86_64. The 32-byte key is /// retained alongside a cached `LessSafeKey` so the per-packet AEAD skips -/// the keyed-cipher construction (key copy + Poly1305 key derivation). -/// `LessSafeKey` itself doesn't implement `Clone` (deliberate, for safety), -/// so `CipherState`'s manual `Clone` impl rebuilds the keyed AEAD from the -/// retained key bytes — cheap for ChaCha20-Poly1305 since the construction -/// is essentially a key copy plus a constant-time check. +/// rebuilding the keyed cipher. (It does not skip Poly1305 key derivation, +/// which is nonce-dependent and happens per message inside seal and open.) +/// `CipherState`'s manual `Clone` impl rebuilds the keyed AEAD from those +/// retained bytes, and so does `cipher_clone` for each off-task AEAD worker; +/// those two are why the bytes are kept. The rebuild is cheap for +/// ChaCha20-Poly1305: a length check and a conversion of the 32 key bytes +/// into little-endian words. +/// +/// Because the key bytes are retained, every clone leaves a second copy in +/// memory; each copy is cleared when it goes out of scope. Only `key` is +/// zeroized. `cipher` is skipped because `LessSafeKey` +/// holds its material behind `ring`'s own API and cannot be cleared from +/// here; `nonce` and `has_key` are skipped because they are not secret. +#[derive(Zeroize, ZeroizeOnDrop)] pub struct CipherState { - /// Encryption key (32 bytes). Retained so we can rebuild the keyed - /// AEAD on `Clone` and on `initialize_key` (ring's `UnboundKey` / - /// `LessSafeKey` do not implement `Clone`). + /// Encryption key (32 bytes). Retained because `Clone` and + /// `cipher_clone` rebuild the keyed AEAD from these bytes. key: [u8; 32], /// Cached keyed AEAD, valid iff `has_key`. None for an un-keyed state. + #[zeroize(skip)] cipher: Option, /// Nonce counter (8 bytes used, 4 bytes zero prefix). + #[zeroize(skip)] pub(super) nonce: u64, /// Whether this cipher has a valid key. + #[zeroize(skip)] has_key: bool, } @@ -395,6 +407,13 @@ impl CipherState { self.has_key } + /// Copy out the retained key bytes, so a test can observe zeroization + /// without reading freed memory. + #[cfg(test)] + fn key_bytes(&self) -> [u8; 32] { + self.key + } + /// Clone the underlying keyed AEAD, for off-task AEAD workers. /// /// Returns `None` if no key. The cloned `LessSafeKey` pairs with diff --git a/src/noise/tests.rs b/src/noise/tests.rs index f452f8bc..c03a0ef8 100644 --- a/src/noise/tests.rs +++ b/src/noise/tests.rs @@ -201,6 +201,26 @@ fn test_cipher_state_nonce_sequence() { assert_eq!(cipher.nonce(), 2); } +#[test] +fn cipher_state_drop_impl_clears_the_retained_key() { + // Reading the bytes back out of a dropped `CipherState` would mean + // reading freed memory, so the drop behaviour is asserted through two + // observable proxies instead: that `CipherState` implements + // `ZeroizeOnDrop`, and that an explicit `zeroize()` leaves `key` + // all-zero while `has_key` is untouched. + fn assert_zeroize_on_drop() {} + assert_zeroize_on_drop::(); + + let mut cipher = CipherState::new([7u8; 32]); + assert_eq!(cipher.key_bytes(), [7u8; 32]); + assert!(cipher.has_key()); + + cipher.zeroize(); + + assert_eq!(cipher.key_bytes(), [0u8; 32]); + assert!(cipher.has_key()); +} + #[test] fn test_session_remote_static() { let keypair1 = generate_keypair();