[PM-15096] Implement xchacha20-poly1305 cipher functions #150

quexten · 2025-02-07T13:30:37Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-15096

📔 Objective

Implements xchacha20-poly1305 (https://libsodium.gitbook.io/doc/secret-key_cryptography/aead/chacha20-poly1305/xchacha20-poly1305_construction) as a new encryption type.

Further, this introduces a dependency patch override (to https://github.com/bitwarden/rustcrypto-formats.git at revision 2b27c63034217dd126bbf5ed874da51b84f8c705) for rustcrypto formats, until this gets updated upstream to fix a type incompatibility when building on WASM and with chacha as a dependency.

These functions are unused until a follow-up PR (soon).

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

github-actions · 2025-02-07T13:34:10Z

Checkmarx One – Scan Summary & Details – 67334bca-c752-4f20-a22c-d48b6c362b37

Great job, no security vulnerabilities found in this Pull Request

codecov · 2025-02-07T16:38:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.31%. Comparing base (aa9ed2a) to head (747e7a7).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #150      +/-   ##
==========================================
+ Coverage   66.08%   66.31%   +0.22%     
==========================================
  Files         198      199       +1     
  Lines       15505    15609     +104     
==========================================
+ Hits        10247    10351     +104     
  Misses       5258     5258

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

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

crates/bitwarden-crypto/src/chacha20.rs

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

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

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

crates/bitwarden-crypto/src/enc_string/symmetric.rs

crates/bitwarden-crypto/src/xchacha20.rs

dani-garcia · 2025-03-07T13:47:42Z

crates/bitwarden-crypto/Cargo.toml

 generic-array = { version = ">=0.14.7, <1.0", features = ["zeroize"] }
 hkdf = ">=0.12.3, <0.13"
 hmac = ">=0.12.1, <0.13"
 num-bigint = ">=0.4, <0.5"
 num-traits = ">=0.2.15, <0.3"
 pbkdf2 = { version = ">=0.12.1, <0.13", default-features = false }
+poly1305 = { version = "0.8.0", features = ["zeroize"] }


This crate doesn't seem to be used at the moment, I assume it's used in your other PR with the key-commiting implementation? If so we can probably remove it for now.

Weird, for chacha the CI pipeline complained about an unused dependency, for poly1305 it did not.

dani-garcia · 2025-03-07T13:58:54Z

crates/bitwarden-crypto/src/xchacha20.rs

+use crate::CryptoError;
+
+#[allow(unused)]
+pub(crate) fn generate_nonce() -> GenericArray<u8, U24> {


Is this function only meant to be used for tests? If so, it should be moved inside mod tests {

Hmm, initially I thought we needed the IV to be included in the authenticated data, but I checked the spec of the format we want to follow for the new keys (COSE https://datatracker.ietf.org/doc/html/rfc8152), and the IV/nonce does not need to be authenticated, so it could just be generated during encrypt. We do however need to serialize it onto the encstring. So What would the recommendation here be, return a tuple / struct of Nonce + Ciphertext, and generate the nonce in the encrypt function?

That's more or less what we do for aes now yeah, so it seems reasonable to do the same for chacha

pub(crate) fn encrypt_aes256_hmac( data_dec: &[u8], mac_key: &GenericArray<u8, U32>, key: &GenericArray<u8, U32>, ) -> Result<([u8; 16], [u8; 32], Vec<u8>)> { /// IV is generated and returned with the encrypted data and the MAC as a tuple Ok((iv, mac, data)) }

dani-garcia · 2025-03-07T14:06:49Z

crates/bitwarden-crypto/src/xchacha20.rs

+use chacha20poly1305::{AeadCore, AeadInPlace, KeyInit, XChaCha20Poly1305};
+use generic_array::{typenum::U24, GenericArray};
+
+/**


Small nit, but I would move this comment to the top of the file and use //! like on the other one. As it is right now, we're technically adding a doc comment to use crate::CryptoError

dani-garcia · 2025-03-07T14:10:31Z

crates/bitwarden-crypto/src/lib.rs

@@ -91,6 +91,7 @@ pub use wordlist::EFF_LONG_WORD_LIST;
 mod store;
 pub use store::{KeyStore, KeyStoreContext};
 mod traits;
+mod xchacha20;


This is more of an idea for the future, but it would be nice to think about moving the aes, rsa and the new xchacha20 modules into their own separate low level algorithms module so they're not all exposed in the root.

dani-garcia · 2025-03-07T14:14:03Z

crates/bitwarden-crypto/src/xchacha20.rs

+
+#[allow(unused)]
+pub(crate) fn encrypt_xchacha20_poly1305(
+    nonce: &[u8; 24],


On the aes module we generate the IV internally to ensure we always generate a random one each time for encryption, assuming that the nonce works the same in chacha20, have you considered not exposing this as a parameter for encryption and just generating it internally?

As mentioned for the nonce generating function, initially I thought the IV needed to be authenticated, but this is not the case, so I'll move this to generate during encrypt.

dani-garcia · 2025-03-07T14:15:35Z

crates/bitwarden-crypto/src/xchacha20.rs

+    associated_data: &[u8],
+) -> Result<Vec<u8>, CryptoError> {
+    // This buffer contains the plaintext, that will be encrypted in-place
+    let mut buffer = Vec::from(plaintext_secret_data);


Minor nit: In decrypt_xchacha20_poly1305 we're using slice.to_vec(), while here we're using Vec::from(slice). We should try to be consistent and use one or the other.

dani-garcia · 2025-03-07T14:17:28Z

crates/bitwarden-crypto/src/xchacha20.rs

+            associated_data,
+            &mut buffer,
+        )
+        .map_err(|_| CryptoError::InvalidKey)?;


I think for aes and rsa we use the CryptoError::KeyDecrypt error when decrypting. If so we should do the same here as well

dani-garcia · 2025-03-07T14:24:17Z

crates/bitwarden-crypto/src/xchacha20.rs

+            associated_data,
+            &mut buffer,
+        )
+        .map_err(|_| CryptoError::InvalidKey)?;


What's the error case for chacha20 encryption? Key and nonce sizes are checked at compile time, and as far as I know, chacha20 doesn't have any specific key format requirements.

If the only error case is the buffer not having enough space, I think we should be fine calling .expect("") and not returning a Result.

crates/bitwarden-crypto/src/xchacha20.rs

MGibson1

My only note is that I kind of want to see the interface one level up, as well.

I say this because we could design certain aspects of the AAD set into this level. I'm not set definitively in either direction, but there's an elegance to tying the cipherText to a key and cipher type at this level, rather than in the Encryptable

quexten · 2025-03-25T10:06:34Z

@MGibson1 Let's revisit this on the cose PR. I do at this point believe that COSE is better suited though, if we want a structured format to add our own additional data.

sonarqubecloud · 2025-03-25T10:08:18Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

quexten marked this pull request as draft February 7, 2025 13:31

quexten changed the title ~~Change cipher to plain xchachapoly1305~~ Implement xchachapoly1305 cipher Feb 7, 2025

quexten changed the title ~~Implement xchachapoly1305 cipher~~ [PM-15096] Implement xchachapoly1305 cipher Feb 7, 2025

github-advanced-security bot found potential problems Feb 11, 2025

View reviewed changes

crates/bitwarden-crypto/src/keys/pin_key.rs Fixed Show fixed Hide fixed

crates/bitwarden-crypto/src/keys/symmetric_crypto_key.rs Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Feb 11, 2025

View reviewed changes

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

crates/bitwarden-crypto/src/keys/symmetric_crypto_key.rs Fixed Show fixed Hide fixed

quexten changed the base branch from km/17370/refactor-sdk-symmetric-keys to km/new-key-format February 12, 2025 12:28

github-advanced-security bot found potential problems Feb 12, 2025

View reviewed changes

quexten changed the title ~~[PM-15096] Implement xchachapoly1305 cipher~~ [PM-15096] Implement xchacha20-poly1305 cipher Feb 12, 2025

quexten force-pushed the km/new-key-format branch from c259ab8 to 03dccda Compare February 18, 2025 12:19

quexten force-pushed the km/pm-15096/xchacha20-poly1305 branch from 7f33323 to 34f25de Compare February 19, 2025 12:23

github-advanced-security bot found potential problems Feb 19, 2025

View reviewed changes

crates/bitwarden-crypto/src/enc_string/symmetric.rs Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Feb 19, 2025

View reviewed changes

crates/bitwarden-crypto/src/enc_string/symmetric.rs Fixed Show fixed Hide fixed

quexten changed the base branch from km/new-key-format to main March 7, 2025 10:36

Add xchacha20poly1305 crypto primitives

72bcf39

quexten force-pushed the km/pm-15096/xchacha20-poly1305 branch from bff507b to 72bcf39 Compare March 7, 2025 10:44

Cargo fmt

0e68201

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

View reviewed changes

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

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

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

quexten added 2 commits March 7, 2025 11:59

Cleanup

a58c53f

Cleanup

ba2d199

quexten marked this pull request as ready for review March 7, 2025 11:17

quexten changed the title ~~[PM-15096] Implement xchacha20-poly1305 cipher~~ [PM-15096] Implement xchacha20-poly1305 cipher functions Mar 7, 2025

quexten requested review from dani-garcia and MGibson1 March 7, 2025 11:18

dani-garcia reviewed Mar 7, 2025

View reviewed changes

quexten added 4 commits March 7, 2025 16:32

Remove poly1305 dependency

7647256

Move comment

6bab049

Adjust interface according to feedback

6b241ec

Remove comment

ced7213

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

View reviewed changes

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

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

quexten and others added 2 commits March 7, 2025 17:22

Fix clippy warning

44e5ffd

Merge branch 'main' into km/pm-15096/xchacha20-poly1305

366d79f

quexten requested a review from dani-garcia March 24, 2025 14:49

dani-garcia approved these changes Mar 24, 2025

View reviewed changes

MGibson1 reviewed Mar 25, 2025

View reviewed changes

Merge branch 'main' into km/pm-15096/xchacha20-poly1305

747e7a7

quexten requested a review from a team as a code owner March 25, 2025 10:07

quexten requested a review from justindbaur March 25, 2025 10:07

quexten merged commit ae3c08d into main Mar 25, 2025
36 checks passed

quexten deleted the km/pm-15096/xchacha20-poly1305 branch March 25, 2025 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-15096] Implement xchacha20-poly1305 cipher functions #150

[PM-15096] Implement xchacha20-poly1305 cipher functions #150

quexten commented Feb 7, 2025 •

edited

Loading

github-actions bot commented Feb 7, 2025 •

edited

Loading

codecov bot commented Feb 7, 2025 •

edited

Loading

dani-garcia Mar 7, 2025

quexten Mar 7, 2025 •

edited

Loading

dani-garcia Mar 7, 2025

quexten Mar 7, 2025

dani-garcia Mar 7, 2025 •

edited

Loading

quexten Mar 7, 2025

dani-garcia Mar 7, 2025

dani-garcia Mar 7, 2025

dani-garcia Mar 7, 2025

quexten Mar 7, 2025

dani-garcia Mar 7, 2025

dani-garcia Mar 7, 2025

dani-garcia Mar 7, 2025

MGibson1 left a comment

quexten commented Mar 25, 2025

sonarqubecloud bot commented Mar 25, 2025

[PM-15096] Implement xchacha20-poly1305 cipher functions #150

[PM-15096] Implement xchacha20-poly1305 cipher functions #150

Conversation

quexten commented Feb 7, 2025 • edited Loading

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented Feb 7, 2025 • edited Loading

Great job, no security vulnerabilities found in this Pull Request

codecov bot commented Feb 7, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

quexten Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-garcia Mar 7, 2025 • 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

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

MGibson1 left a comment

Choose a reason for hiding this comment

quexten commented Mar 25, 2025

sonarqubecloud bot commented Mar 25, 2025

Quality Gate passed

quexten commented Feb 7, 2025 •

edited

Loading

github-actions bot commented Feb 7, 2025 •

edited

Loading

codecov bot commented Feb 7, 2025 •

edited

Loading

quexten Mar 7, 2025 •

edited

Loading

dani-garcia Mar 7, 2025 •

edited

Loading