Add sensitive types to SymmetricCryptoKey #672

dani-garcia · 2024-03-20T16:32:54Z

Type of change

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

Objective

This is a small subset of #536, which only contains the minimum code to protect the import/export functions on SymmetricCryptoKey. It also enables the from_base64 test on SymmetricCryptoKey as that passes now.

After this PR is merged I'll be expanding the use of Sensitive to other parts of the codebase while adding some extra memory testing checks to validate that it works for them.

codecov · 2024-03-20T16:38:18Z

Codecov Report

Attention: Patch coverage is 81.72589% with 36 lines in your changes are missing coverage. Please review.

Project coverage is 60.63%. Comparing base (9cd9a15) to head (c57d6ef).

Files	Patch %	Lines
crates/bitwarden-crypto/src/sensitive/sensitive.rs	90.08%	12 Missing ⚠️
crates/bitwarden-crypto/src/uniffi_support.rs	0.00%	6 Missing ⚠️
crates/bitwarden/src/mobile/crypto.rs	50.00%	5 Missing ⚠️
crates/bitwarden/src/vault/cipher/attachment.rs	16.66%	5 Missing ⚠️
crates/bitwarden-crypto/src/sensitive/decrypted.rs	0.00%	3 Missing ⚠️
crates/bitwarden-uniffi/src/crypto.rs	0.00%	1 Missing ⚠️
crates/bitwarden/src/auth/login/access_token.rs	66.66%	1 Missing ⚠️
crates/bitwarden/src/auth/tde.rs	0.00%	1 Missing ⚠️
crates/bitwarden/src/mobile/client_crypto.rs	0.00%	1 Missing ⚠️
crates/bitwarden/src/secrets_manager/state.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #672      +/-   ##
==========================================
+ Coverage   60.41%   60.63%   +0.21%     
==========================================
  Files         171      173       +2     
  Lines       10426    10555     +129     
==========================================
+ Hits         6299     6400     +101     
- Misses       4127     4155      +28

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

crates/bitwarden/src/mobile/crypto.rs

Hinton · 2024-03-28T11:15:16Z

crates/bitwarden-crypto/src/sensitive/sensitive.rs

