Skip to content

Commit e6b15dc

Browse files
authored
Fix sensitive leak after enabling opt-z (#360)
## 🎟️ Tracking <!-- Paste the link to the Jira or GitHub issue or otherwise describe / point to where this change is coming from. --> ## 📔 Objective Discovered in #356, the recent PR #353 has made some [sensitive memory leak tests fail](https://github.com/bitwarden/sdk-internal/actions/runs/16499844362/job/46655074711?pr=356). I've had to do two things to fix this: - In the `argon2` implementation, we used to hash into a fixed size array, and then box it. I've rewritten this code to store the value into the box directly, avoiding stack copies. This fixes the `Master Key Argon2 / Key` test case. - I've set the opt level for the crypto crate specifically to level 3, overriding the z level from the workspace. This is to ensure the compiler is able to inline functions as needed to avoid stack copies of values. This increases the WASM bundle size by around 0.1MB, but seems worth it in this case, as the code in bitwarden-crypto is more performance and security sensitive than the rest of the application. This fixes the `Master Key Argon2 / Hash bytes` test case. ## ⏰ Reminders before review - Contributor guidelines followed - All formatters and local linters executed and passed - Written new unit and / or integration tests where applicable - Protected functional changes with optionality (feature flags) - Used internationalization (i18n) for all UI strings - CI builds passed - Communicated to DevOps any deployment requirements - Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team ## 🦮 Reviewer guidelines <!-- Suggested interactions but feel free to use (or not) as you desire! --> - 👍 (`:+1:`) or similar for great changes - 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info - ❓ (`:question:`) for questions - 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion - 🎨 (`:art:`) for suggestions / improvements - ❌ (`:x:`) or ⚠️ (`:warning:`) for more significant problems or concerns needing attention - 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or indications of technical debt - ⛏ (`:pick:`) for minor or nitpick changes
1 parent 9f58e91 commit e6b15dc

File tree

2 files changed

+19
-13
lines changed

2 files changed

+19
-13
lines changed

Cargo.toml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,12 @@ codegen-units = 1
109109
lto = true
110110
opt-level = "z"
111111

112+
# Enable optimization for the bitwarden-crypto crate. This will increase the binary size slightly (~0.1MB),
113+
# but it will more aggressively inline functions. This will help us avoid extra stack copies of keys and
114+
# other sensitive values being left behind without cleanup.
115+
[profile.release.package.bitwarden-crypto]
116+
opt-level = 3
117+
112118
# Stripping the binary reduces the size by ~30%, but the stacktraces won't be usable anymore.
113119
# This is fine as long as we don't have any unhandled panics, but let's keep it disabled for now
114120
# strip = true

crates/bitwarden-crypto/src/keys/kdf.rs

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,18 @@ impl KdfDerivedKeyMaterial {
3434
salt: &[u8],
3535
kdf: &Kdf,
3636
) -> Result<Self, CryptoError> {
37-
let mut hash = match kdf {
37+
match kdf {
3838
Kdf::PBKDF2 { iterations } => {
3939
let iterations = iterations.get();
4040
if iterations < PBKDF2_MIN_ITERATIONS {
4141
return Err(CryptoError::InsufficientKdfParameters);
4242
}
4343

44-
crate::util::pbkdf2(secret, salt, iterations)
44+
let mut hash = crate::util::pbkdf2(secret, salt, iterations);
45+
46+
let key_material = Box::pin(hash.into());
47+
hash.zeroize();
48+
Ok(KdfDerivedKeyMaterial(key_material))
4549
}
4650
Kdf::Argon2id {
4751
iterations,
@@ -59,15 +63,14 @@ impl KdfDerivedKeyMaterial {
5963
return Err(CryptoError::InsufficientKdfParameters);
6064
}
6165

62-
use argon2::*;
66+
let salt_sha = sha2::Sha256::new().chain_update(salt).finalize();
6367

68+
let mut hash = Box::pin(GenericArray::<u8, U32>::default());
69+
70+
use argon2::*;
6471
let params = Params::new(memory, iterations, parallelism, Some(32))?;
6572
let argon = Argon2::new(Algorithm::Argon2id, Version::V0x13, params);
66-
67-
let salt_sha = sha2::Sha256::new().chain_update(salt).finalize();
68-
69-
let mut hash = [0u8; 32];
70-
argon.hash_password_into(secret, &salt_sha, &mut hash)?;
73+
argon.hash_password_into(secret, &salt_sha, hash.as_mut_slice())?;
7174

7275
// Argon2 is using some stack memory that is not zeroed. Eventually some function
7376
// will overwrite the stack, but we use this trick to force the used
@@ -78,12 +81,9 @@ impl KdfDerivedKeyMaterial {
7881
}
7982
clear_stack();
8083

81-
hash
84+
Ok(KdfDerivedKeyMaterial(hash))
8285
}
83-
};
84-
let key_material = Box::pin(GenericArray::clone_from_slice(&hash));
85-
hash.zeroize();
86-
Ok(KdfDerivedKeyMaterial(key_material))
86+
}
8787
}
8888

8989
/// Derives a users master key from their password, email and KDF.

0 commit comments

Comments
 (0)