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

[PM-5791] Change decrypt to return Sensitive #536

Merged
merged 29 commits into from
Apr 25, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
8c869f2
Explore introducing a Decrypted struct
Hinton Jan 19, 2024
74a5b08
Replace with std::mem::take
Hinton Jan 19, 2024
c57bea6
Merge branch 'main' of github.com:bitwarden/sdk into ps/zeroize-decry…
Hinton Jan 25, 2024
d5f2ae5
Document and introduce type alias
Hinton Jan 25, 2024
97ecc2c
Format
Hinton Jan 25, 2024
449466a
Add Decrypted to asymmetric
Hinton Jan 25, 2024
ed4d92b
Use Decrypt everywhere
Hinton Jan 25, 2024
18cf0d6
Use boxed value and fix tests
Hinton Jan 26, 2024
3d247ad
Merge branch 'main' of github.com:bitwarden/sdk into ps/zeroize-decry…
Hinton Jan 26, 2024
b848625
Resolve review feedback
Hinton Jan 29, 2024
d85fc9f
Extract a common sensitive type.
Hinton Jan 29, 2024
d7eedaa
Format
Hinton Jan 29, 2024
cb5f43a
Merge branch 'main' into ps/zeroize-decrypted
dani-garcia Mar 7, 2024
ba14e8c
Update exporters to use Sensitive
dani-garcia Mar 7, 2024
19fecae
Add support for base64 encode/decode for the Sensitive types
dani-garcia Mar 7, 2024
cece11e
Merge branch 'main' into ps/zeroize-decrypted
dani-garcia Mar 11, 2024
ef8ba37
Make base64 memory test required
dani-garcia Mar 11, 2024
621cf72
Merge branch 'main' of github.com:bitwarden/sdk into ps/zeroize-decry…
Hinton Apr 22, 2024
b87bd95
Use ::test in tests
Hinton Apr 22, 2024
37b18ad
Mark send key as sensitive
Hinton Apr 22, 2024
b2c3ff3
Merge branch 'main' of github.com:bitwarden/sdk into ps/zeroize-decry…
Hinton Apr 22, 2024
1f731a8
Remove accidentally duplicated generate_cipher
Hinton Apr 22, 2024
ef08b7c
Convert Send::derive_shareable_key to expect SensitiveVec
Hinton Apr 23, 2024
16ea5c3
Fix compile
Hinton Apr 23, 2024
31a02f8
Refactor sub_title to not expose temporary values
Hinton Apr 23, 2024
456f54f
Merge branch 'main' of github.com:bitwarden/sdk into ps/zeroize-decry…
Hinton Apr 23, 2024
0d40054
Shore up uri checksum
Hinton Apr 23, 2024
da787a2
Remove converter for DecryptedString
Hinton Apr 25, 2024
7baa6f1
Review feedback
Hinton Apr 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
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.

28 changes: 15 additions & 13 deletions crates/bitwarden-crypto/src/enc_string/asymmetric.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use super::{from_b64_vec, split_enc_string};
use crate::{
error::{CryptoError, EncStringParseError, Result},
rsa::encrypt_rsa2048_oaep_sha1,
AsymmetricCryptoKey, AsymmetricEncryptable, KeyDecryptable,
AsymmetricCryptoKey, AsymmetricEncryptable, DecryptedString, DecryptedVec, KeyDecryptable,
};

// This module is a workaround to avoid deprecated warnings that come from the ZeroizeOnDrop
Expand Down Expand Up @@ -166,8 +166,8 @@ impl AsymmetricEncString {
}
}

