Merge pull request #979 from NordSecurity/LLT-5457_add_zeroize_on_dro…

…p_for_sensitive_key_types

Zeroize memory upon destruction of sensitive key structs

.unreleased/LLT-5457

@@ -0,0 +1 @@
+LLT-5457 - clear memory for sensitive key material on destructor

clis/derpcli/src/conf.rs

@@ -96,10 +96,10 @@ impl StressConfig {
 
 impl Config {
     pub fn get_key1(&self) -> crypto_box::SecretKey {
-        self.my_key.into()
+        self.my_key.clone().into()
     }
     pub fn get_key2(&self) -> crypto_box::SecretKey {
-        self.target_key.into()
+        self.target_key.clone().into()
     }
     pub fn get_pub_key1(&self) -> crypto_box::PublicKey {
         BoxPublicKey::from(&self.get_key1())

clis/derpcli/src/main.rs

@@ -163,7 +163,7 @@ async fn connect_client(
         &client.derp_server,
         *addrs.first().ok_or_else(|| anyhow!("Empty socket Addr"))?,
         Config {
-            secret_key: client.private_key,
+            secret_key: client.private_key.clone(),
             timeout: Duration::from_secs(10),
             ..Default::default()
         },
@@ -343,7 +343,7 @@ async fn run_without_clients_config(config: conf::Config) -> Result<()> {
             .first()
             .ok_or_else(|| anyhow!("ERR!! Empty server addr"))?,
         Config {
-            secret_key: config.my_key,
+            secret_key: config.my_key.clone(),
             timeout: Duration::from_secs(5),
             use_built_in_root_certificates: config.use_built_in_root_certificates,
             ..Default::default()
@@ -623,9 +623,9 @@ async fn run_with_clients_config(
                 .ok_or_else(|| anyhow!("Err! No derp found"))?
                 .clone();
             let client1 = conf::ClientConfig {
-                private_key: key1,
+                private_key: key1.clone(),
                 derp_server: derps1,
-                peers: vec![key2],
+                peers: vec![key2.clone()],
             };
             let client2 = conf::ClientConfig {
                 private_key: key2,

clis/interderpcli/src/main.rs

@@ -203,10 +203,10 @@ async fn test_pair(
     let addr_2 = resolve_domain_name(&host_2).await?;
 
     info!("Starting clients");
-    let mut client_1 = Box::pin(connect_client(addr_1, host_1.clone(), key_1))
+    let mut client_1 = Box::pin(connect_client(addr_1, host_1.clone(), key_1.clone()))
         .await
         .context("Failed to connect to first server")?;
-    let mut client_2 = Box::pin(connect_client(addr_2, host_2.clone(), key_2))
+    let mut client_2 = Box::pin(connect_client(addr_2, host_2.clone(), key_2.clone()))
         .await
         .context("Failed to connect to second server")?;
 
@@ -228,9 +228,9 @@ async fn config_file_scenario(config_file: PathBuf, verbose: bool) -> Result<()>
             (Some(first), Some(second)) => {
                 match Box::pin(test_pair(
                     first.to_string(),
-                    config.private_key_1,
+                    config.private_key_1.clone(),
                     second.to_string(),
-                    config.private_key_2,
+                    config.private_key_2.clone(),
                 ))
                 .await
                 {

clis/tcli/src/cli.rs

@@ -772,7 +772,7 @@ impl Cli {
     ) -> Result<(), Error> {
         if !self.telio.is_running() {
             let device_config = DeviceConfig {
-                private_key,
+                private_key: private_key.clone(),
                 name: Some(name),
                 adapter: adapter_type,
                 ..Default::default()

clis/tcli/src/derp.rs

@@ -71,7 +71,7 @@ impl DerpClient {
                 allowed_pk,
             } => {
                 let mut config = telio_relay::Config {
-                    secret_key,
+                    secret_key: secret_key.clone(),
                     ..Default::default()
                 };
 

clis/teliod/src/core_api.rs

@@ -86,7 +86,7 @@ pub async fn init_with_api(
         Some(identity) => identity,
         None => {
             let private_key = SecretKey::gen();
-            let hw_identifier = generate_hw_identifier(private_key);
+            let hw_identifier = generate_hw_identifier(private_key.clone());
 
             let machine_identifier =
                 match fetch_identifier_with_exp_backoff(auth_token, private_key.public()).await {

clis/teliod/src/daemon.rs

@@ -69,7 +69,7 @@ fn telio_task(
     if !auth_token.as_str().eq("") {
         start_telio(
             &mut telio,
-            node_identity.private_key,
+            node_identity.private_key.clone(),
             &interface_config.name,
         )?;
         task_retrieve_meshmap(node_identity, auth_token, tx_channel.clone());

crates/telio-crypto/Cargo.toml

@@ -11,7 +11,7 @@ aead = { version = "0.5.2", default-features = false }
 serde_with = { features = ["macros"], default-features = false, version = "3.7.0" }
 chacha20poly1305 = { default-features = false, version = "0.10.1" }
 chacha20 = "0.9.1"
-zeroize = { version = "1", default-features = false }
+zeroize = { version = "1.8.1", features = ["derive"] }
 curve25519-dalek = { default-features = false, version = "4.1.3" }
 
 base64.workspace = true

crates/telio-crypto/src/lib.rs

@@ -29,13 +29,24 @@ use rand::prelude::*;
 use serde::{Deserialize, Serialize};
 use serde_with::{DeserializeFromStr, SerializeDisplay};
 use telio_utils::Hidden;
+use zeroize::ZeroizeOnDrop;
 
 /// Secret, Public and Wireguard Preshared key size in bytes
 pub const KEY_SIZE: usize = 32;
 
 /// Secret key type
 #[derive(
-    Default, Debug, PartialOrd, Ord, PartialEq, Eq, Hash, Copy, Clone, Serialize, Deserialize,
+    Default,
+    Debug,
+    PartialOrd,
+    Ord,
+    PartialEq,
+    Eq,
+    Hash,
+    Clone,
+    Serialize,
+    Deserialize,
+    ZeroizeOnDrop,
 )]
 pub struct SecretKey(Hidden<[u8; KEY_SIZE]>);
 
@@ -97,7 +108,7 @@ pub fn smaller_key_in_meshnet_canonical_order<'a>(
 }
 
 /// Preshared key type
-#[derive(Default, Debug, PartialOrd, Ord, PartialEq, Eq, Hash, Copy, Clone)]
+#[derive(Default, Debug, PartialOrd, Ord, PartialEq, Eq, Hash, Clone, ZeroizeOnDrop)]
 pub struct PresharedKey(pub Hidden<[u8; KEY_SIZE]>);
 
 /// Error returned when parsing fails for SecretKey or PublicKey.

crates/telio-dns/src/dns.rs

@@ -103,7 +103,7 @@ impl LocalDnsResolver {
 
         Ok(LocalDnsResolver {
             socket: Arc::new(socket),
-            secret_key: dns_secret_key,
+            secret_key: dns_secret_key.clone(),
             peer: Arc::new(Mutex::new(Tunn::new(
                 StaticSecret::from(dns_secret_key.into_bytes()),
                 telio_public_key,
@@ -149,7 +149,7 @@ impl DnsResolver for LocalDnsResolver {
     }
 
     fn public_key(&self) -> PublicKey {
-        let static_secret = &StaticSecret::from(self.secret_key.into_bytes());
+        let static_secret = &StaticSecret::from(self.secret_key.clone().into_bytes());
         telio_log_debug!(
             "Dns - public_key: {:?}",
             PublicKeyDalek::from(static_secret)
@@ -191,7 +191,7 @@ impl DnsResolver for LocalDnsResolver {
 
     async fn set_peer_public_key(&self, pubkey: PublicKey) {
         let peer = match Tunn::new(
-            StaticSecret::from(self.secret_key.into_bytes()),
+            StaticSecret::from(self.secret_key.clone().into_bytes()),
             PublicKeyDalek::from(pubkey.0),
             None,
             None,

crates/telio-nurse/src/aggregator.rs

@@ -555,7 +555,7 @@ mod tests {
                 .collect();
             keys.sort_by_key(|(_sk, pk)| *pk);
             let mut i = Interface::default();
-            i.private_key = Some(keys[1].0);
+            i.private_key = Some(keys[1].0.clone());
             match rx_tx_bytes {
                 Some((rx, tx)) => {
                     i.peers = [(
@@ -577,7 +577,7 @@ mod tests {
             }
         };
 
-        let local_key = interface.private_key.unwrap().public();
+        let local_key = interface.private_key.clone().unwrap().public();
 
         let mut wg_interface = MockWireGuard::new();
         wg_interface

crates/telio-nurse/src/heartbeat.rs

@@ -1255,7 +1255,7 @@ mod tests {
         let peer_sk_1 = SecretKey::gen();
         let peer_sk_2 = SecretKey::gen();
 
-        setup_local_nodes(&mut analytics, true, [peer_sk_1, peer_sk_2]).await;
+        setup_local_nodes(&mut analytics, true, [peer_sk_1.clone(), peer_sk_2.clone()]).await;
 
         analytics.handle_collection().await;
         assert_eq!(RuntimeState::Collecting, analytics.state);
@@ -1280,7 +1280,7 @@ mod tests {
         let peer_sk_1 = SecretKey::gen();
         let peer_sk_2 = SecretKey::gen();
 
-        setup_local_nodes(&mut analytics, true, [peer_sk_1, peer_sk_2]).await;
+        setup_local_nodes(&mut analytics, true, [peer_sk_1.clone(), peer_sk_2.clone()]).await;
 
         analytics.handle_collection().await;
         assert_eq!(RuntimeState::Collecting, analytics.state);
@@ -1310,13 +1310,13 @@ mod tests {
 
         let peer_sk = SecretKey::gen();
 
-        setup_local_nodes(&mut analytics, true, [peer_sk]).await;
+        setup_local_nodes(&mut analytics, true, [peer_sk.clone()]).await;
 
         analytics.handle_collection().await;
         assert_eq!(RuntimeState::Collecting, analytics.state);
 
         let other_meshnet_id = Uuid::new_v4();
-        send_heartbeat_response(&mut analytics, peer_sk, other_meshnet_id).await;
+        send_heartbeat_response(&mut analytics, peer_sk.clone(), other_meshnet_id).await;
 
         analytics.handle_aggregation().await;
 
@@ -1565,7 +1565,7 @@ mod tests {
 
         let mut state: State = setup(Some(Duration::from_secs(3600)), Some(Arc::new(aggregator)));
 
-        setup_local_nodes(&mut state.analytics, true, [peer_sk]).await;
+        setup_local_nodes(&mut state.analytics, true, [peer_sk.clone()]).await;
 
         state.analytics.handle_collection().await;
         assert_eq!(RuntimeState::Collecting, state.analytics.state);

crates/telio-pq/src/conn.rs

@@ -61,7 +61,9 @@ impl ConnKeyRotation {
             // It can only be closed on library shutdown, in that case we
             // may not care since the task itself will be killed soon
             #[allow(mpsc_blocking_send)]
-            let _ = chan.send(super::Event::Handshake(addr, wg_keys)).await;
+            let _ = chan
+                .send(super::Event::Handshake(addr, wg_keys.clone()))
+                .await;
 
             telio_log_debug!("Rekey interval: {}s", rekey_interval.as_secs());
             let mut interval = telio_utils::interval(rekey_interval);
@@ -77,13 +79,13 @@ impl ConnKeyRotation {
                 match tokio::time::timeout(request_retry, rekey).await {
                     Ok(Ok(key)) => {
                         telio_log_debug!("Successful PQ REKEY");
-                        wg_keys.pq_shared = key;
+                        wg_keys.pq_shared = key.clone();
 
                         // The channel is allways open during the library operation.
                         // It can only be closed on library shutdown, in that case we
                         // may not care since the task itself will be killed soon
                         #[allow(mpsc_blocking_send)]
-                        let _ = chan.send(super::Event::Rekey(wg_keys)).await;
+                        let _ = chan.send(super::Event::Rekey(wg_keys.clone())).await;
                     }
                     Ok(Err(err)) => {
                         telio_log_warn!("Failed to perform PQ rekey: {err}");

crates/telio-pq/src/entity.rs

@@ -21,7 +21,7 @@ pub struct Entity {
 
 impl crate::PostQuantum for Entity {
     fn keys(&self) -> Option<crate::Keys> {
-        self.peer.as_ref().and_then(|p| p.keys)
+        self.peer.as_ref().and_then(|p| p.keys.clone())
     }
 
     fn is_rotating_keys(&self) -> bool {

crates/telio-pq/src/lib.rs

@@ -8,7 +8,7 @@ use std::net::SocketAddr;
 pub use entity::Entity;
 
 /// Post quantum keys retrived from hanshake
-#[derive(Clone, Copy)]
+#[derive(Clone)]
 pub struct Keys {
     /// Kyber shared secret
     pub pq_shared: telio_crypto::PresharedKey,
@@ -22,7 +22,7 @@ pub trait PostQuantum {
     fn is_rotating_keys(&self) -> bool;
 }
 
-#[derive(Copy, Clone)]
+#[derive(Clone)]
 pub enum Event {
     Handshake(SocketAddr, Keys),
     Rekey(Keys),

crates/telio-pq/src/proto.rs

@@ -178,7 +178,7 @@ async fn handshake(
     sock.connect(endpoint).await?;
 
     let mut tunn = noise::Tunn::new(
-        secret.into_bytes().into(),
+        secret.clone().into_bytes().into(),
         peers_pubkey.0.into(),
         None,
         None,

crates/telio-relay/src/derp.rs

@@ -542,7 +542,8 @@ impl State {
             msg.packet_type()
         );
         match msg.encode() {
-            Ok(buf) => match DerpRelay::encrypt_if_needed(config.secret_key, pk, rng, &buf) {
+            Ok(buf) => match DerpRelay::encrypt_if_needed(config.secret_key.clone(), pk, rng, &buf)
+            {
                 Ok(cipher_text) => {
                     let _ = permit.send((pk, cipher_text));
                 }
@@ -581,7 +582,7 @@ impl State {
         config: &Config,
     ) {
         if config.meshnet_peers.contains(&pk) {
-            match DerpRelay::decrypt_if_needed(config.secret_key, pk, &buf) {
+            match DerpRelay::decrypt_if_needed(config.secret_key.clone(), pk, &buf) {
                 Ok(plain_text) => match PacketRelayed::decode(&plain_text) {
                     Ok(msg) => {
                         telio_log_trace!(
@@ -995,7 +996,7 @@ mod tests {
         } = McChan::default();
 
         let config = Config {
-            secret_key: test_conf.private_key,
+            secret_key: test_conf.private_key.clone(),
             servers: SortedServers::new(vec![Server {
                 hostname: "de1047.nordvpn.com".into(),
                 relay_port: 8765,

crates/telio-relay/src/derp/proto.rs

@@ -502,10 +502,10 @@ mod tests {
         let server_key1 = SecretKey::new([1_u8; KEY_SIZE]).public();
         let secret_key2 = SecretKey::new([2_u8; KEY_SIZE]);
         let server_key2 = SecretKey::new([3_u8; KEY_SIZE]).public();
-        write_client_key(&mut buf1, secret_key1, server_key1, None)
+        write_client_key(&mut buf1, secret_key1.clone(), server_key1.clone(), None)
             .await
             .unwrap();
-        write_client_key(&mut buf2, secret_key1, server_key1, None)
+        write_client_key(&mut buf2, secret_key1.clone(), server_key1.clone(), None)
             .await
             .unwrap();
         write_client_key(&mut buf3, secret_key2, server_key2, None)

crates/telio-starcast/src/starcast_peer.rs

@@ -170,7 +170,7 @@ impl State {
 
         self.tunnel = Some(
             Tunn::new(
-                StaticSecret::from(self.secret_key.into_bytes()),
+                StaticSecret::from(self.secret_key.clone().into_bytes()),
                 PublicKeyDalek::from(config.public_key.0),
                 None,
                 None,
@@ -184,7 +184,7 @@ impl State {
     }
 
     fn get_peer(&self) -> Result<Peer, Error> {
-        let static_secret = &StaticSecret::from(self.secret_key.into_bytes());
+        let static_secret = &StaticSecret::from(self.secret_key.clone().into_bytes());
 
         Ok(Peer {
             public_key: PublicKey(PublicKeyDalek::from(static_secret).to_bytes()),