Skip to content

Security: harden auth, dlimit enforcement, and crypto#1

Merged
jaschadub merged 1 commit into
masterfrom
security/audit-fixes
May 22, 2026
Merged

Security: harden auth, dlimit enforcement, and crypto#1
jaschadub merged 1 commit into
masterfrom
security/audit-fixes

Conversation

@jaschadub
Copy link
Copy Markdown
Member

Summary

Security audit of the server-side code surfaced several concrete, exploitable issues. This PR fixes them. See per-file notes below.

Crypto

  • app/keychain.js — password-protected files derived the HMAC auth key with PBKDF2-HMAC-SHA-256 / 100 iterations. Raised to 100,000 (1000× factor). OWASP currently recommends ≥600k; 100k is a UX-balanced jump that nonetheless removes the offline-brute-force trivial-cost concern. Older password-protected links uploaded by previous clients will not be auth-compatible with the new client; since files expire within at most 7 days this is acceptable.

Auth gating actually honored

  • server/config.jsconfig.fxa_required was referenced in middleware/auth.js:75 and routes/ws.js:37 but never declared in the convict schema, so FXA_REQUIRED=true was silently a no-op. Added the key with env FXA_REQUIRED.

Download-count enforcement (TOCTOU)

  • server/routes/download.js + server/middleware/auth.js — N concurrent download requests sharing one Authorization header all read the same meta.dl, all streamed, then all incremented the counter afterward — effectively bypassing dlimit. Moved counting to an atomic HINCRBY before streaming, with refund on stream error / client cancel / setup failure. Also awaits the nonce-rotation write in the HMAC middleware.

Authorization-header strictness

  • server/routes/upload.js + server/routes/ws.js — a malformed header (no space) made auth.split(' ')[1] evaluate to undefined, which Redis serialized as the literal string "undefined". The HMAC verification key then became Buffer.from("undefined", "base64") — a known, short, deterministic buffer any party could forge against. Now both endpoints require send-v1 <base64> and reject otherwise.

Input validation on download count

  • server/routes/params.js + server/routes/ws.jsdlimit accepted negatives, floats, and non-numbers. A negative value made the first download immediately delete the file. Now requires Number.isInteger(dlimit) && 1 <= dlimit <= max_downloads.

S3 storage isolation

  • server/storage/s3.jsAWS.config.update(cfg) mutated the SDK config process-wide, potentially redirecting unrelated AWS service clients to a custom S3 endpoint. Now passes cfg to new AWS.S3(cfg) only.

Plumbing

  • server/storage/redis.js — added promisified hsetAsync / hincrbyAsync.
  • server/storage/index.jssetField / incrementField now return promises so race-sensitive callers can await.

Reviewed and intentionally not changed

  • Host-header trust under DETECT_BASE_URL=true — opt-in operator footgun.
  • base_url default of https://send.example.com — misconfiguration, not a vulnerability.
  • FxA verify() delegating to a remote /verify instead of local JWT signature verification — design tied to the upstream FxA flow; would need a JWKS-based verifier (out of scope here).
  • Path traversal in storage/fs.js — adequately gated by route-level [0-9a-fA-F]{10,16} regexes and server-prepended prefix.
  • AES-GCM with zero IV in keychain.encryptMetadata — safe because the key (rawSecret) is randomized per file.

Test plan

  • npm install && npm run lint
  • npm run test:backend
  • Manual smoke: upload via web client, download once (counter increments), confirm file deletes when dlimit reached.
  • Manual smoke: upload a password-protected file with the new client, download it with the new client (auth header should validate via the new 100k-iteration PBKDF2).
  • Concurrent-download regression: with dlimit=1, issue two parallel GET /api/download/:id requests sharing the same fresh Authorization header — exactly one should succeed.
  • Set FXA_REQUIRED=true with FXA_CLIENT_ID configured; anonymous upload should 401.
  • Send malformed Authorization: foo to POST /api/upload — should 400.

- keychain: raise PBKDF2 iterations from 100 to 100000 for password
  protected file auth-key derivation
- config: declare fxa_required (FXA_REQUIRED) so the auth gate in
  middleware/auth.js and routes/ws.js is actually honored
- download: atomically claim a dl slot via HINCRBY before streaming
  so concurrent requests sharing an Authorization header cannot
  exceed dlimit; refund on cancel or error
- auth middleware: await nonce rotation write before responding
- upload + ws: reject Authorization headers that are not
  send-v1 <base64>, preventing a stored "undefined" auth key
- params + ws: require dlimit to be a positive integer <= max
- storage: make setField/incrementField return promises
  (promisified hset/hincrby) so callers can await
- s3 storage: pass endpoint/path-style config to new AWS.S3(cfg)
  instead of mutating global AWS.config
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