Skip to content

Fix out-of-bounds writing to browser messages - #13679

Draft
varjolintu wants to merge 2 commits into
developfrom
fix/browser_messaging_limits
Draft

varjolintu wants to merge 2 commits into
developfrom
fix/browser_messaging_limits

Conversation

@varjolintu

@varjolintu varjolintu commented Sep 15, 2026

Copy link
Copy Markdown
Member

Adds safeguards when encrypting or decrypting messages for browser integration / native messaging.

Socket/pipes are transferring base64 encoded messages that are always larger than the actual message. The socket buffer must remain with 1MB (TODO).

Both encrypt and decrypt now checks if the message, public key and nonce lenghts are valid. For encryption, the size of the buffer must be size+16 (this is what crypto_box_easy() requires). For decryption, the base64 decoded buffer holds that extra 16 bytes, and the buffer for decrypted data is added with that amount.

Note that this change does not handle possible fragmented messages.

Testing strategy

Automatic tests added for situations where key and nonce lengths are not valid. Modified the encrypt/decrypt functions to take the max length as a parameter, so proper tests can be constructed. Tested the limits by reducing the max length of the message.

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)

@varjolintu
varjolintu marked this pull request as draft September 15, 2026 16:56
@varjolintu
varjolintu force-pushed the fix/browser_messaging_limits branch 3 times, most recently from f2c11d5 to 7004bc1 Compare September 15, 2026 17:21
@varjolintu
varjolintu force-pushed the fix/browser_messaging_limits branch from 7004bc1 to 148440b Compare September 15, 2026 17:39
@codecov

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.36620% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.18%. Comparing base (84658ad) to head (148440b).

Files with missing lines Patch % Lines
src/browser/BrowserHost.cpp 0.00% 2 Missing ⚠️
src/proxy/NativeMessagingProxy.cpp 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #13679      +/-   ##
===========================================
+ Coverage    63.14%   63.18%   +0.04%     
===========================================
  Files          388      388              
  Lines        41119    41157      +38     
===========================================
+ Hits         25963    26005      +42     
+ Misses       15156    15152       -4     

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

@phoerious
phoerious requested a balanced review from Copilot September 15, 2026 18:11

This comment was marked as outdated.

@phoerious
phoerious requested a balanced review from Copilot September 15, 2026 18:20

This comment was marked as outdated.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The changes affect cryptographic buffer sizing and messaging boundaries, warranting final human review.

Suppressed comments (2)

src/browser/BrowserMessageBuilder.cpp:33

  • This hardcodes the 16-byte MAC overhead even though the sodium API exposes crypto_box_MACBYTES (and the previous implementation used it). Keeping a separate literal can make the allocation and output length diverge from crypto_box_easy() if the implementation constant changes; derive this constant from crypto_box_MACBYTES instead.
#define EXTRA_BYTES 16 // For crypto_box_easy()

tests/TestBrowser.cpp:140

  • Succesful is misspelled in this newly added test comment; please use Successful.
    // Succesful decryption
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread tests/TestBrowser.cpp Outdated
@keepassxreboot keepassxreboot deleted a comment from sigure21 Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants