Skip to content

Commit

Permalink
Replace SensitiveString with the new implementation
Browse files Browse the repository at this point in the history
  • Loading branch information
Hinton committed May 2, 2024
1 parent efdb8b2 commit b424a92
Show file tree
Hide file tree
Showing 26 changed files with 225 additions and 185 deletions.
9 changes: 8 additions & 1 deletion crates/bitwarden-crypto/src/enc_string/symmetric.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ use serde::Deserialize;
use super::{check_length, from_b64, from_b64_vec, split_enc_string};
use crate::{
error::{CryptoError, EncStringParseError, Result},
DecryptedString, DecryptedVec, KeyDecryptable, KeyEncryptable, LocateKey, SymmetricCryptoKey,
DecryptedString, DecryptedVec, KeyDecryptable, KeyEncryptable, LocateKey, SensitiveString,
SymmetricCryptoKey,
};

/// # Encrypted string primitive
Expand Down Expand Up @@ -281,6 +282,12 @@ impl KeyEncryptable<SymmetricCryptoKey, EncString> for String {
}
}

impl KeyEncryptable<SymmetricCryptoKey, EncString> for SensitiveString {
fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> crate::Result<EncString> {
self.as_str().as_bytes().encrypt_with_key(key)
}
}

impl KeyDecryptable<SymmetricCryptoKey, DecryptedString> for EncString {
fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<DecryptedString> {
let dec: DecryptedVec = self.decrypt_with_key(key)?;
Expand Down
2 changes: 1 addition & 1 deletion crates/bitwarden-crypto/src/keys/asymmetric_crypto_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ impl AsymmetricCryptoKey {
use rsa::pkcs8::DecodePrivateKey;
Ok(Self {
key: Box::pin(
RsaPrivateKey::from_pkcs8_pem(pem.expose()).map_err(|_| CryptoError::InvalidKey)?,
RsaPrivateKey::from_pkcs8_pem(pem.as_str()).map_err(|_| CryptoError::InvalidKey)?,
),
})
}
Expand Down
36 changes: 36 additions & 0 deletions crates/bitwarden-crypto/src/sensitive/base64.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
use zeroize::Zeroize;

use crate::{CryptoError, Sensitive, SensitiveString, SensitiveVec};

impl SensitiveString {
pub fn decode_base64<T: base64::Engine>(self, engine: T) -> Result<SensitiveVec, CryptoError> {
// Prevent accidental copies by allocating the full size
let len = base64::decoded_len_estimate(self.len());
let mut value = SensitiveVec::new(Box::new(Vec::with_capacity(len)));

engine
.decode_vec(self.as_str(), &mut value.value)
.map_err(|_| CryptoError::InvalidKey)?;

Ok(value)
}
}

impl<T: Zeroize + AsRef<[u8]>> Sensitive<T> {
pub fn encode_base64<E: base64::Engine>(self, engine: E) -> SensitiveString {
use base64::engine::Config;

let inner: &[u8] = self.value.as_ref().as_ref();

// Prevent accidental copies by allocating the full size
let padding = engine.config().encode_padding();
let len = base64::encoded_len(inner.len(), padding).expect("Valid length");

let mut value = SensitiveVec::new(Box::new(vec![0u8; len]));
engine
.encode_slice(inner, &mut value.value[..len])
.expect("Valid base64 string length");

value.try_into().expect("Valid base64 string")
}
}
4 changes: 2 additions & 2 deletions crates/bitwarden-crypto/src/sensitive/decrypted.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use zeroize::Zeroize;

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

/// 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>;
pub type DecryptedString = SensitiveString;

impl<T: KeyEncryptable<Key, Output> + Zeroize + Clone, Key: CryptoKey, Output>
KeyEncryptable<Key, Output> for Decrypted<T>
Expand Down
5 changes: 3 additions & 2 deletions crates/bitwarden-crypto/src/sensitive/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
#[allow(clippy::module_inception)]
mod sensitive;
pub use sensitive::{Sensitive, SensitiveString, SensitiveVec};
pub use sensitive::{Sensitive, SensitiveVec};
mod decrypted;
pub use decrypted::{Decrypted, DecryptedString, DecryptedVec};
mod string;
pub use string::BitString;
pub use string::SensitiveString;
mod base64;
91 changes: 3 additions & 88 deletions crates/bitwarden-crypto/src/sensitive/sensitive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use schemars::JsonSchema;
use serde::{Deserialize, Deserializer, Serialize, Serializer};
use zeroize::{Zeroize, ZeroizeOnDrop};

use super::string::SensitiveString;
use crate::CryptoError;

/// Wrapper for sensitive values which makes a best effort to enforce zeroization of the inner value
Expand All @@ -28,12 +29,6 @@ pub struct Sensitive<V: Zeroize> {
/// To avoid this, use Vec::with_capacity to preallocate the capacity you need.
pub type SensitiveVec = Sensitive<Vec<u8>>;

/// Important: This type does not protect against reallocations made by the String.
/// This means that if you insert any characters past the capacity, the data will be copied to a
/// new allocation and the old allocation will not be zeroized.
/// To avoid this, use String::with_capacity to preallocate the capacity you need.
pub type SensitiveString = Sensitive<String>;

impl<V: Zeroize> Sensitive<V> {
/// Create a new [`Sensitive`] value. In an attempt to avoid accidentally placing this on the
/// stack it only accepts a [`Box`] value. The rust compiler should be able to optimize away the
Expand Down Expand Up @@ -86,14 +81,7 @@ impl TryFrom<SensitiveVec> for SensitiveString {
let value = std::mem::take(&mut v.value);

let rtn = String::from_utf8(*value).map_err(|_| CryptoError::InvalidUtf8String);
rtn.map(|v| Sensitive::new(Box::new(v)))
}
}

impl From<SensitiveString> for SensitiveVec {
fn from(mut s: SensitiveString) -> Self {
let value = std::mem::take(&mut s.value);
Sensitive::new(Box::new(value.into_bytes()))
rtn.map(|v| SensitiveString::new(v))

Check failure

Code scanning / clippy

redundant closure Error

redundant closure
}
}

Expand All @@ -103,64 +91,6 @@ impl<N: ArrayLength<u8>> From<Sensitive<GenericArray<u8, N>>> for SensitiveVec {
}
}

impl SensitiveString {
pub fn decode_base64<T: base64::Engine>(self, engine: T) -> Result<SensitiveVec, CryptoError> {
// Prevent accidental copies by allocating the full size
let len = base64::decoded_len_estimate(self.value.len());
let mut value = SensitiveVec::new(Box::new(Vec::with_capacity(len)));

engine
.decode_vec(self.value.as_ref(), &mut value.value)
.map_err(|_| CryptoError::InvalidKey)?;

Ok(value)
}

#[inline(always)]
pub fn len(&self) -> usize {
self.value.len()
}

#[inline(always)]
pub fn is_empty(&self) -> bool {
self.value.is_empty()
}

// The predicate is specifically a fn() and not a closure to forbid capturing values
// from the environment, which would make it easier to accidentally leak some data.
// For example, the following won't compile with fn() but would work with impl Fn():
// ```
// let mut chars = Mutex::new(Vec::new());
// self.any_chars(|c| {chars.lock().unwrap().push(c); true});
// ```
// Note that this is not a perfect solution, as it is still possible to leak the characters by
// using a global variable or a static variable. Also `char` implements Copy so it's hard to
// ensure the compiler is not making a copy of the character.
#[inline(always)]
pub fn any_chars(&self, predicate: fn(char) -> bool) -> bool {
self.value.chars().any(predicate)
}
}

impl<T: Zeroize + AsRef<[u8]>> Sensitive<T> {
pub fn encode_base64<E: base64::Engine>(self, engine: E) -> SensitiveString {
use base64::engine::Config;

let inner: &[u8] = self.value.as_ref().as_ref();

// Prevent accidental copies by allocating the full size
let padding = engine.config().encode_padding();
let len = base64::encoded_len(inner.len(), padding).expect("Valid length");

let mut value = SensitiveVec::new(Box::new(vec![0u8; len]));
engine
.encode_slice(inner, &mut value.value[..len])
.expect("Valid base64 string length");

value.try_into().expect("Valid base64 string")
}
}

impl<V: Zeroize + Default> Default for Sensitive<V> {
fn default() -> Self {
Self::new(Box::default())
Expand All @@ -186,11 +116,7 @@ impl<V: Zeroize + PartialEq<V>> PartialEq<V> for Sensitive<V> {
self.value.as_ref().eq(other)
}
}
impl PartialEq<&str> for SensitiveString {
fn eq(&self, other: &&str) -> bool {
self.value.as_ref().eq(other)
}
}

impl PartialEq<&[u8]> for SensitiveVec {
fn eq(&self, other: &&[u8]) -> bool {
self.value.as_ref().eq(other)
Expand Down Expand Up @@ -248,12 +174,6 @@ mod tests {

#[test]
fn test_debug() {
let string = SensitiveString::test("test");
assert_eq!(
format!("{:?}", string),
"Sensitive { type: \"alloc::string::String\", value: \"********\" }"
);

let vector = Sensitive::new(Box::new(vec![1, 2, 3]));
assert_eq!(
format!("{:?}", vector),
Expand All @@ -265,8 +185,6 @@ mod tests {
fn test_schemars() {
#[derive(JsonSchema)]
struct TestStruct {
#[allow(dead_code)]
s: SensitiveString,
#[allow(dead_code)]
v: SensitiveVec,
}
Expand All @@ -279,9 +197,6 @@ mod tests {
"type": "object",
"required": ["s", "v"],
"properties": {
"s": {
"$ref": "#/definitions/String"
},
"v": {
"$ref": "#/definitions/Array_of_uint8"
}
Expand Down
Loading

0 comments on commit b424a92

Please sign in to comment.