Skip to content

[PM-32597] - create short-lived signed attachment URL for self-hosted instances#7100

Open
jaasen-livefront wants to merge 1 commit intomainfrom
VULN-386
Open

[PM-32597] - create short-lived signed attachment URL for self-hosted instances#7100
jaasen-livefront wants to merge 1 commit intomainfrom
VULN-386

Conversation

@jaasen-livefront
Copy link
Collaborator

🎟️ Tracking

https://bitwarden.atlassian.net/browse/VULN-386

📔 Objective

In self-hosted Bitwarden, cipher attachment download URLs were predictable (/attachments/{cipherId}/{attachmentId}) and served by a static file server with no authentication. An attacker with knowledge of a cipher ID and attachment ID could download encrypted attachment files without authorization.

Changes

Signed download URLs for local storage — When FileUploadType is Direct (local/self-hosted), CipherService.GetAttachmentDownloadDataAsync now generates a time-limited, cryptographically signed token using ASP.NET Core Data Protection instead of returning a bare static file path. Tokens expire after 1 minute, matching the short-lived nature of Azure SAS URLs used in cloud deployments.

New download endpoint — Added GET /ciphers/attachment/download?token=... ([AllowAnonymous]) to CiphersController. This endpoint validates the signed token, resolves the cipher and attachment, and streams the file. The token itself is the authorization — it can only be obtained through the existing authenticated GET /ciphers/{id}/attachment/{attachmentId} endpoint.

GetAttachmentReadStreamAsync — Added to IAttachmentStorageService and all implementations. LocalAttachmentStorageService returns a FileStream; Azure and Noop implementations return null (Azure uses SAS URLs directly).

Null check hardeningGetAttachmentData now explicitly throws NotFoundException when the cipher is not found.

Security considerations

  • No change to the client-facing API contract — GET /ciphers/{id}/attachment/{attachmentId} still requires [Authorize("Application")]
  • Token payload is {cipherId}|{attachmentId}, encrypted and time-limited via ITimeLimitedDataProtector
  • Expired/invalid/malformed tokens all return 404 (no information leakage)
  • Azure (cloud) deployments are unaffected — they already use time-limited SAS URLs
  • The static file server in util/Server should separately be configured to stop serving the /attachments directory, but this change ensures that even if it does, the API no longer returns those predictable URLs

Files changed

File Change
CipherService.cs Signed URL generation, ParseAttachmentDownloadToken, CreateAttachmentDownloadProtector
ICipherService.cs Added CreateAttachmentDownloadProtector()
IAttachmentStorageService.cs Added GetAttachmentReadStreamAsync
LocalAttachmentStorageService.cs Implemented GetAttachmentReadStreamAsync
AzureAttachmentStorageService.cs Implemented GetAttachmentReadStreamAsync (returns null)
NoopAttachmentStorageService.cs Implemented GetAttachmentReadStreamAsync (returns null)
CiphersController.cs Added DownloadAttachmentAsync endpoint, null check in GetAttachmentData
CiphersControllerTests.cs 6 new tests
CipherServiceTests.cs 8 new tests

📸 Screenshots

@jaasen-livefront jaasen-livefront requested a review from a team as a code owner February 27, 2026 01:01
@jaasen-livefront jaasen-livefront changed the title create short-lived signed attachment URL for self-hosted instances [VULN-386] - create short-lived signed attachment URL for self-hosted instances Feb 27, 2026
@sonarqubecloud
Copy link

@github-actions
Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Detailse91542f8-9045-449e-972c-42e3c9d39db9

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Feb 27, 2026

Codecov Report

❌ Patch coverage is 64.28571% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.74%. Comparing base (44e993e) to head (e55e255).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/Api/Vault/Controllers/CiphersController.cs 69.56% 6 Missing and 1 partial ⚠️
...s/Implementations/LocalAttachmentStorageService.cs 0.00% 7 Missing ⚠️
...re/Vault/Services/Implementations/CipherService.cs 85.29% 5 Missing ⚠️
...s/Implementations/AzureAttachmentStorageService.cs 0.00% 3 Missing ⚠️
...oopImplementations/NoopAttachmentStorageService.cs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7100      +/-   ##
==========================================
- Coverage   56.76%   56.74%   -0.02%     
==========================================
  Files        2014     2013       -1     
  Lines       88218    88239      +21     
  Branches     7855     7864       +9     
==========================================
- Hits        50073    50069       -4     
- Misses      36321    36348      +27     
+ Partials     1824     1822       -2     

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

@jaasen-livefront jaasen-livefront changed the title [VULN-386] - create short-lived signed attachment URL for self-hosted instances [PM-32597] - create short-lived signed attachment URL for self-hosted instances Mar 2, 2026
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.

1 participant