impl KeyDecryptable<AsymmetricCryptoKey, Vec<u8>> for AsymmetricEncString {
fn decrypt_with_key(&self, key: &AsymmetricCryptoKey) -> Result<Vec<u8>> {
impl KeyDecryptable<AsymmetricCryptoKey, DecryptedVec> for AsymmetricEncString {
fn decrypt_with_key(&self, key: &AsymmetricCryptoKey) -> Result<DecryptedVec> {
use AsymmetricEncString::*;
match self {
Rsa2048_OaepSha256_B64 { data } => key.key.decrypt(Oaep::new::<sha2::Sha256>(), data),
Expand All @@ -181,14 +181,15 @@ impl KeyDecryptable<AsymmetricCryptoKey, Vec<u8>> for AsymmetricEncString {
key.key.decrypt(Oaep::new::<sha1::Sha1>(), data)
}
}
.map(|v| DecryptedVec::new(Box::new(v)))
.map_err(|_| CryptoError::KeyDecrypt)
}
}

impl KeyDecryptable<AsymmetricCryptoKey, String> for AsymmetricEncString {
fn decrypt_with_key(&self, key: &AsymmetricCryptoKey) -> Result<String> {
let dec: Vec<u8> = self.decrypt_with_key(key)?;
String::from_utf8(dec).map_err(|_| CryptoError::InvalidUtf8String)
impl KeyDecryptable<AsymmetricCryptoKey, DecryptedString> for AsymmetricEncString {
fn decrypt_with_key(&self, key: &AsymmetricCryptoKey) -> Result<DecryptedString> {
let dec: DecryptedVec = self.decrypt_with_key(key)?;
dec.try_into()
}
}

Expand All @@ -209,6 +210,7 @@ mod tests {
use schemars::schema_for;

use super::{AsymmetricCryptoKey, AsymmetricEncString, KeyDecryptable};
use crate::DecryptedString;

const RSA_PRIVATE_KEY: &str = "-----BEGIN PRIVATE KEY-----
MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCXRVrCX+2hfOQS
Expand Down Expand Up @@ -247,8 +249,8 @@ XKZBokBGnjFnTnKcs7nv/O8=

assert_eq!(enc_string.enc_type(), 3);

let res: String = enc_string.decrypt_with_key(&private_key).unwrap();
assert_eq!(res, "EncryptMe!");
let res: DecryptedString = enc_string.decrypt_with_key(&private_key).unwrap();
assert_eq!(res.expose(), "EncryptMe!");
}

#[test]
Expand All @@ -259,8 +261,8 @@ XKZBokBGnjFnTnKcs7nv/O8=

assert_eq!(enc_string.enc_type(), 4);

let res: String = enc_string.decrypt_with_key(&private_key).unwrap();
assert_eq!(res, "EncryptMe!");
let res: DecryptedString = enc_string.decrypt_with_key(&private_key).unwrap();
assert_eq!(res.expose(), "EncryptMe!");
}

#[test]
Expand All @@ -271,8 +273,8 @@ XKZBokBGnjFnTnKcs7nv/O8=

assert_eq!(enc_string.enc_type(), 6);

let res: String = enc_string.decrypt_with_key(&private_key).unwrap();
assert_eq!(res, "EncryptMe!");
let res: DecryptedString = enc_string.decrypt_with_key(&private_key).unwrap();
assert_eq!(res.expose(), "EncryptMe!");
}

#[test]
Expand Down
57 changes: 37 additions & 20 deletions crates/bitwarden-crypto/src/enc_string/symmetric.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use serde::Deserialize;
use super::{check_length, from_b64, from_b64_vec, split_enc_string};
use crate::{
error::{CryptoError, EncStringParseError, Result},
KeyDecryptable, KeyEncryptable, LocateKey, SymmetricCryptoKey,
DecryptedString, DecryptedVec, KeyDecryptable, KeyEncryptable, LocateKey, SymmetricCryptoKey,
};

/// # Encrypted string primitive
Expand Down Expand Up @@ -233,11 +233,15 @@ impl KeyEncryptable<SymmetricCryptoKey, EncString> for &[u8] {
}
}

impl KeyDecryptable<SymmetricCryptoKey, Vec<u8>> for EncString {
fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<Vec<u8>> {
impl KeyDecryptable<SymmetricCryptoKey, DecryptedVec> for EncString {
fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<DecryptedVec> {
match self {
EncString::AesCbc256_B64 { iv, data } => {
let dec = crate::aes::decrypt_aes256(iv, data.clone(), &key.key)?;
let dec = DecryptedVec::new(Box::new(crate::aes::decrypt_aes256(
iv,
data.clone(),
&key.key,
)?));
Ok(dec)
}
EncString::AesCbc128_HmacSha256_B64 { iv, mac, data } => {
Expand All @@ -247,13 +251,24 @@ impl KeyDecryptable<SymmetricCryptoKey, Vec<u8>> for EncString {
// When refactoring the key handling, this should be fixed.
let enc_key = key.key[0..16].into();
let mac_key = key.key[16..32].into();
let dec = crate::aes::decrypt_aes128_hmac(iv, mac, data.clone(), mac_key, enc_key)?;
let dec = DecryptedVec::new(Box::new(crate::aes::decrypt_aes128_hmac(
iv,
mac,
data.clone(),
mac_key,
enc_key,
)?));
Ok(dec)
}
EncString::AesCbc256_HmacSha256_B64 { iv, mac, data } => {
let mac_key = key.mac_key.as_ref().ok_or(CryptoError::InvalidMac)?;
let dec =
crate::aes::decrypt_aes256_hmac(iv, mac, data.clone(), mac_key, &key.key)?;
let dec = DecryptedVec::new(Box::new(crate::aes::decrypt_aes256_hmac(
iv,
mac,
data.clone(),
mac_key,
&key.key,
)?));
Ok(dec)
}
}
Expand All @@ -266,10 +281,10 @@ impl KeyEncryptable<SymmetricCryptoKey, EncString> for String {
}
}

impl KeyDecryptable<SymmetricCryptoKey, String> for EncString {
fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<String> {
let dec: Vec<u8> = self.decrypt_with_key(key)?;
String::from_utf8(dec).map_err(|_| CryptoError::InvalidUtf8String)
impl KeyDecryptable<SymmetricCryptoKey, DecryptedString> for EncString {
fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<DecryptedString> {
let dec: DecryptedVec = self.decrypt_with_key(key)?;
dec.try_into()
}
}

Expand All @@ -290,17 +305,19 @@ mod tests {
use schemars::schema_for;

use super::EncString;
use crate::{derive_symmetric_key, KeyDecryptable, KeyEncryptable, SymmetricCryptoKey};
use crate::{
derive_symmetric_key, DecryptedString, KeyDecryptable, KeyEncryptable, SymmetricCryptoKey,
};

#[test]
fn test_enc_string_roundtrip() {
let key = derive_symmetric_key("test");

let test_string = "encrypted_test_string".to_string();
let cipher = test_string.clone().encrypt_with_key(&key).unwrap();
let test_string = "encrypted_test_string";
let cipher = test_string.to_owned().encrypt_with_key(&key).unwrap();

let decrypted_str: String = cipher.decrypt_with_key(&key).unwrap();
assert_eq!(decrypted_str, test_string);
let decrypted_str: DecryptedString = cipher.decrypt_with_key(&key).unwrap();
assert_eq!(decrypted_str.expose(), test_string);
}

#[test]
Expand Down Expand Up @@ -397,8 +414,8 @@ mod tests {
let enc_string: EncString = enc_str.parse().unwrap();
assert_eq!(enc_string.enc_type(), 0);

let dec_str: String = enc_string.decrypt_with_key(&key).unwrap();
assert_eq!(dec_str, "EncryptMe!");
let dec_str: DecryptedString = enc_string.decrypt_with_key(&key).unwrap();
assert_eq!(dec_str.expose(), "EncryptMe!");
}

#[test]
Expand All @@ -410,8 +427,8 @@ mod tests {
let enc_string: EncString = enc_str.parse().unwrap();
assert_eq!(enc_string.enc_type(), 1);

let dec_str: String = enc_string.decrypt_with_key(&key).unwrap();
assert_eq!(dec_str, "EncryptMe!");
let dec_str: DecryptedString = enc_string.decrypt_with_key(&key).unwrap();
assert_eq!(dec_str.expose(), "EncryptMe!");
}

#[test]
Expand Down
7 changes: 4 additions & 3 deletions crates/bitwarden-crypto/src/keys/asymmetric_crypto_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ mod tests {
use base64::{engine::general_purpose::STANDARD, Engine};

use crate::{
AsymmetricCryptoKey, AsymmetricEncString, AsymmetricPublicCryptoKey, KeyDecryptable,
AsymmetricCryptoKey, AsymmetricEncString, AsymmetricPublicCryptoKey, DecryptedString,
KeyDecryptable,
};

#[test]
Expand Down Expand Up @@ -218,8 +219,8 @@ DnqOsltgPomWZ7xVfMkm9niL2OA=
let encrypted =
AsymmetricEncString::encrypt_rsa2048_oaep_sha1(plaintext.as_bytes(), &public_key)
.unwrap();
let decrypted: String = encrypted.decrypt_with_key(&private_key).unwrap();
let decrypted: DecryptedString = encrypted.decrypt_with_key(&private_key).unwrap();

assert_eq!(plaintext, decrypted);
assert_eq!(plaintext, decrypted.expose());
}
}
12 changes: 7 additions & 5 deletions crates/bitwarden-crypto/src/keys/device_key.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::str::FromStr;

use crate::{
error::Result, AsymmetricCryptoKey, AsymmetricEncString, CryptoError, EncString,
error::Result, AsymmetricCryptoKey, AsymmetricEncString, CryptoError, DecryptedVec, EncString,
KeyDecryptable, KeyEncryptable, SymmetricCryptoKey,
};

Expand Down Expand Up @@ -64,11 +64,13 @@ impl DeviceKey {
protected_device_private_key: EncString,
protected_user_key: AsymmetricEncString,
) -> Result<SymmetricCryptoKey> {
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 device_private_key: DecryptedVec =
protected_device_private_key.decrypt_with_key(&self.0)?;
let device_private_key =
AsymmetricCryptoKey::from_der(device_private_key.expose().as_slice())?;

let mut dec: Vec<u8> = protected_user_key.decrypt_with_key(&device_private_key)?;
let user_key: SymmetricCryptoKey = dec.as_mut_slice().try_into()?;
let mut dec: DecryptedVec = protected_user_key.decrypt_with_key(&device_private_key)?;
let user_key: SymmetricCryptoKey = dec.expose_mut().as_mut_slice().try_into()?;

Ok(user_key)
}
Expand Down
10 changes: 6 additions & 4 deletions crates/bitwarden-crypto/src/keys/master_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ use base64::{engine::general_purpose::STANDARD, Engine};
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};

use super::utils::{derive_kdf_key, stretch_kdf_key};
use crate::{util, EncString, KeyDecryptable, Result, SymmetricCryptoKey, UserKey};
use crate::{
keys::utils::{derive_kdf_key, stretch_kdf_key},
util, DecryptedVec, EncString, KeyDecryptable, Result, SymmetricCryptoKey, UserKey,
};

#[derive(Serialize, Deserialize, Debug, JsonSchema, Clone)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
Expand Down Expand Up @@ -59,8 +61,8 @@ impl MasterKey {
pub fn decrypt_user_key(&self, user_key: EncString) -> Result<SymmetricCryptoKey> {
let stretched_key = stretch_kdf_key(&self.0)?;

let mut dec: Vec<u8> = user_key.decrypt_with_key(&stretched_key)?;
SymmetricCryptoKey::try_from(dec.as_mut_slice())
let mut dec: DecryptedVec = user_key.decrypt_with_key(&stretched_key)?;
SymmetricCryptoKey::try_from(dec.expose_mut().as_mut_slice())
}

pub fn encrypt_user_key(&self, user_key: &SymmetricCryptoKey) -> Result<EncString> {
Expand Down
2 changes: 1 addition & 1 deletion crates/bitwarden-crypto/src/keys/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
mod key_encryptable;
pub use key_encryptable::{KeyDecryptable, KeyEncryptable};
pub use key_encryptable::{CryptoKey, KeyDecryptable, KeyEncryptable};
mod master_key;
pub use master_key::{HashPurpose, Kdf, MasterKey};
mod shareable_key;
Expand Down
2 changes: 2 additions & 0 deletions crates/bitwarden-crypto/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ pub use util::generate_random_bytes;
mod wordlist;
pub use util::pbkdf2;
pub use wordlist::EFF_LONG_WORD_LIST;
mod sensitive;
pub use sensitive::*;

#[cfg(feature = "mobile")]
uniffi::setup_scaffolding!();
Expand Down
16 changes: 16 additions & 0 deletions crates/bitwarden-crypto/src/sensitive/decrypted.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
use zeroize::Zeroize;

use crate::{CryptoError, CryptoKey, KeyEncryptable, Sensitive};

/// Type alias for a [`Sensitive`] value to denote decrypted data.
pub type Decrypted<V> = Sensitive<V>;
pub type DecryptedVec = Decrypted<Vec<u8>>;
pub type DecryptedString = Decrypted<String>;

impl<T: KeyEncryptable<Key, Output> + Zeroize + Clone, Key: CryptoKey, Output>
KeyEncryptable<Key, Output> for Decrypted<T>
{
fn encrypt_with_key(self, key: &Key) -> Result<Output, CryptoError> {
self.value.clone().encrypt_with_key(key)
}
}
4 changes: 4 additions & 0 deletions crates/bitwarden-crypto/src/sensitive/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
mod sensitive;
Fixed Show fixed Hide fixed
pub use sensitive::{Sensitive, SensitiveString, SensitiveVec};
mod decrypted;
pub use decrypted::{Decrypted, DecryptedString, DecryptedVec};
Loading
Loading