Skip to content

Commit

Permalink
Fix make_user_key which previously didn't stretch the users key (#315)
Browse files Browse the repository at this point in the history
## Type of change

<!-- (mark with an `X`) -->

```
- [x] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other
```

## Objective

<!--Describe what the purpose of this PR is. For example: what bug
you're fixing or what new feature you're adding-->

The `make_user_key` needs to use the same key as `decrypt_user_key`,
which should be the stretched users key.

## Before you submit

- Please add **unit tests** where it makes sense to do so (encouraged
but not required)
  • Loading branch information
Hinton authored Nov 1, 2023
1 parent 6754c7d commit 1c86c09
Show file tree
Hide file tree
Showing 7 changed files with 111 additions and 19 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/bitwarden/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -59,5 +59,6 @@ bitwarden-api-identity = { path = "../bitwarden-api-identity", version = "=0.2.2
bitwarden-api-api = { path = "../bitwarden-api-api", version = "=0.2.2" }

[dev-dependencies]
rand_chacha = "0.3.1"
tokio = { version = "1.28.2", features = ["rt", "macros"] }
wiremock = "0.5.18"
6 changes: 3 additions & 3 deletions crates/bitwarden/src/auth/register.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ pub(super) fn make_register_keys(

#[cfg_attr(feature = "mobile", derive(uniffi::Record))]
pub struct RegisterKeyResponse {
master_password_hash: String,
encrypted_user_key: String,
keys: RsaKeyPair,
pub master_password_hash: String,
pub encrypted_user_key: String,
pub keys: RsaKeyPair,
}
10 changes: 5 additions & 5 deletions crates/bitwarden/src/client/client.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
use std::time::{Duration, Instant};

use reqwest::header::{self};
use uuid::Uuid;

#[cfg(feature = "secrets")]
use crate::auth::login::{access_token_login, AccessTokenLoginRequest, AccessTokenLoginResponse};
#[cfg(feature = "internal")]
use crate::{
auth::login::{
Expand All @@ -13,11 +18,6 @@ use crate::{
SecretVerificationRequest, SyncRequest, SyncResponse, UserApiKeyResponse,
},
};
use reqwest::header::{self};
use uuid::Uuid;

#[cfg(feature = "secrets")]
use crate::auth::login::{access_token_login, AccessTokenLoginRequest, AccessTokenLoginResponse};
use crate::{
auth::renew::renew_token,
client::{
Expand Down
75 changes: 65 additions & 10 deletions crates/bitwarden/src/crypto/master_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ use rand::Rng;
use sha2::Digest;

use super::{
encrypt_aes256, hkdf_expand, EncString, KeyDecryptable, PbkdfSha256Hmac, SymmetricCryptoKey,
UserKey, PBKDF_SHA256_HMAC_OUT_SIZE,
encrypt_aes256_hmac, hkdf_expand, EncString, KeyDecryptable, PbkdfSha256Hmac,
SymmetricCryptoKey, UserKey, PBKDF_SHA256_HMAC_OUT_SIZE,
};
use crate::{client::kdf::Kdf, error::Result, util::BASE64_ENGINE};

Expand Down Expand Up @@ -41,13 +41,7 @@ impl MasterKey {
}

pub(crate) fn make_user_key(&self) -> Result<(UserKey, EncString)> {
let mut user_key = [0u8; 64];
rand::thread_rng().fill(&mut user_key);

let protected = encrypt_aes256(&user_key, self.0.key)?;

let u: &[u8] = &user_key;
Ok((UserKey::new(SymmetricCryptoKey::try_from(u)?), protected))
make_user_key(rand::thread_rng(), self)
}

pub(crate) fn decrypt_user_key(&self, user_key: EncString) -> Result<SymmetricCryptoKey> {
Expand All @@ -58,6 +52,22 @@ impl MasterKey {
}
}

/// Generate a new random user key and encrypt it with the master key.
fn make_user_key(
mut rng: impl rand::RngCore,
master_key: &MasterKey,
) -> Result<(UserKey, EncString)> {
let mut user_key = [0u8; 64];
rng.fill(&mut user_key);

let stretched_key = stretch_master_key(master_key)?;
let protected =
encrypt_aes256_hmac(&user_key, stretched_key.mac_key.unwrap(), stretched_key.key)?;

let u: &[u8] = &user_key;
Ok((UserKey::new(SymmetricCryptoKey::try_from(u)?), protected))
}

/// Derive a generic key from a secret and salt using the provided KDF.
fn derive_key(secret: &[u8], salt: &[u8], kdf: &Kdf) -> Result<SymmetricCryptoKey> {
let hash = match kdf {
Expand Down Expand Up @@ -112,7 +122,9 @@ fn stretch_master_key(master_key: &MasterKey) -> Result<SymmetricCryptoKey> {
mod tests {
use std::num::NonZeroU32;

use super::{stretch_master_key, HashPurpose, MasterKey};
use rand::SeedableRng;

use super::{make_user_key, stretch_master_key, HashPurpose, MasterKey};
use crate::{client::kdf::Kdf, crypto::SymmetricCryptoKey};

#[test]
Expand Down Expand Up @@ -225,4 +237,47 @@ mod tests {
.unwrap(),
);
}

#[test]
fn test_make_user_key() {
let mut rng = rand_chacha::ChaCha8Rng::from_seed([0u8; 32]);

let master_key = MasterKey(SymmetricCryptoKey {
key: [
31, 79, 104, 226, 150, 71, 177, 90, 194, 80, 172, 209, 17, 129, 132, 81, 138, 167,
69, 167, 254, 149, 2, 27, 39, 197, 64, 42, 22, 195, 86, 75,
]
.into(),
mac_key: None,
});

let (user_key, protected) = make_user_key(&mut rng, &master_key).unwrap();

assert_eq!(
user_key.0.key.as_slice(),
[
62, 0, 239, 47, 137, 95, 64, 214, 127, 91, 184, 232, 31, 9, 165, 161, 44, 132, 14,
195, 206, 154, 127, 59, 24, 27, 225, 136, 239, 113, 26, 30
]
);
assert_eq!(
user_key.0.mac_key.unwrap().as_slice(),
[
152, 76, 225, 114, 185, 33, 111, 65, 159, 68, 83, 103, 69, 109, 86, 25, 49, 74, 66,
163, 218, 134, 176, 1, 56, 123, 253, 184, 14, 12, 254, 66
]
);

// Ensure we can decrypt the key and get back the same key
let decrypted = master_key.decrypt_user_key(protected).unwrap();

assert_eq!(
decrypted.key, user_key.0.key,
"Decrypted key doesn't match user key"
);
assert_eq!(
decrypted.mac_key, user_key.0.mac_key,
"Decrypted key doesn't match user key"
);
}
}
2 changes: 1 addition & 1 deletion crates/bitwarden/src/crypto/user_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::{
error::Result,
};

pub(crate) struct UserKey(SymmetricCryptoKey);
pub(crate) struct UserKey(pub(super) SymmetricCryptoKey);

impl UserKey {
pub(crate) fn new(key: SymmetricCryptoKey) -> Self {
Expand Down
35 changes: 35 additions & 0 deletions crates/bitwarden/tests/register.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/// Integration test for registering a new user and unlocking the vault
#[cfg(feature = "mobile")]
#[tokio::test]
async fn test_register_initialize_crypto() {
use std::num::NonZeroU32;

use bitwarden::{client::kdf::Kdf, mobile::crypto::InitCryptoRequest, Client};

let mut client = Client::new(None);

let email = "[email protected]";
let password = "test123";
let kdf = Kdf::PBKDF2 {
iterations: NonZeroU32::new(600_000).unwrap(),
};

let register_response = client
.auth()
.make_register_keys(email.to_owned(), password.to_owned(), kdf.clone())
.unwrap();

// Ensure we can initialize the crypto with the new keys
client
.crypto()
.initialize_crypto(InitCryptoRequest {
kdf_params: kdf,
email: email.to_owned(),
password: password.to_owned(),
user_key: register_response.encrypted_user_key,
private_key: register_response.keys.private.to_string(),
organization_keys: Default::default(),
})
.await
.unwrap();
}

0 comments on commit 1c86c09

Please sign in to comment.