Skip to content

refactor(crypto): implement AES256-CBC, AES256-CBC-HMAC256 hazmat primitives - #1263

Merged
quexten merged 1 commit into
mainfrom
km/aes-hazmat
Jul 28, 2026
Merged

refactor(crypto): implement AES256-CBC, AES256-CBC-HMAC256 hazmat primitives#1263
quexten merged 1 commit into
mainfrom
km/aes-hazmat

Conversation

@quexten

@quexten quexten commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Implements aes256-cbc (as legacy compat for old user-keys), and AES256-CBC-HMAC-SHA256 in the hazmat module. This will replace the aes.rs file at the crate root. These implementations are close to being zero-copy. Only the plaintext buffer that is passed out is allocated.

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

@quexten
quexten force-pushed the km/aes-hazmat branch 2 times, most recently from ee8473c to 4c7be80 Compare July 15, 2026 09:45
@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

🔍 SDK Breaking Change Detection

SDK Version: km/aes-hazmat (29fc53b)

⚠️ If breaking changes are detected, a corresponding pull request addressing them must be ready for merge in the affected client repository.

Client Status Details
typescript ✅ No breaking changes detected Compilation passed with new SDK version - View Details
android ✅ No breaking changes detected Compilation passed with new SDK version - View Details

Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm.
Results update as workflows complete.

@codecov

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.23404% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.80%. Comparing base (6ae3f66) to head (09353d7).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
.../symmetric_encryption/aes256_cbc_hmac_sha256_ae.rs 90.32% 9 Missing ⚠️
...ypto/src/hazmat/symmetric_encryption/aes256_cbc.rs 86.66% 6 Missing ⚠️
...rden-crypto/src/hazmat/symmetric_encryption/mod.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #1263    +/-   ##
========================================
  Coverage   85.79%   85.80%            
========================================
  Files         487      490     +3     
  Lines       69999    70140   +141     
========================================
+ Hits        60059    60182   +123     
- Misses       9940     9958    +18     

☔ View full report in Codecov by Harness.
📢 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.


impl From<SymmetricEncryptionError> for CryptoError {
fn from(_: SymmetricEncryptionError) -> Self {
CryptoError::KeyDecrypt

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update in the future. Please note, mac should ALWAYS be checked before the padding, otherwise we may allow a padding oracle.

@quexten
quexten requested a review from eligrubb July 22, 2026 13:21
@quexten
quexten marked this pull request as ready for review July 22, 2026 13:21
@quexten
quexten requested a review from a team as a code owner July 22, 2026 13:21
Copilot AI review requested due to automatic review settings July 22, 2026 13:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@quexten
quexten marked this pull request as draft July 22, 2026 13:21
@quexten
quexten marked this pull request as ready for review July 22, 2026 13:25
@quexten

quexten commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Despite the AEAD bits living on this stack, do not feel the need to wait for it. The refactor can merge even without the aead stuff.

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the new aes256_cbc and aes256_cbc_hmac_sha256_ae hazmat primitives and the symmetric_encryption/mod.rs wiring. These reimplement the legacy type 0 (unauthenticated CBC) and type 2 (Encrypt-then-MAC CBC) EncString ciphers ahead of replacing the crate-root aes.rs. The implementation preserves backward-compatible behavior: identical HMAC-SHA256 MAC computation, MAC verified before decryption/padding (no padding oracle), PKCS7 padding, and the enc-key-then-mac-key composite key layout. Test vectors lock in the serialized formats and cover MAC-tampering and invalid-padding failure paths.

Code Review Details

No blocking findings.

Notes considered and cleared during review:

  • MAC-before-padding ordering is correct in the new type 2 decrypt path, preventing a padding oracle. This aligns with the existing thread on mod.rs.
  • The ciphertext.to_vec() allocation in decrypt is intentional and documented in the PR description ("Only the plaintext buffer that is passed out is allocated").
  • MAC computation is byte-identical to the existing aes.rs (hmac::Hmac<sha2::Sha256> in both), preserving decryptability of existing data.

This PR carries the breaking-change label and Android breaking-change detection fired; that is expected given the crate-root aes.rs will be replaced, and is tracked outside this diff.

@quexten
quexten merged commit 50417ae into main Jul 28, 2026
63 checks passed
@quexten
quexten deleted the km/aes-hazmat branch July 28, 2026 04:20
bw-ghapp Bot added a commit to bitwarden/sdk-swift that referenced this pull request Jul 28, 2026
… implement AES256-CBC, AES256-CBC-HMAC256 hazmat primitives (bitwarden/sdk-internal#1263)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants