[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: REQUEST CHANGES

This PR adds PNG chunk allowlisting and ICO-embedded PNG stripping to IconLink.FetchAsync to prevent bidirectional C2 in the icons proxy, and drops SVG from the allowed media types. The whitelist of chunk types (IHDR, PLTE, IDAT, IEND, tRNS, sRGB, gAMA, cHRM) matches the PR description, and test coverage exercises happy paths, trailing garbage, invalid signatures, overflow lengths, and corrupt ICO offsets. One concern: malformed PNG/ICO inputs are handled fail-open (the original untouched bytes are returned), which an attacker who controls the icon source can deliberately trigger to bypass the strip entirely.

Code Review Details

• ⚠️ : Fail-open on malformed PNG chunks lets a hostile upstream bypass metadata stripping
  • src/Icons/Models/IconLink.cs:250-253 (same pattern at lines 281, 288, 302 in StripIcoEmbeddedPngMetadata)

[claude]

⚠️ IMPORTANT: Fail-open on malformed PNG chunks lets an attacker bypass metadata stripping.

Details and fix

When a chunk has length < 0 or a length that overflows the buffer, StripPngMetadata returns the original bytes array unmodified. An attacker controlling the icon source can intentionally craft a PNG that places a malicious chunk (e.g. tEXt carrying C2 payload) early in the stream, followed by a chunk with a deliberately invalid length. The parser hits this branch, bails, and the full original byte array — including the malicious chunk — is delivered to the client.

This directly undermines the PR's stated goal of dropping anything "an attacker might try." Since the threat model assumes the upstream server is hostile, a malformed PNG should be treated as untrusted, not preserved.

The same fail-open pattern appears in StripIcoEmbeddedPngMetadata at the early-return branches for invalid count, entry size, and data offset.

Suggested fix: on parse failure, return the bytes already written to output plus a synthetic IEND (or return an empty array and let the caller drop the icon), so malformed input fails closed rather than open.

[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

[codecov]

Codecov Report

❌ Patch coverage is 84.88372% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.00%. Comparing base (766c33b) to head (7c45a84).

Files with missing lines	Patch %	Lines
src/Icons/Models/IconLink.cs	84.88%	7 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7668      +/-   ##
==========================================
+ Coverage   59.98%   60.00%   +0.02%     
==========================================
  Files        2133     2133              
  Lines       93731    93815      +84     
  Branches     8311     8325      +14     
==========================================
+ Hits        56220    56296      +76     
- Misses      35531    35535       +4     
- Partials     1980     1984       +4     

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