+    pub fn decode_base64<T: base64::Engine>(self, engine: T) -> Result<SensitiveVec, CryptoError> {
+        // Preallocate a Vec with the necessary capacity
+        let len = base64::decoded_len_estimate(self.value.len());
+        let mut value = SensitiveVec::new(Box::new(Vec::with_capacity(len)));


Do we know what happens when a vector grows, will it make a copy of the data or have pointers to the new locations.

We should probably provide warnings for this behaviour.

When the Vec grows over capacity, it's going to make a new allocation that is bigger than the current one, copy the data over and then dealloc the old allocation. Note that the deallocation of the older data won't call the Vec's zeroize so it's definitely problematic. The same will happen with Strings as they are Vecs under the hood, but I think it's more unlikely for a string to be appended to.

I think adding warnings to those types sounds like a good idea yeah!

Hinton · 2024-03-28T11:24:07Z

crates/bitwarden-crypto/src/sensitive/sensitive.rs

+    // IMPORTANT: This should not be used outside of test code
+    // Note that we can't just mark it with #[cfg(test)] because that only applies
+    // when testing this crate, not when testing other crates that depend on it.
+    // By at least limiting it to &'static str we should be able to avoid accidental usages


Would exposing a specific feature and only using that feature in tests work?

Hmm I think it might, we could even enable the feature in dev-dependencies too to avoid requiring it when calling cargo test. I'll give it a try!

github-actions · 2024-03-28T12:05:40Z

Checkmarx One – Scan Summary & Details – cb9bcd93-8686-426f-ac7b-01df7600efc5

No New Or Fixed Issues Found

Hinton

Looks good, some open questions on expectations for inputs/outputs.

Hinton · 2024-03-28T11:24:55Z

crates/bitwarden-crypto/src/uniffi_support.rs

+/// Uniffi doesn't seem to be generating the SensitiveString unless it's being used by
+/// a record somewhere. This is a workaround to make sure the type is generated.
+#[derive(uniffi::Record)]
+struct SupportSensitiveString {
+    sensitive_string: SensitiveString,
+}


Let's open an issue for this.

Opened PM-7214

Hinton · 2024-03-28T11:26:17Z

crates/bitwarden/src/vault/cipher/attachment.rs

+        attachment.key = Some(
+            attachment_key
+                .to_vec()
+                .expose()


Exposing a sensitive value for encrypting seems silly. Could we implement encrypt for sensitive values?

We currently implement encrypt for sensitive values, the problem is for Vecs we implement encrypt on &[u8] and rely on rust auto deref to coerce the Vec to a slice, but that will not happen for Sensitive.

This is going to require either updating all the Encrypt impls to work only on Vec or SensitiveVec, or adding a specific impl only for Sensitive, which might have to be done carefully to avoid conflicts with the current blanket impl.

I think this is best left for a separate PR to avoid bloating this one, though.

crates/bitwarden-crypto/src/keys/device_key.rs

Hinton · 2024-03-28T12:23:12Z

crates/bitwarden-crypto/src/keys/master_key.rs

@@ -67,7 +67,7 @@ impl MasterKey {
        let stretched_key = stretch_kdf_key(&self.0)?;

        EncString::encrypt_aes256_hmac(
-            user_key.to_vec().as_slice(),
+            user_key.to_vec().expose(),


Should we look into if the encrypt methods should accept sensitive values?

Definitely, but it needs to be updated at the same time as the KeyEncryptable impls, as they use this function and we don't want to be boxing/unboxing all the time.

Hinton · 2024-03-28T12:29:44Z

crates/bitwarden-crypto/src/keys/symmetric_crypto_key.rs

-    pub fn to_vec(&self) -> Zeroizing<Vec<u8>> {
-        let mut buf = Vec::with_capacity(self.total_len());
+    pub fn to_vec(&self) -> SensitiveVec {
+        let mut buf = SensitiveVec::new(Box::new(Vec::with_capacity(self.total_len())));


Maybe add a comment why it's important to allocate the full length. It could be seen as an optimization method otherwise.

Suggested change

let mut buf = SensitiveVec::new(Box::new(Vec::with_capacity(self.total_len())));

// Prevent accidental copies by allocating the full size

let mut buf = SensitiveVec::new(Box::new(Vec::with_capacity(self.total_len())));

Hinton · 2024-03-28T12:30:43Z

crates/bitwarden/src/auth/renew.rs

@@ -63,8 +63,10 @@ pub(crate) async fn renew_token(client: &mut Client) -> Result<()> {
                    ) = (&result, state_file, client.get_encryption_settings())
                    {
                        if let Some(enc_key) = enc_settings.get_key(&None) {
-                            let state =
-                                ClientState::new(r.access_token.clone(), enc_key.to_base64());
+                            let state = ClientState::new(


Should state expect sensitive values directly?

Just checked and it's not too big a diff, changed it

# Conflicts: # crates/bitwarden-crypto/src/keys/master_key.rs

Hinton

SM state needs to be backwards compatible, did we check if the struct works with old state files?

Hinton · 2024-04-01T10:27:54Z

crates/bitwarden/src/secrets_manager/state.rs

@@ -15,11 +15,11 @@ const STATE_VERSION: u32 = 1;
 pub struct ClientState {
    pub(crate) version: u32,
    pub(crate) token: String,
-    pub(crate) encryption_key: String,
+    pub(crate) encryption_key: SensitiveString,


Any concerns with backwards compatibility here?

I gave it a test to be sure now and it works yeah. The struct is only used for serialization/deserialization and the sensitive type works the same way as a plain string there.

Add sensitive types to SymmetricCryptoKey

d2de7d6

dani-garcia marked this pull request as ready for review March 20, 2024 16:42

dani-garcia requested a review from Hinton March 20, 2024 16:42

Hinton reviewed Mar 25, 2024

View reviewed changes

crates/bitwarden/src/mobile/crypto.rs Outdated Show resolved Hide resolved

dani-garcia added 2 commits March 25, 2024 18:30

Add uniffi support

a5a8097

Make sure the uniffi type is generated correctly

2c1ff1c

Hinton reviewed Mar 28, 2024

View reviewed changes

Add reallocation note

9eccf0e

Hinton reviewed Mar 28, 2024

View reviewed changes

dani-garcia added 4 commits March 28, 2024 13:45

Review comments

fee97ec

Use sensitivestring in ClientState and payload

2a354b5

Merge branch 'main' into ps/sensitive

f61e85a

# Conflicts: # crates/bitwarden-crypto/src/keys/master_key.rs

Missed new change

c57d6ef

dani-garcia requested a review from Hinton April 1, 2024 10:11

Hinton approved these changes Apr 1, 2024

View reviewed changes

dani-garcia merged commit 538b9f2 into main Apr 1, 2024
75 checks passed

dani-garcia deleted the ps/sensitive branch April 1, 2024 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sensitive types to SymmetricCryptoKey #672

Add sensitive types to SymmetricCryptoKey #672

dani-garcia commented Mar 20, 2024 •

edited

Loading

codecov bot commented Mar 20, 2024 •

edited

Loading

Hinton Mar 28, 2024 •

edited

Loading

dani-garcia Mar 28, 2024

Hinton Mar 28, 2024

dani-garcia Mar 28, 2024

github-actions bot commented Mar 28, 2024 •

edited

Loading

Hinton left a comment

Hinton Mar 28, 2024

dani-garcia Apr 1, 2024

Hinton Mar 28, 2024

dani-garcia Mar 28, 2024

Hinton Mar 28, 2024

dani-garcia Apr 1, 2024

Hinton Mar 28, 2024

dani-garcia Mar 28, 2024

Hinton Mar 28, 2024

dani-garcia Mar 28, 2024

Hinton left a comment

Hinton Apr 1, 2024

dani-garcia Apr 1, 2024

	let mut buf = SensitiveVec::new(Box::new(Vec::with_capacity(self.total_len())));
	// Prevent accidental copies by allocating the full size
	let mut buf = SensitiveVec::new(Box::new(Vec::with_capacity(self.total_len())));

Add sensitive types to SymmetricCryptoKey #672

Add sensitive types to SymmetricCryptoKey #672

Conversation

dani-garcia commented Mar 20, 2024 • edited Loading

Type of change

Objective

codecov bot commented Mar 20, 2024 • edited Loading

Codecov Report

Hinton Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 28, 2024 • edited Loading

No New Or Fixed Issues Found

Hinton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hinton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-garcia commented Mar 20, 2024 •

edited

Loading

codecov bot commented Mar 20, 2024 •

edited

Loading

Hinton Mar 28, 2024 •

edited

Loading

github-actions bot commented Mar 28, 2024 •

edited

Loading