Use Zeroize for temporary sensitive data

[SanjoDeundiak]

We use Zeroize for Vault secrets, however there are places (e.g. key agreement) where we have temporary sensitive values, those should use Zeroize wrappers for secure wipe

[abhishekc-sharma]

I can attempt this. Is this within the ockam_vault crate ? I can try to look through the code and understand, but may need pointers on where to start.

[birkett83]

I did this and then discovered you need a code-signing-key to make a PR, and I don't have one. If you can use a patch, feel free, otherwise, never mind.

index d4623e54..a37ed2d5 100644
--- a/implementations/rust/ockam/ockam_key_exchange_x3dh/Cargo.toml
+++ b/implementations/rust/ockam/ockam_key_exchange_x3dh/Cargo.toml
@@ -31,6 +31,7 @@ ockam_core = { path = "../ockam_core", version = "0.36.0", default_features = fa
 ockam_vault_core = { path = "../ockam_vault_core" , version = "0.30.0", default_features = false }
 ockam_key_exchange_core = { path = "../ockam_key_exchange_core" , version = "0.28.0", default_features = false }
 arrayref = "0.3"
+zeroize = { version = "1.4.2", features = ["zeroize_derive"] }
 
 [dev-dependencies]
 ockam_vault_sync_core = { path = "../ockam_vault_sync_core", version = "0.27.0"         }
diff --git a/implementations/rust/ockam/ockam_key_exchange_x3dh/src/initiator.rs b/implementations/rust/ockam/ockam_key_exchange_x3dh/src/initiator.rs
index a5fe00a1..541da605 100644
--- a/implementations/rust/ockam/ockam_key_exchange_x3dh/src/initiator.rs
+++ b/implementations/rust/ockam/ockam_key_exchange_x3dh/src/initiator.rs
@@ -12,6 +12,7 @@ use ockam_vault_core::{
     Secret, SecretAttributes, SecretPersistence, SecretType, AES256_SECRET_LENGTH,
     CURVE25519_SECRET_LENGTH,
 };
+use zeroize::Zeroizing;
 
 #[derive(Debug, Clone, Copy)]
 enum InitiatorState {
@@ -23,8 +24,8 @@ enum InitiatorState {
 /// The responder of X3DH receives a prekey bundle and computes the shared secret
 /// to communicate the first message to the initiator
 pub struct Initiator<V: X3dhVault> {
-    identity_key: Option<Secret>,
-    ephemeral_identity_key: Option<Secret>,
+    identity_key: Zeroizing<Option<Secret>>,
+    ephemeral_identity_key: Zeroizing<Option<Secret>>,
     prekey_bundle: Option<PreKeyBundle>,
     state: InitiatorState,
     vault: V,
@@ -34,8 +35,8 @@ pub struct Initiator<V: X3dhVault> {
 impl<V: X3dhVault> Initiator<V> {
     pub(crate) fn new(vault: V, identity_key: Option<Secret>) -> Self {
         Self {
-            identity_key,
-            ephemeral_identity_key: None,
+            identity_key: Zeroizing::new(identity_key),
+            ephemeral_identity_key: Zeroizing::new(None),
             prekey_bundle: None,
             state: InitiatorState::GenerateEphemeralIdentityKey,
             vault,
@@ -51,7 +52,7 @@ impl<V: X3dhVault> Initiator<V> {
                 CURVE25519_SECRET_LENGTH,
             );
 
-            self.identity_key = Some(self.vault.secret_generate(p_atts).await?);
+            self.identity_key = Zeroizing::new(Some(self.vault.secret_generate(p_atts).await?));
         }
         Ok(())
     }
@@ -96,7 +97,7 @@ impl<V: X3dhVault> KeyExchanger for Initiator<V> {
                     .vault
                     .secret_public_key_get(&ephemeral_identity_key)
                     .await?;
-                self.ephemeral_identity_key = Some(ephemeral_identity_key);
+                self.ephemeral_identity_key = Zeroizing::new(Some(ephemeral_identity_key));
                 self.state = InitiatorState::ProcessPreKeyBundle;
 
                 let mut response = Vec::new();
diff --git a/implementations/rust/ockam/ockam_key_exchange_x3dh/src/responder.rs b/implementations/rust/ockam/ockam_key_exchange_x3dh/src/responder.rs
index aca3a863..ebd2cac7 100644
--- a/implementations/rust/ockam/ockam_key_exchange_x3dh/src/responder.rs
+++ b/implementations/rust/ockam/ockam_key_exchange_x3dh/src/responder.rs
@@ -11,6 +11,7 @@ use ockam_vault_core::{
     PublicKey, Secret, SecretAttributes, SecretPersistence, SecretType, AES256_SECRET_LENGTH,
     CURVE25519_SECRET_LENGTH,
 };
+use zeroize::Zeroizing;
 
 #[derive(Debug)]
 enum ResponderState {
@@ -25,9 +26,9 @@ enum ResponderState {
 /// The responder of X3DH creates a prekey bundle that can be used to establish a shared
 /// secret key with another party that can use
 pub struct Responder<V: X3dhVault> {
-    identity_key: Option<Secret>,
-    signed_prekey: Option<Secret>,
-    one_time_prekey: Option<Secret>,
+    identity_key: Zeroizing<Option<Secret>>,
+    signed_prekey: Zeroizing<Option<Secret>>,
+    one_time_prekey: Zeroizing<Option<Secret>>,
     state: ResponderState,
     vault: V,
     completed_key_exchange: Option<CompletedKeyExchange>,
@@ -36,9 +37,9 @@ pub struct Responder<V: X3dhVault> {
 impl<V: X3dhVault> Responder<V> {
     pub(crate) fn new(vault: V, identity_key: Option<Secret>) -> Self {
         Self {
-            identity_key,
-            signed_prekey: None,
-            one_time_prekey: None,
+            identity_key: Zeroizing::new(identity_key),
+            signed_prekey: Zeroizing::new(None),
+            one_time_prekey: Zeroizing::new(None),
             completed_key_exchange: None,
             state: ResponderState::HandleInitiatorKeys,
             vault,
@@ -57,10 +58,10 @@ impl<V: X3dhVault> Responder<V> {
             CURVE25519_SECRET_LENGTH,
         );
         if self.identity_key.is_none() {
-            self.identity_key = Some(self.vault.secret_generate(p_atts).await?);
+            self.identity_key = Zeroizing::new(Some(self.vault.secret_generate(p_atts).await?));
         }
-        self.signed_prekey = Some(self.vault.secret_generate(p_atts).await?);
-        self.one_time_prekey = Some(self.vault.secret_generate(e_atts).await?);
+        self.signed_prekey = Zeroizing::new(Some(self.vault.secret_generate(p_atts).await?));
+        self.one_time_prekey = Zeroizing::new(Some(self.vault.secret_generate(e_atts).await?));
         Ok(())
     }
 }
diff --git a/implementations/rust/ockam/ockam_key_exchange_xx/Cargo.toml b/implementations/rust/ockam/ockam_key_exchange_xx/Cargo.toml
index 239224ef..cc773ec9 100644
--- a/implementations/rust/ockam/ockam_key_exchange_xx/Cargo.toml
+++ b/implementations/rust/ockam/ockam_key_exchange_xx/Cargo.toml
@@ -30,6 +30,7 @@ alloc = ["ockam_core/alloc", "ockam_key_exchange_core/alloc", "ockam_vault_core/
 ockam_core = { path = "../ockam_core", version = "0.36.0", default_features = false }
 ockam_vault_core = { path = "../ockam_vault_core", version = "0.30.0", default_features = false }
 ockam_key_exchange_core = { path = "../ockam_key_exchange_core", version = "0.28.0", default_features = false }
+zeroize = { version = "1.4.2", features = ["zeroize_derive"] }
 
 [dev-dependencies]
 ockam_vault_sync_core = { path = "../ockam_vault_sync_core", version = "0.27.0"         }
diff --git a/implementations/rust/ockam/ockam_key_exchange_xx/src/state.rs b/implementations/rust/ockam/ockam_key_exchange_xx/src/state.rs
index 58803f2c..038cdbc0 100644
--- a/implementations/rust/ockam/ockam_key_exchange_xx/src/state.rs
+++ b/implementations/rust/ockam/ockam_key_exchange_xx/src/state.rs
@@ -395,6 +395,7 @@ mod tests {
         CURVE25519_SECRET_LENGTH,
     };
     use ockam_vault_sync_core::VaultSync;
+    use zeroize::Zeroizing;
 
     #[test]
     fn prologue() {
@@ -705,8 +706,8 @@ mod tests {
             _remote_static_public_key: None,
             remote_ephemeral_public_key: None,
             dh_state: DhState {
-                key: None,
-                ck: Some(ck),
+                key: Zeroizing::new(None),
+                ck: Zeroizing::new(Some(ck)),
                 vault: vault.async_try_clone().await.unwrap(),
             },
             nonce: 0,
diff --git a/implementations/rust/ockam/ockam_key_exchange_xx/src/state/dh_state.rs b/implementations/rust/ockam/ockam_key_exchange_xx/src/state/dh_state.rs
index 78379e75..266d7bb5 100644
--- a/implementations/rust/ockam/ockam_key_exchange_xx/src/state/dh_state.rs
+++ b/implementations/rust/ockam/ockam_key_exchange_xx/src/state/dh_state.rs
@@ -3,18 +3,19 @@ use ockam_core::Result;
 use ockam_vault_core::{
     PublicKey, Secret, SecretAttributes, SecretPersistence, SecretType, AES256_SECRET_LENGTH,
 };
+use zeroize::Zeroizing;
 
 pub(crate) struct DhState<V: XXVault> {
-    pub(crate) key: Option<Secret>,
-    pub(crate) ck: Option<Secret>,
+    pub(crate) key: Zeroizing<Option<Secret>>,
+    pub(crate) ck: Zeroizing<Option<Secret>>,
     pub(crate) vault: V,
 }
 
 impl<V: XXVault> DhState<V> {
     pub(crate) fn empty(vault: V) -> Self {
         Self {
-            key: None,
-            ck: None,
+            key: Zeroizing::new(None),
+            ck: Zeroizing::new(None),
             vault,
         }
     }
@@ -29,8 +30,8 @@ impl<V: XXVault> DhState<V> {
         let ck = vault.secret_import(protocol_name, attributes).await?;
 
         Ok(Self {
-            key: None,
-            ck: Some(ck),
+            key: Zeroizing::new(None),
+            ck: Zeroizing::new(Some(ck)),
             vault,
         })
     }
@@ -90,12 +91,12 @@ impl<V: XXVault> DhState<V> {
             self.vault.secret_destroy(key.unwrap()).await?;
         }
 
-        self.key = Some(hkdf_output.pop().unwrap());
+        self.key = Zeroizing::new(Some(hkdf_output.pop().unwrap()));
 
         let ck = self.ck.take();
 
         self.vault.secret_destroy(ck.unwrap()).await?;
-        self.ck = Some(hkdf_output.pop().unwrap());
+        self.ck = Zeroizing::new(Some(hkdf_output.pop().unwrap()));
 
         Ok(())
     }

[mrinalwadhwa]

Hey @birkett83 👋.

We would love to merge your patch, but we do require a signed CLA and signed commits.

Please consider creating a key and sending a PR ... here's a guide from github that may be helpful. Some of us use Krypton for code signing, which is really quick and convenient.

Thank you.

[selectiveduplicate]

Hi @mrinalwadhwa. I can take this up if it's not being worked on anymore.

[SanjoDeundiak]

Hi @mrinalwadhwa. I can take this up if it's not being worked on anymore.

Hey @selectiveduplicate , you can take this. Also, I was thinking that maybe better approach to start with, is to create dedicated types for data that is returned from vault. Those structs can implement Zeroize (and run zeroize on drop). This should be much cleaner that using Zeroizing wrappers

[selectiveduplicate]

Hi @mrinalwadhwa. I can take this up if it's not being worked on anymore.

Hey @selectiveduplicate , you can take this. Also, I was thinking that maybe better approach to start with, is to create dedicated types for data that is returned from vault. Those structs can implement Zeroize (and run zeroize on drop). This should be much cleaner that using Zeroizing wrappers

Awesome! I'll start working on it. If I have questions, I can ping on this discussion thread, or here if that's suitable.

[selectiveduplicate]

Update 1 I'm looking at ockam_key_exchange_x3dh\src\lib.rs, and as far I can understand, the Signature and PreKeyBundle struct can have Zeroize. All the members of the Initiator and Responder structs seem to have Zeroize on them.

This is the relevant diff:

/* snip */

--- a/implementations/rust/ockam/ockam_key_exchange_x3dh/Cargo.toml
+++ b/implementations/rust/ockam/ockam_key_exchange_x3dh/Cargo.toml
@@ -31,6 +31,7 @@ ockam_core = { path = "../ockam_core", version = "^0.39.0", default_features = f
 ockam_vault_core = { path = "../ockam_vault_core" , version = "^0.33.0", default_features = false }
 ockam_key_exchange_core = { path = "../ockam_key_exchange_core" , version = "^0.31.0", default_features = false }
 arrayref = "0.3"
+zeroize = { version = "1.4.2", features = ["zeroize_derive"] }

 [dev-dependencies]
 ockam_vault_sync_core = { path = "../ockam_vault_sync_core", version = "^0.31.0"}
diff --git a/implementations/rust/ockam/ockam_key_exchange_x3dh/src/lib.rs b/implementations/rust/ockam/ockam_key_exchange_x3dh/src/lib.rs
index ed4144db..228125c2 100644
--- a/implementations/rust/ockam/ockam_key_exchange_x3dh/src/lib.rs
+++ b/implementations/rust/ockam/ockam_key_exchange_x3dh/src/lib.rs
@@ -12,6 +12,7 @@ use ockam_core::{compat::vec::Vec, hex::encode, AsyncTryClone};
 use ockam_vault_core::{
     AsymmetricVault, Hasher, PublicKey, SecretVault, Signer, SymmetricVault, Verifier,
 };
+use zeroize::Zeroize;

 mod error;
 pub use error::*;
@@ -25,7 +26,8 @@ pub use new_key_exchanger::*;

 /// Represents and (X)EdDSA or ECDSA signature
 /// from Ed25519 or P-256
-#[derive(Clone, Copy)]
+#[derive(Clone, Zeroize)]
+#[zeroize(drop)]
 pub struct Signature([u8; 64]);

 impl AsRef<[u8; 64]> for Signature {
@@ -53,7 +55,8 @@ impl core::fmt::Debug for Signature {
 }

 /// Represents all the keys and signature to send to an enrollee
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Zeroize)]
+#[zeroize(drop)]
 pub struct PreKeyBundle {
     identity_key: PublicKey,
     signed_prekey: PublicKey,

Kindly let me know if I missed something whenever you get some time. I haven't yet looked at the other protocol; I'll continue working on it.

[SanjoDeundiak]

@selectiveduplicate looks great so far! Please, submit a PR

[etorreborre]

@SanjoDeundiak @mrinalwadhwa it seems that this issue was fixed with this PR: https://github.com/build-trust/ockam/pull/2265. Can you please confirm?

[SanjoDeundiak]

I'm not 100% sure there aren't places we should had used Zeroize. Requires thorough review