P1 security: q and fit are outside the HMAC signature, allowing 500x cache/transcode amplification from one signed URL #60

New Issue

clawbot · 2026-08-09T03:42:53+02:00

clawbot commented

2026-08-09 03:42:53 +02:00

Verified against main at 61f42e6.

Signer.buildSignatureData (internal/signature/signature.go:99-111) signs exactly:

host:path:query:width:height:format:expiration

Neither q (quality) nor fit is included. But internal/handlers/image.go:50-63 reads both from the query string after signature validation, and both are part of the variant cache key (internal/imgcache/storage.go:327-328 — req.Quality, req.FitMode).

Consequence: a single valid signed URL can be replayed with q=1..100 and fit in {cover,contain,fill,inside,outside}, yielding 500 distinct cache entries and 500 libvips transcodes that the signature was never meant to authorize. That is disk amplification against the cache budget #51 just introduced, plus CPU/memory amplification against the processing path — from one URL, by anyone who has seen it.

This also makes the README false. README.md:69-73 states signatures are "exact match only: every component (host, path, query, dimensions, format, expiration) must match exactly what was signed", and the repo has an explicit exact-match-only enforcement commitment from #40.

Definition of done

Decide and implement one of:
- (a) include quality and fit in the signed payload (preferred — makes the README's claim true), or
- (b) reject any non-default q/fit on signature-authenticated requests with a 400.
If (a): the signed-data format changes, so update the format string, README.md's signature specification section (both the field list and the worked example), and regenerate/extend the golden vectors in internal/signature/golden_test.go. Note this is a breaking change to the URL signing scheme — any external signer must be updated in lockstep. Call that out in the PR body.
Failing test first per repo TDD rules: a test proving that a URL signed for q=85 is rejected (or produces a different signature) when replayed with q=40.
Existing exact-match tests from #40 must still pass unmodified.
make check green.

Notes

The /v1/e/ encrypted-URL path is unaffected by this specific issue (the whole payload is sealed), but see the separate issue on /v1/e/ input validation.
Option (a) is recommended: option (b) leaves the documented guarantee narrower than it reads and keeps quality/fit unauthenticated-but-ignored, which is easy to regress later.

Verified against `main` at `61f42e6`. `Signer.buildSignatureData` (`internal/signature/signature.go:99-111`) signs exactly: ``` host:path:query:width:height:format:expiration ``` Neither `q` (quality) nor `fit` is included. But `internal/handlers/image.go:50-63` reads both from the query string *after* signature validation, and both are part of the variant cache key (`internal/imgcache/storage.go:327-328` — `req.Quality`, `req.FitMode`). Consequence: a single valid signed URL can be replayed with `q=1..100` and `fit` in {`cover`,`contain`,`fill`,`inside`,`outside`}, yielding **500 distinct cache entries and 500 libvips transcodes** that the signature was never meant to authorize. That is disk amplification against the cache budget #51 just introduced, plus CPU/memory amplification against the processing path — from one URL, by anyone who has seen it. This also makes the README false. `README.md:69-73` states signatures are "**exact match only**: every component (host, path, query, dimensions, format, expiration) must match exactly what was signed", and the repo has an explicit exact-match-only enforcement commitment from #40. ## Definition of done 1. Decide and implement one of: - **(a)** include quality and fit in the signed payload (preferred — makes the README's claim true), or - **(b)** reject any non-default `q`/`fit` on signature-authenticated requests with a 400. 2. If (a): the signed-data format changes, so update the format string, `README.md`'s signature specification section (both the field list and the worked example), and regenerate/extend the golden vectors in `internal/signature/golden_test.go`. Note this is a **breaking change to the URL signing scheme** — any external signer must be updated in lockstep. Call that out in the PR body. 3. Failing test first per repo TDD rules: a test proving that a URL signed for `q=85` is rejected (or produces a different signature) when replayed with `q=40`. 4. Existing exact-match tests from #40 must still pass unmodified. 5. `make check` green. ## Notes - The `/v1/e/` encrypted-URL path is unaffected by this specific issue (the whole payload is sealed), but see the separate issue on `/v1/e/` input validation. - Option (a) is recommended: option (b) leaves the documented guarantee narrower than it reads and keeps quality/fit unauthenticated-but-ignored, which is easy to regress later.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:42:53 +02:00

clawbot referenced this issue

2026-08-09 16:10:39 +02:00

Tracking: 1.0.0 sequencing and critical path #103

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/pixa#60