From 7ff270d976d89c4467123bddf24aa579d5d37a2b Mon Sep 17 00:00:00 2001 From: Aaron Feickert <66188213+AaronFeickert@users.noreply.github.com> Date: Mon, 23 Sep 2024 03:43:44 -0500 Subject: [PATCH] Improve zeroization of key buffer (#1069) Currently, the temporary buffer used for deriving shareable keys is [manually zeroized](https://github.com/bitwarden/sdk/blob/76417172489d5790babbe14bb8c6ad8b3aac2a33/crates/bitwarden-crypto/src/keys/shareable_key.rs#L32-L33). While documentation indicates that preceding `expect` calls cannot fail, this still seems brittle to future changes. This PR places the buffer into a `Zeroizing` wrapper. It is still the case that zeroization may not occur on a panic, but this was already the case with the existing implementation, which would never zeroize in such a case. --- .../bitwarden-crypto/src/keys/shareable_key.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/crates/bitwarden-crypto/src/keys/shareable_key.rs b/crates/bitwarden-crypto/src/keys/shareable_key.rs index 556432aea..c6a44405c 100644 --- a/crates/bitwarden-crypto/src/keys/shareable_key.rs +++ b/crates/bitwarden-crypto/src/keys/shareable_key.rs @@ -3,7 +3,7 @@ use std::pin::Pin; use aes::cipher::typenum::U64; use generic_array::GenericArray; use hmac::Mac; -use zeroize::{Zeroize, Zeroizing}; +use zeroize::Zeroizing; use crate::{ keys::SymmetricCryptoKey, @@ -20,18 +20,17 @@ pub fn derive_shareable_key( info: Option<&str>, ) -> SymmetricCryptoKey { // Because all inputs are fixed size, we can unwrap all errors here without issue - let mut res = PbkdfSha256Hmac::new_from_slice(format!("bitwarden-{}", name).as_bytes()) - .expect("hmac new_from_slice should not fail") - .chain_update(secret) - .finalize() - .into_bytes(); + let res = Zeroizing::new( + PbkdfSha256Hmac::new_from_slice(format!("bitwarden-{}", name).as_bytes()) + .expect("hmac new_from_slice should not fail") + .chain_update(secret) + .finalize() + .into_bytes(), + ); let mut key: Pin>> = hkdf_expand(&res, info).expect("Input is a valid size"); - // Zeroize the temporary buffer - res.zeroize(); - SymmetricCryptoKey::try_from(key.as_mut_slice()).expect("Key is a valid size") }