Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-5728] Enable Zeroize support for keys and EncStrings #515

Merged
merged 12 commits into from
Jan 25, 2024
Merged

[PM-5728] Enable Zeroize support for keys and EncStrings #515

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
158308e
Enable Zeroize support for keys and EncStrings
dani-garcia Jan 16, 2024
73f77ce
Fix doclinks
dani-garcia Jan 19, 2024
76ee8c3
Fix review comments
dani-garcia Jan 22, 2024
c3d9d31
Merge branch 'main' into ps/PM-5728-zeroize
dani-garcia Jan 22, 2024
29c7e8a
Welp that segfaults on CI lol
dani-garcia Jan 22, 2024
1cc8b26
Review comments
dani-garcia Jan 23, 2024
c7f6b5a
Zeroize for SHA is on master but not the release :facepalm:
dani-garcia Jan 23, 2024
29e174d
Fmt
dani-garcia Jan 23, 2024
6142c0d
Merge branch 'main' into ps/PM-5728-zeroize
dani-garcia Jan 24, 2024
6e84446
Fix conflicts
dani-garcia Jan 24, 2024
9311349
Add mut comment
dani-garcia Jan 25, 2024
9fbb514
Merge branch 'main' into ps/PM-5728-zeroize
dani-garcia Jan 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/bitwarden-crypto/src/keys/asymmetric_crypto_key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@
// Note that RsaPrivateKey already implements ZeroizeOnDrop, so we don't need to do anything
// We add this assertion to make sure that this is still true in the future
const _: () = {
fn assert_zeroize_on_drop<T: zeroize::ZeroizeOnDrop>() {}
fn assert_all() {
assert_zeroize_on_drop::<RsaPrivateKey>();
}

Check warning on line 23 in crates/bitwarden-crypto/src/keys/asymmetric_crypto_key.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-crypto/src/keys/asymmetric_crypto_key.rs#L20-L23 

Added lines #L20 - L23 were not covered by tests
};

impl zeroize::ZeroizeOnDrop for AsymmetricCryptoKey {}
Expand All @@ -31,7 +31,7 @@
let bits = 2048;

Self {
key: RsaPrivateKey::new(rng, bits).expect("failed to generate a key"),
key: Box::pin(RsaPrivateKey::new(rng, bits).expect("failed to generate a key")),
}
}

Expand Down
8 changes: 4 additions & 4 deletions crates/bitwarden-crypto/src/keys/device_key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ impl DeviceKey {
let device_private_key: Vec<u8> = protected_device_private_key.decrypt_with_key(&self.0)?;
let device_private_key = AsymmetricCryptoKey::from_der(device_private_key.as_slice())?;

let dec: Vec<u8> = protected_user_key.decrypt_with_key(&device_private_key)?;
let user_key: SymmetricCryptoKey = dec.as_slice().try_into()?;
let mut dec: Vec<u8> = protected_user_key.decrypt_with_key(&device_private_key)?;
let user_key: SymmetricCryptoKey = dec.as_mut_slice().try_into()?;
Comment on lines +66 to +67
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this consumes the key. Do we need to zero dec?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We zeroize the key inside the TryFrom<&mut [u8]> impl:

https://github.com/bitwarden/sdk/blob/6e844469c9e1555a5abc45248ea88abcc025679b/crates/bitwarden-crypto/src/keys/symmetric_crypto_key.rs#L90-L115

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That feels potentially confusing? Do you expect a TryFrom of a slice to manipulate the input? I do see the benefit of ensuring it gets consumed though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also just pass an owned Vec<> and consume it in the function, but that will limit the use of the API for someone trying to pass a GenericArray or a [u8; 64] for example.

I think the fact that the function receives a &mut [u8] should make it clear enough that something is being modified, but we could add a comment mentioning explicitly that it's being zeroed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a comment to make it explicit. We can revaluate if this turns out to be confusing, potentially not using try from and instead having an explicit function could make it less confusing.


Ok(UserKey(user_key))
}
Expand Down Expand Up @@ -96,15 +96,15 @@ mod tests {
#[test]
fn test_decrypt_user_key() {
// Example keys from desktop app
let user_key: &[u8] = &[
let user_key: &mut [u8] = &mut [
109, 128, 172, 147, 206, 123, 134, 95, 16, 36, 155, 113, 201, 18, 186, 230, 216, 212,
173, 188, 74, 11, 134, 131, 137, 242, 105, 178, 105, 126, 52, 139, 248, 91, 215, 21,
128, 91, 226, 222, 165, 67, 251, 34, 83, 81, 77, 147, 225, 76, 13, 41, 102, 45, 183,
218, 106, 89, 254, 208, 251, 101, 130, 10,
];
let user_key = SymmetricCryptoKey::try_from(user_key).unwrap();

let key_data: &[u8] = &[
let key_data: &mut [u8] = &mut [
114, 235, 60, 115, 172, 156, 203, 145, 195, 130, 215, 250, 88, 146, 215, 230, 12, 109,
245, 222, 54, 217, 255, 211, 221, 105, 230, 236, 65, 52, 209, 133, 76, 208, 113, 254,
194, 216, 156, 19, 230, 62, 32, 93, 87, 7, 144, 156, 117, 142, 250, 32, 182, 118, 187,
Expand Down
Loading