[PM-5728] Enable Zeroize support for keys and EncStrings

[dani-garcia]

Type of change

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

Objective

• Implemented Zeroize support for the encryption keys and the encStrings.
• Boxed the internals of the encryption keys to avoid Rust from moving them around on the stack.
• Changed some functions that passed around owned copies of the GenericArrays to pass references instead. GenericArray implements Copy, so passing them around by value only means more stack copies if the compiler can't optimize them out.
• Changed the from impls for SymmCryptoKey to take mut slices so we can zeroize them after load.

[codecov]

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (af880e6) 51.51% compared to head (9fbb514) 51.69%.

Files	Patch %	Lines
...bitwarden-crypto/src/keys/asymmetric_crypto_key.rs	50.00%	4 Missing ⚠️
crates/bitwarden/src/client/encryption_settings.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #515      +/-   ##
==========================================
+ Coverage   51.51%   51.69%   +0.17%     
==========================================
  Files         163      163              
  Lines        8168     8202      +34     
==========================================
+ Hits         4208     4240      +32     
- Misses       3960     3962       +2     

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

[Hinton]

I suspect we need to spend some more time thinking about vectors potentially copying on resize, this might also apply to strings?

Generally good improvements, I do wonder how we'll test this efficiently though, would be nice if we could produce memory dumps of tests somehow and assert.

[bitwarden-bot]

Checkmarx One – Scan Summary & Details – e0ddd242-9062-41c4-96c9-2b1c1c3a6117

Fixed Issues

Severity	Issue	Source File / Package
	Command_Injection	/languages/python/example.py: 66
	Command_Injection	/languages/python/example.py: 35
	Missing User Instruction	/Dockerfile: 23
	Reflected_XSS	/languages/kotlin/app/src/main/java/com/bitwarden/myapplication/MainActivity.kt: 379
	Apt Get Install Pin Version Not Defined	/Dockerfile: 9
	Privacy_Violation	/languages/java/Example.java: 12
	Privacy_Violation	/languages/java/src/main/java/com/bitwarden/sdk/SecretsClient.java: 40
	Privacy_Violation	/languages/java/Example.java: 13
	Unchecked_Input_for_Loop_Condition	/languages/kotlin/app/src/main/java/com/bitwarden/myapplication/MainActivity.kt: 379
	Unpinned Actions Full Length Commit SHA	/build-cli-docker.yml: 151
	Unpinned Actions Full Length Commit SHA	/build-cli-docker.yml: 68
	Unpinned Actions Full Length Commit SHA	/publish-rust-crates.yml: 134
	Unpinned Actions Full Length Commit SHA	/release-napi.yml: 50
	Unpinned Actions Full Length Commit SHA	/release-cli.yml: 137
	Unpinned Actions Full Length Commit SHA	/release-cli.yml: 78
	Unpinned Actions Full Length Commit SHA	/release-cli.yml: 61
	Unpinned Actions Full Length Commit SHA	/version-bump.yml: 52
	Unpinned Actions Full Length Commit SHA	/build-cli-docker.yml: 75
	Unpinned Actions Full Length Commit SHA	/release-napi.yml: 135
	Unpinned Actions Full Length Commit SHA	/release-napi.yml: 104
	Using Platform Flag with FROM Command	/Dockerfile: 4
	Healthcheck Instruction Missing	/Dockerfile: 23
	Missing_CSP_Header	/about.hbs: 48

[dani-garcia]

I've added some tests to SymmetricCryptoKey checking if the key got zeroized correctly with the help of some unsafe. It's not perfect as some of the memory gets reused, but it shows that the zeroize works at least.

Welp scratch that, those tests worked for me locally but they segfault on CI

[Hinton]

I don't believe this consumes the key. Do we need to zero dec?

[dani-garcia]

We zeroize the key inside the TryFrom<&mut [u8]> impl:

https://github.com/bitwarden/sdk/blob/6e844469c9e1555a5abc45248ea88abcc025679b/crates/bitwarden-crypto/src/keys/symmetric_crypto_key.rs#L90-L115

[Hinton]

That feels potentially confusing? Do you expect a TryFrom of a slice to manipulate the input? I do see the benefit of ensuring it gets consumed though.

[dani-garcia]

We could also just pass an owned Vec<> and consume it in the function, but that will limit the use of the API for someone trying to pass a GenericArray or a [u8; 64] for example.

I think the fact that the function receives a &mut [u8] should make it clear enough that something is being modified, but we could add a comment mentioning explicitly that it's being zeroed.

[Hinton]

Let's add a comment to make it explicit. We can revaluate if this turns out to be confusing, potentially not using try from and instead having an explicit function could make it less confusing.