[PM-36584] Bidirectional C2 in icons.bitwarden.net

[jengstrom-bw]

🎟️ Tracking

jira

📔 Objective

strip PNG metadata chunks
In IconLink.cs, FetchAsync just returns the raw bytes. Before we hand them back, we should walk the PNG chunk list and only keep the ones we actually need for rendering:

Keep: IHDR, PLTE, IDAT, IEND, tRNS, sRGB, gAMA, cHRM

Drop everything else. That kills tEXt/iTXt/zTXt and anything else an attacker might try. PNG chunk structure is simple (4-byte length, 4-byte type, data, CRC). Apply the same logic to PNG frames embedded inside ICO files.

Also drop SVG support from the proxy

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the PNG/ICO metadata stripping added to IconLink.cs, which removes non-essential PNG chunks (allowlisting IHDR, PLTE, IDAT, IEND, tRNS, sRGB, gAMA, cHRM) to eliminate tEXt/iTXt/zTXt-based bidirectional C2 channels, applies the same stripping to PNG frames embedded in ICO files, and drops SVG support from the proxy. Parsing is fail-closed: malformed PNG signatures, length overflows, missing IEND, and corrupt ICO offsets all return null so the icon is not served. Bounds checks correctly cast to long to avoid integer overflow, signed Int32 length reads reject the high-bit-set case, and the ICO parser caps entries at 64. Test coverage is thorough, covering chunk filtering, trailing-garbage truncation, overflow, missing-IEND, and corrupt/malformed embedded PNG cases.

Code Review Details

No new findings. All four prior review threads (fail-open → fail-closed on malformed PNG chunks, dead _svgXmlMediaType removal, ICO memory-amplification DoS cap, and redundant count == 0 check) are resolved.

The t:deps label is present, but the diff contains no dependency manifest changes (only an InternalsVisibleTo entry in Icons.csproj), so no dependency review applies.

[codecov]

Codecov Report

❌ Patch coverage is 81.18812% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.09%. Comparing base (766c33b) to head (edb977d).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Icons/Models/IconLink.cs	81.18%	11 Missing and 8 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7668      +/-   ##
==========================================
+ Coverage   59.98%   60.09%   +0.11%     
==========================================
  Files        2133     2132       -1     
  Lines       93731    93985     +254     
  Branches     8311     8355      +44     
==========================================
+ Hits        56220    56479     +259     
+ Misses      35531    35514      -17     
- Partials     1980     1992      +12     

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

[sonarqubecloud]

Quality Gate passed

Issues
3 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

[lockness-Ko]

RE: https://thecontractor.io/bitwarden-c2/

just an FYI this doesn't prevent the proxy from being used as a C2 channel. Attackers can still:

1. Embed malicious bytes in the image data, which can be mitigated by applying a layer of lossy compression to the image before serving it to the user. (see favicon_original.ico and favicon_modified.ico with the embedded command in, search for "MAGIC" in a hex editor)
2. Steganographic techniques, where an attacker crafts a specific image with a pattern (e.g. a QR code or other bespoke encoding method) to use as a channel to encode data.

I recommend you follow the other recommendations provided in John Carroll's article as well as the technique used by 1password, which appears to have a large cached list of favicons that only get populated when multiple different users request favicon for the same domain.

favicon_modified.zip