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.

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

Mark generate_random_bytes to return sensitive #726

Merged
merged 3 commits into from
Apr 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
"totp",
"uniffi",
"wordlist",
"Zeroize",
"zxcvbn"
]
}
27 changes: 19 additions & 8 deletions crates/bitwarden-crypto/src/keys/shareable_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,47 +3,58 @@ use std::pin::Pin;
use aes::cipher::typenum::U64;
use generic_array::GenericArray;
use hmac::Mac;
use zeroize::Zeroize;

use crate::{
keys::SymmetricCryptoKey,
util::{hkdf_expand, PbkdfSha256Hmac},
Sensitive,
};

/// Derive a shareable key using hkdf from secret and name.
///
/// A specialized variant of this function was called `CryptoService.makeSendKey` in the Bitwarden
/// `clients` repository.
pub fn derive_shareable_key(
secret: [u8; 16],
secret: Sensitive<[u8; 16]>,
name: &str,
info: Option<&str>,
) -> SymmetricCryptoKey {
// Because all inputs are fixed size, we can unwrap all errors here without issue

// TODO: Are these the final `key` and `info` parameters or should we change them? I followed
// the pattern used for sends
let res = PbkdfSha256Hmac::new_from_slice(format!("bitwarden-{}", name).as_bytes())
let mut res = PbkdfSha256Hmac::new_from_slice(format!("bitwarden-{}", name).as_bytes())
.expect("hmac new_from_slice should not fail")
.chain_update(secret)
.chain_update(secret.expose())
.finalize()
.into_bytes();

let mut key: Pin<Box<GenericArray<u8, U64>>> =
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")
}

#[cfg(test)]
mod tests {
use super::derive_shareable_key;
use crate::Sensitive;

#[test]
fn test_derive_shareable_key() {
let key = derive_shareable_key(*b"&/$%F1a895g67HlX", "test_key", None);
let key = derive_shareable_key(
Sensitive::new(Box::new(*b"&/$%F1a895g67HlX")),
"test_key",
None,
);
assert_eq!(key.to_base64().expose(), "4PV6+PcmF2w7YHRatvyMcVQtI7zvCyssv/wFWmzjiH6Iv9altjmDkuBD1aagLVaLezbthbSe+ktR+U6qswxNnQ==");

let key = derive_shareable_key(*b"67t9b5g67$%Dh89n", "test_key", Some("test"));
let key = derive_shareable_key(
Sensitive::new(Box::new(*b"67t9b5g67$%Dh89n")),
"test_key",
Some("test"),
);
assert_eq!(key.to_base64().expose(), "F9jVQmrACGx9VUPjuzfMYDjr726JtL300Y3Yg+VYUnVQtQ1s8oImJ5xtp1KALC9h2nav04++1LDW4iFD+infng==");
}
}
4 changes: 2 additions & 2 deletions crates/bitwarden-crypto/src/keys/symmetric_crypto_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,9 @@ impl std::fmt::Debug for SymmetricCryptoKey {

#[cfg(test)]
pub fn derive_symmetric_key(name: &str) -> SymmetricCryptoKey {
use crate::{derive_shareable_key, generate_random_bytes};
use crate::{derive_shareable_key, generate_random_bytes, Sensitive};

let secret: [u8; 16] = generate_random_bytes();
let secret: Sensitive<[u8; 16]> = generate_random_bytes();
derive_shareable_key(secret, name, None)
}

Expand Down
7 changes: 7 additions & 0 deletions crates/bitwarden-crypto/src/sensitive/sensitive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,13 @@ impl<V: Zeroize> Sensitive<V> {
}
}

/// Helper to convert a `Sensitive<[u8, N]>` to a `SensitiveVec`.
impl<const N: usize> From<Sensitive<[u8; N]>> for SensitiveVec {
fn from(sensitive: Sensitive<[u8; N]>) -> Self {
SensitiveVec::new(Box::new(sensitive.value.to_vec()))
}
}

/// Helper to convert a `Sensitive<Vec<u8>>` to a `Sensitive<String>`, care is taken to ensure any
/// intermediate copies are zeroed to avoid leaking sensitive data.
impl TryFrom<SensitiveVec> for SensitiveString {
Expand Down
8 changes: 5 additions & 3 deletions crates/bitwarden-crypto/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ use rand::{
distributions::{Distribution, Standard},
Rng,
};
use zeroize::Zeroize;

use crate::{CryptoError, Result};
use crate::{CryptoError, Result, Sensitive};

pub(crate) type PbkdfSha256Hmac = hmac::Hmac<sha2::Sha256>;
pub(crate) const PBKDF_SHA256_HMAC_OUT_SIZE: usize =
Expand All @@ -30,11 +31,12 @@ pub(crate) fn hkdf_expand<T: ArrayLength<u8>>(
}

/// Generate random bytes that are cryptographically secure
pub fn generate_random_bytes<T>() -> T
pub fn generate_random_bytes<T>() -> Sensitive<T>
where
Standard: Distribution<T>,
T: Zeroize,
{
rand::thread_rng().gen()
Sensitive::new(Box::new(rand::thread_rng().gen::<T>()))
}

pub fn pbkdf2(password: &[u8], salt: &[u8], rounds: u32) -> [u8; PBKDF_SHA256_HMAC_OUT_SIZE] {
Expand Down
14 changes: 8 additions & 6 deletions crates/bitwarden-exporters/src/encrypted_json.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use base64::{engine::general_purpose::STANDARD, Engine};
use bitwarden_crypto::{generate_random_bytes, Kdf, KeyEncryptable, PinKey};
use base64::engine::general_purpose::STANDARD;
use bitwarden_crypto::{
generate_random_bytes, Kdf, KeyEncryptable, PinKey, Sensitive, SensitiveVec,
};
use serde::Serialize;
use thiserror::Error;
use uuid::Uuid;
Expand Down Expand Up @@ -43,16 +45,16 @@ pub(crate) fn export_encrypted_json(
),
};

let salt: [u8; 16] = generate_random_bytes();
let salt = STANDARD.encode(salt);
let key = PinKey::derive(password.as_bytes(), salt.as_bytes(), &kdf)?;
let salt: Sensitive<[u8; 16]> = generate_random_bytes();
let salt = SensitiveVec::from(salt).encode_base64(STANDARD);
let key = PinKey::derive(password.as_bytes(), salt.expose().as_bytes(), &kdf)?;

let enc_key_validation = Uuid::new_v4().to_string();

let encrypted_export = EncryptedJsonExport {
encrypted: true,
password_protected: true,
salt,
salt: salt.expose().to_string(),
kdf_type,
kdf_iterations,
kdf_memory,
Expand Down
6 changes: 3 additions & 3 deletions crates/bitwarden/src/auth/access_token.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{fmt::Debug, str::FromStr};

use base64::Engine;
use bitwarden_crypto::{derive_shareable_key, SymmetricCryptoKey};
use bitwarden_crypto::{derive_shareable_key, Sensitive, SymmetricCryptoKey};
use uuid::Uuid;

use crate::{error::AccessTokenInvalidError, util::STANDARD_INDIFFERENT};
Expand Down Expand Up @@ -45,12 +45,12 @@ impl FromStr for AccessToken {
let encryption_key = STANDARD_INDIFFERENT
.decode(encryption_key)
.map_err(AccessTokenInvalidError::InvalidBase64)?;
let encryption_key: [u8; 16] = encryption_key.try_into().map_err(|e: Vec<_>| {
let encryption_key = Sensitive::new(encryption_key.try_into().map_err(|e: Vec<_>| {
AccessTokenInvalidError::InvalidBase64Length {
expected: 16,
got: e.len(),
}
})?;
})?);
let encryption_key =
derive_shareable_key(encryption_key, "accesstoken", Some("sm-access-token"));

Expand Down
10 changes: 6 additions & 4 deletions crates/bitwarden/src/vault/send.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use base64::{
use bitwarden_api_api::models::{SendFileModel, SendResponseModel, SendTextModel};
use bitwarden_crypto::{
derive_shareable_key, generate_random_bytes, CryptoError, EncString, KeyDecryptable,
KeyEncryptable, LocateKey, SymmetricCryptoKey,
KeyEncryptable, LocateKey, Sensitive, SensitiveVec, SymmetricCryptoKey,
};
use chrono::{DateTime, Utc};
use schemars::JsonSchema;
Expand Down Expand Up @@ -149,7 +149,9 @@ impl Send {
}

fn derive_shareable_key(key: &[u8]) -> Result<SymmetricCryptoKey, CryptoError> {
let key = key.try_into().map_err(|_| CryptoError::InvalidKeyLen)?;
let key = Sensitive::new(Box::new(
key.try_into().map_err(|_| CryptoError::InvalidKeyLen)?,
));
Ok(derive_shareable_key(key, "send", Some("send")))
}
}
Expand Down Expand Up @@ -265,8 +267,8 @@ impl KeyEncryptable<SymmetricCryptoKey, Send> for SendView {
.map_err(|_| CryptoError::InvalidKey)?,
// New send, generate random key
(None, None) => {
let key: [u8; 16] = generate_random_bytes();
key.to_vec()
let key: Sensitive<[u8; 16]> = generate_random_bytes();
SensitiveVec::from(key).expose().to_owned()
}
// Existing send without key
_ => return Err(CryptoError::InvalidKey),
Expand Down
Loading