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

Hinton · 2024-01-25T09:40:07Z

Type of change

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

Objective

Updates the decrypt methods to return a Sensitive value which implements zeroize and zeroize on drop to ensure the values are zeroed on drop within the rust ecosystem.

Before you submit

Please add unit tests where it makes sense to do so

…pted

bitwarden-bot · 2024-01-25T15:34:59Z

Checkmarx One – Scan Summary & Details – 579923c4-0baf-4a17-8736-6834ae7f3c3a

No New Or Fixed Issues Found

Hinton · 2024-01-25T18:21:31Z

crates/bitwarden-crypto/src/decrypted.rs

+    KeyEncryptable<Key, Output> for Decrypted<T>
+{
+    fn encrypt_with_key(self, key: &Key) -> Result<Output, CryptoError> {
+        self.value.clone().encrypt_with_key(key)


Todo: need to validate what happens with the string after encrypting it. If we consume it we should zero it.

Yeah, we should probably add the zeroize in the KeyEncryptable implementation of the EncStrings, which is where the String is consumed finally.

Also, we could avoid the extra clone here by doing

std::mem::take(&mut self.value).encrypt_with_key(key)

It would mean adding a Default bound to the impl, though

…pted # Conflicts: # Cargo.lock # crates/bitwarden-crypto/Cargo.toml # crates/bitwarden-crypto/src/enc_string/symmetric.rs # crates/bitwarden-crypto/src/keys/device_key.rs # crates/bitwarden-crypto/src/keys/master_key.rs # crates/bitwarden/src/auth/login/access_token.rs # crates/bitwarden/src/client/encryption_settings.rs # crates/bitwarden/src/vault/cipher/attachment.rs # crates/bitwarden/src/vault/cipher/cipher.rs

codecov · 2024-01-26T10:24:58Z

Codecov Report

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

Project coverage is 61.72%. Comparing base (dc4ad16) to head (7baa6f1).
Report is 5 commits behind head on main.

Files	Patch %	Lines
crates/bitwarden/src/vault/cipher/cipher.rs	83.33%	27 Missing ⚠️
crates/bitwarden/src/auth/tde.rs	0.00%	3 Missing ⚠️
crates/bitwarden/src/secrets_manager/state.rs	0.00%	3 Missing ⚠️
crates/bitwarden/src/client/encryption_settings.rs	50.00%	2 Missing ⚠️
crates/bitwarden/src/mobile/vault/client_sends.rs	0.00%	2 Missing ⚠️
...n/src/secrets_manager/projects/project_response.rs	0.00%	2 Missing ⚠️
...s/bitwarden/src/mobile/vault/client_attachments.rs	0.00%	1 Missing ⚠️
crates/bitwarden/src/vault/send.rs	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #536      +/-   ##
==========================================
+ Coverage   60.90%   61.72%   +0.82%     
==========================================
  Files         173      173              
  Lines       10694    10905     +211     
==========================================
+ Hits         6513     6731     +218     
+ Misses       4181     4174       -7

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

dani-garcia

Looks pretty good to me, just some small comments

crates/bitwarden-crypto/Cargo.toml

crates/bitwarden-crypto/src/decrypted.rs

dani-garcia · 2024-01-26T10:48:10Z

crates/bitwarden-crypto/src/decrypted.rs

+    KeyEncryptable<Key, Output> for Decrypted<T>
+{
+    fn encrypt_with_key(self, key: &Key) -> Result<Output, CryptoError> {
+        self.value.clone().encrypt_with_key(key)


Yeah, we should probably add the zeroize in the KeyEncryptable implementation of the EncStrings, which is where the String is consumed finally.

Also, we could avoid the extra clone here by doing

std::mem::take(&mut self.value).encrypt_with_key(key)

It would mean adding a Default bound to the impl, though

crates/bitwarden/src/vault/send.rs

# Conflicts: # crates/bitwarden-crypto/src/enc_string/symmetric.rs # crates/bitwarden-crypto/src/keys/device_key.rs # crates/bitwarden-crypto/src/keys/master_key.rs # crates/bitwarden/src/auth/auth_request.rs # crates/bitwarden/src/vault/cipher/field.rs # crates/bitwarden/src/vault/folder.rs

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

## 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.

…pted # Conflicts: # crates/bitwarden-crypto/src/enc_string/symmetric.rs # crates/bitwarden-crypto/src/keys/device_key.rs # crates/bitwarden-crypto/src/keys/master_key.rs # crates/bitwarden-crypto/src/sensitive/sensitive.rs # crates/bitwarden/src/auth/login/access_token.rs # crates/bitwarden/src/mobile/crypto.rs # crates/bitwarden/src/secrets_manager/projects/project_response.rs # crates/bitwarden/src/secrets_manager/secrets/list.rs # crates/bitwarden/src/secrets_manager/secrets/secret_response.rs # crates/bitwarden/src/secrets_manager/state.rs # crates/bitwarden/src/uniffi_support.rs # crates/bitwarden/src/vault/cipher/attachment.rs # crates/bitwarden/src/vault/cipher/cipher.rs # crates/bitwarden/src/vault/send.rs

…pted # Conflicts: # crates/bitwarden/src/vault/send.rs

Hinton

Reminders for myself.

crates/bitwarden/src/vault/send.rs

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

crates/bitwarden/src/auth/auth_request.rs

crates/bitwarden/src/auth/login/access_token.rs

crates/bitwarden/src/mobile/vault/client_sends.rs

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

…pted # Conflicts: # crates/bitwarden-crypto/src/keys/master_key.rs # crates/bitwarden/src/mobile/crypto.rs # crates/bitwarden/src/vault/send.rs

Hinton · 2024-04-23T16:43:25Z

I think the remaining tasks are large enough to be separate tasks. @dani-garcia let me know if you disagree. I'll write up tickets for them.

dani-garcia

Mostly LGTM, just a couple of comments.

Let's go ahead and open tickets for the other unresolved tasks, plus the SensitiveString improvements we've mentioned.

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

crates/bitwarden/src/auth/tde.rs

crates/bitwarden/src/uniffi_support.rs

Hinton added 5 commits January 19, 2024 14:40

Explore introducing a Decrypted struct

8c869f2

Replace with std::mem::take

74a5b08

Merge branch 'main' of github.com:bitwarden/sdk into ps/zeroize-decry…

c57bea6

…pted

Document and introduce type alias

d5f2ae5

Format

97ecc2c

Hinton changed the title ~~Introducing a Decrypted struct~~ [PM-5791] Introducing a Decrypted struct Jan 25, 2024

Hinton added 2 commits January 25, 2024 17:00

Add Decrypted to asymmetric

449466a

Use Decrypt everywhere

ed4d92b

Hinton commented Jan 25, 2024

View reviewed changes

Hinton added 2 commits January 26, 2024 10:35

Use boxed value and fix tests

18cf0d6

Hinton requested a review from dani-garcia January 26, 2024 10:21

dani-garcia reviewed Jan 26, 2024

View reviewed changes

Hinton added 2 commits January 29, 2024 13:37

Resolve review feedback

b848625

Extract a common sensitive type.

d85fc9f

Hinton changed the title ~~[PM-5791] Introducing a Decrypted struct~~ [PM-5791] Introducing a Sensitive type Jan 29, 2024

Hinton and others added 3 commits January 29, 2024 14:11

Format

d7eedaa

Update exporters to use Sensitive

ba14e8c

github-advanced-security bot found potential problems Mar 7, 2024

View reviewed changes

crates/bitwarden-crypto/src/sensitive/mod.rs Fixed Show fixed Hide fixed

dani-garcia added 3 commits March 7, 2024 18:21

Add support for base64 encode/decode for the Sensitive types

19fecae

Merge branch 'main' into ps/zeroize-decrypted

cece11e

Make base64 memory test required

ef8ba37

dani-garcia mentioned this pull request Mar 20, 2024

Add sensitive types to SymmetricCryptoKey #672

Merged

Hinton added 3 commits April 22, 2024 15:12

Use ::test in tests

b87bd95

Mark send key as sensitive

37b18ad

Merge branch 'main' of github.com:bitwarden/sdk into ps/zeroize-decry…

b2c3ff3

…pted # Conflicts: # crates/bitwarden/src/vault/send.rs

Hinton changed the title ~~[PM-5791] Introducing a Sensitive type~~ [PM-5791] Change decrypt to return Sensitive Apr 22, 2024

Remove accidentally duplicated generate_cipher

1f731a8

Hinton commented Apr 22, 2024

View reviewed changes

Hinton added 5 commits April 23, 2024 11:11

Convert Send::derive_shareable_key to expect SensitiveVec

ef08b7c

Fix compile

16ea5c3

Refactor sub_title to not expose temporary values

31a02f8

Merge branch 'main' of github.com:bitwarden/sdk into ps/zeroize-decry…

456f54f

…pted # Conflicts: # crates/bitwarden-crypto/src/keys/master_key.rs # crates/bitwarden/src/mobile/crypto.rs # crates/bitwarden/src/vault/send.rs

Shore up uri checksum

0d40054

Hinton marked this pull request as ready for review April 23, 2024 16:43

Hinton requested a review from dani-garcia April 23, 2024 16:43

dani-garcia reviewed Apr 24, 2024

View reviewed changes

crates/bitwarden-crypto/src/sensitive/sensitive.rs Show resolved Hide resolved

crates/bitwarden/src/auth/tde.rs Outdated Show resolved Hide resolved

crates/bitwarden/src/uniffi_support.rs Outdated Show resolved Hide resolved

Hinton added 2 commits April 25, 2024 10:29

Remove converter for DecryptedString

da787a2

Review feedback

7baa6f1

dani-garcia approved these changes Apr 25, 2024

View reviewed changes

Hinton merged commit 51db07e into main Apr 25, 2024
77 checks passed

Hinton deleted the ps/zeroize-decrypted branch April 25, 2024 09:20

Hinton added a commit that referenced this pull request May 17, 2024

Revert "[PM-5791] Change decrypt to return Sensitive (#536)"

7eea301

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Hinton commented Jan 25, 2024 •

edited

Loading

bitwarden-bot commented Jan 25, 2024 •

edited by github-actions bot

Loading

Hinton Jan 25, 2024

dani-garcia Jan 26, 2024

codecov bot commented Jan 26, 2024 •

edited

Loading

dani-garcia left a comment

dani-garcia Jan 26, 2024

Hinton left a comment

Hinton commented Apr 23, 2024

dani-garcia left a comment

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

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

Conversation

Hinton commented Jan 25, 2024 • edited Loading

Type of change

Objective

Before you submit

bitwarden-bot commented Jan 25, 2024 • edited by github-actions bot Loading

No New Or Fixed Issues Found

Hinton Jan 25, 2024

Choose a reason for hiding this comment

dani-garcia Jan 26, 2024

Choose a reason for hiding this comment

codecov bot commented Jan 26, 2024 • edited Loading

Codecov Report

dani-garcia left a comment

Choose a reason for hiding this comment

dani-garcia Jan 26, 2024

Choose a reason for hiding this comment

Hinton left a comment

Choose a reason for hiding this comment

Hinton commented Apr 23, 2024

dani-garcia left a comment

Choose a reason for hiding this comment

Hinton commented Jan 25, 2024 •

edited

Loading

bitwarden-bot commented Jan 25, 2024 •

edited by github-actions bot

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading