Skip to content

[PM-2460/24639] Allow empty arrays, and fix string plaintext length hiding #379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

quexten
Copy link
Contributor

@quexten quexten commented Aug 11, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-24640
https://bitwarden.atlassian.net/browse/PM-24639

📔 Objective

"" is a valid value to encrypt, and the current vault code in clients sometimes does encrypt "" (and sometimes just returns null). This was not anticipated when writing the padding initially. This changes the padding to allow padding empty byte arrays.

Further, it seems the block padding for strings was done incorrectly and only hides the first block's plaintext length, but afterwards has a 1:1 correlation to plaintext length:

Before:

String Length -> EncString Length
================================
  0 ->  194
  1 ->  194
  2 ->  194
  3 ->  194
  4 ->  194
  5 ->  194
  6 ->  194
  7 ->  194
  8 ->  194
  9 ->  194
 10 ->  194
 11 ->  194
 12 ->  194
 13 ->  194
 14 ->  194
 15 ->  194
 16 ->  194
 17 ->  194
 18 ->  194
 19 ->  194
 20 ->  194
 21 ->  194
 22 ->  194
 23 ->  194
 24 ->  194
 25 ->  194
 26 ->  194
 27 ->  194
 28 ->  194
 29 ->  194
 30 ->  194
 31 ->  194
 32 ->  198
 33 ->  198
 34 ->  198
 35 ->  202
 36 ->  202
 37 ->  202
 38 ->  206
 39 ->  206
 40 ->  206
 41 ->  210
 42 ->  210
 43 ->  210
 44 ->  214
 45 ->  214
 46 ->  214
 47 ->  218
 48 ->  218
 49 ->  218
 50 ->  222
 51 ->  222
 52 ->  222
 53 ->  226
 54 ->  226
 55 ->  226
 56 ->  230
 57 ->  230
 58 ->  230
 59 ->  234
 60 ->  234
 61 ->  234
 62 ->  238
 63 ->  238
 64 ->  238
 65 ->  242
 66 ->  242
 67 ->  242
 68 ->  246
 69 ->  246
 70 ->  246
 71 ->  250
 72 ->  250
 73 ->  250
 74 ->  254
 75 ->  254
 76 ->  254
 77 ->  258
 78 ->  258
 79 ->  258
 80 ->  262

After:

String Length -> EncString Length
================================
  0 ->  194
  1 ->  194
  2 ->  194
  3 ->  194
  4 ->  194
  5 ->  194
  6 ->  194
  7 ->  194
  8 ->  194
  9 ->  194
 10 ->  194
 11 ->  194
 12 ->  194
 13 ->  194
 14 ->  194
 15 ->  194
 16 ->  194
 17 ->  194
 18 ->  194
 19 ->  194
 20 ->  194
 21 ->  194
 22 ->  194
 23 ->  194
 24 ->  194
 25 ->  194
 26 ->  194
 27 ->  194
 28 ->  194
 29 ->  194
 30 ->  194
 31 ->  194
 32 ->  238
 33 ->  238
 34 ->  238
 35 ->  238
 36 ->  238
 37 ->  238
 38 ->  238
 39 ->  238
 40 ->  238
 41 ->  238
 42 ->  238
 43 ->  238
 44 ->  238
 45 ->  238
 46 ->  238
 47 ->  238
 48 ->  238
 49 ->  238
 50 ->  238
 51 ->  238
 52 ->  238
 53 ->  238
 54 ->  238
 55 ->  238
 56 ->  238
 57 ->  238
 58 ->  238
 59 ->  238
 60 ->  238
 61 ->  238
 62 ->  238
 63 ->  238
 64 ->  282
 65 ->  282
 66 ->  282
 67 ->  282
 68 ->  282
 69 ->  282
 70 ->  282
 71 ->  282
 72 ->  282
 73 ->  282
 74 ->  282
 75 ->  282
 76 ->  282
 77 ->  282
 78 ->  282
 79 ->  282
 80 ->  282

Both of these changes don't break compatibility. However, even if they did, the code is not rolled out yet so it would be OK.

⏰ 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

@quexten quexten changed the title Allow empty arrays, and fix string plaintext length hiding [PM-2460/24639] Allow empty arrays, and fix string plaintext length hiding Aug 11, 2025
Copy link
Contributor

github-actions bot commented Aug 11, 2025

Logo
Checkmarx One – Scan Summary & Details5b01c2df-5b94-4732-ad67-573c7d40b95b

New Issues (1)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
LOW CVE-2025-54798 Npm-tmp-0.0.33
detailsRecommended version: 0.2.4
Description: tmp is a temporary file and directory creator for node.js. In versions prior to 0.2.4, tmp is vulnerable to an arbitrary temporary file "/" directo...
Attack Vector: LOCAL
Attack Complexity: HIGH

ID: YvcsBhvcXHJIADLCWJPbGW%2F8K3t4Ef3jdjFoSh7nE%2BM%3D
Vulnerable Package

Copy link

codecov bot commented Aug 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.22%. Comparing base (4813492) to head (15bcf15).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #379      +/-   ##
==========================================
+ Coverage   74.09%   74.22%   +0.13%     
==========================================
  Files         253      253              
  Lines       21781    21866      +85     
==========================================
+ Hits        16138    16231      +93     
+ Misses       5643     5635       -8     

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

@quexten quexten marked this pull request as ready for review August 11, 2025 13:42
@quexten quexten requested review from a team as code owners August 11, 2025 13:42
@quexten quexten requested review from coroiu and mzieniukbw August 11, 2025 13:42
Copy link
Member

@Hinton Hinton left a comment

Choose a reason for hiding this comment

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

Can we add a test confirming the padding improvements for values over > 192?

Copy link

@quexten
Copy link
Contributor Author

quexten commented Aug 11, 2025

@Hinton good call, I added the tests for the first 3 padding sizes.

@quexten quexten requested a review from Hinton August 11, 2025 15:21
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