fix(uploads): bound image decode with explicit Limits, closing the unbounded GIF path [operator-action] (LC-206-IMAGE-LIMITS) #288

Merged

longjacksonle merged 1 commit from fix/lc-206-image-limits into main

2026-05-31 05:07:22 +02:00

longjacksonle commented

2026-05-31 05:05:55 +02:00

Owner

Closes #284 (LC-206-IMAGE-LIMITS). Filed investigate-first; the spike confirmed a real unbounded-decode path, so this applies the fix.

Investigation result

image 0.25.10 Limits::default() caps still-image max_alloc at 512 MiB, BUT GifDecoder::new initialises Limits::no_limits() - the GIF decode path in uploads::pipeline was unbounded.
The byte caps (10 MiB upload / 1 MiB avatar) bound the input FILE, not the decoded pixel buffer. A small high-ratio GIF (tiny LZW of a huge constant frame, or a long animation) decodes to an arbitrarily large buffer.
The avatar proxy fetches attacker-controlled foreign GIFs through this same decoder at the 1 MiB cap, so the vector is remotely reachable, not just self-upload. This is a remote DoS (memory exhaustion / crash) in shipped versions.
The still path was bounded but loose: 512 MiB x THUMBNAIL_CONCURRENCY (4) is ~2 GiB transient on a burst, and the bound was an implicit crate default. A spike comment in-tree already wrongly asserted "image 0.25 has no default Limits" - the kind of stale assumption a version bump silently breaks.

Fix

A shared decode_limits() (256 MiB max_alloc + 16384 dimension caps) is applied at every decode site in pipeline.rs: ImageReader::limits() on the two still paths, GifDecoder::set_limits() on the GIF path. 256 MiB admits a ~64 MP RGBA image (covers real photos under the 10 MiB cap) while rejecting bombs; the bound no longer depends on the image-crate default, and the GIF hole is closed. The LC-206 grep-ban already forces all decoders through pipeline.rs, so coverage is structural.

Tests

uploads_pipeline.rs:

png_decompression_bomb_rejected_by_limits: a VALID 12000x12000 PNG (~410 KB on disk, under the 10 MiB byte cap; ~432 MB decoded) is rejected specifically as PipelineError::Decode(ImageError::Limits(_)) - asserting the LIMIT fires, not malformedness.
constant_png_builder_produces_a_decodable_image: the same builder at 8x8 decodes fine, proving the bomb fixture is well-formed (so the rejection is the limit, not a broken file).
The existing animated-GIF round-trip test still passes - set_limits does not break legit GIFs.

The stale spike comment is corrected; CLAUDE.md gains an "Image decoders need explicit Limits" gotcha. flate2 added as a dev-dependency (already in-tree transitively) to stream the bomb fixture's zero scanlines without materialising 432 MB.

Validation

just test + just test-saas + just check all green (both modes clippy -D warnings, fmt, desktop).

Operator action

Security fix present in shipped versions - upgrade promptly. An attacker-supplied GIF decompression/animation bomb fetched via the foreign-avatar proxy (or uploaded) could exhaust server memory and crash the process. No configuration change required; the fix is the explicit decode Limits in this release.

🤖 Generated with Claude Code

Closes #284 (LC-206-IMAGE-LIMITS). Filed investigate-first; the spike confirmed a real unbounded-decode path, so this applies the fix. ## Investigation result - `image` 0.25.10 `Limits::default()` caps still-image `max_alloc` at 512 MiB, BUT `GifDecoder::new` initialises `Limits::no_limits()` - the GIF decode path in `uploads::pipeline` was **unbounded**. - The byte caps (10 MiB upload / 1 MiB avatar) bound the input FILE, not the decoded pixel buffer. A small high-ratio GIF (tiny LZW of a huge constant frame, or a long animation) decodes to an arbitrarily large buffer. - The **avatar proxy fetches attacker-controlled foreign GIFs** through this same decoder at the 1 MiB cap, so the vector is remotely reachable, not just self-upload. This is a remote DoS (memory exhaustion / crash) in shipped versions. - The still path was bounded but loose: 512 MiB x `THUMBNAIL_CONCURRENCY` (4) is ~2 GiB transient on a burst, and the bound was an implicit crate default. A spike comment in-tree already wrongly asserted "image 0.25 has no default Limits" - the kind of stale assumption a version bump silently breaks. ## Fix A shared `decode_limits()` (256 MiB `max_alloc` + 16384 dimension caps) is applied at **every** decode site in `pipeline.rs`: `ImageReader::limits()` on the two still paths, `GifDecoder::set_limits()` on the GIF path. 256 MiB admits a ~64 MP RGBA image (covers real photos under the 10 MiB cap) while rejecting bombs; the bound no longer depends on the image-crate default, and the GIF hole is closed. The LC-206 grep-ban already forces all decoders through `pipeline.rs`, so coverage is structural. ## Tests `uploads_pipeline.rs`: - `png_decompression_bomb_rejected_by_limits`: a VALID 12000x12000 PNG (~410 KB on disk, under the 10 MiB byte cap; ~432 MB decoded) is rejected specifically as `PipelineError::Decode(ImageError::Limits(_))` - asserting the LIMIT fires, not malformedness. - `constant_png_builder_produces_a_decodable_image`: the same builder at 8x8 decodes fine, proving the bomb fixture is well-formed (so the rejection is the limit, not a broken file). - The existing animated-GIF round-trip test still passes - `set_limits` does not break legit GIFs. The stale spike comment is corrected; CLAUDE.md gains an "Image decoders need explicit Limits" gotcha. `flate2` added as a dev-dependency (already in-tree transitively) to stream the bomb fixture's zero scanlines without materialising 432 MB. ## Validation `just test` + `just test-saas` + `just check` all green (both modes clippy `-D warnings`, fmt, desktop). ## Operator action Security fix present in shipped versions - **upgrade promptly**. An attacker-supplied GIF decompression/animation bomb fetched via the foreign-avatar proxy (or uploaded) could exhaust server memory and crash the process. No configuration change required; the fix is the explicit decode `Limits` in this release. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

longjacksonle added 1 commit

2026-05-31 05:05:55 +02:00

fix(uploads): bound image decode with explicit Limits, closing the unbounded GIF path [operator-action] (LC-206-IMAGE-LIMITS, #284 )

check-secrets / Nosey parker (push) Successful in 3s

Details

check-secrets / Nosey parker (pull_request) Successful in 5s

Details

check-secrets / Kingfisher (push) Successful in 4s

Details

check-secrets / TruffleHog (push) Successful in 5s

Details

check-secrets / Kingfisher (pull_request) Successful in 8s

Details

check-secrets / TruffleHog (pull_request) Successful in 10s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

Check / clippy + fmt + tests (pull_request) Successful in 7m20s

Details

ede3a2d775

Investigate-first finding for #284: image 0.25's `Limits::default()` caps still-image allocation at 512 MiB, but `GifDecoder::new` starts at `Limits::no_limits()` - the GIF decode path in `uploads::pipeline` was UNBOUNDED. The byte caps (10 MiB upload, 1 MiB avatar) bound the input FILE, not the decoded pixel buffer, so a small high-ratio GIF (tiny LZW of a huge constant frame, or a long animation) could allocate without limit and OOM the process. The avatar proxy fetches attacker-controlled foreign GIFs through this same decoder at the 1 MiB cap, so the vector is remotely reachable, not just self-upload. The still path was bounded but loosely: 512 MiB x THUMBNAIL_CONCURRENCY (4 permits) is ~2 GiB of transient decode memory on a burst, and the bound was an implicit crate default a version bump could change (an in-tree spike comment already wrongly believed "image 0.25 has no default Limits").

Fix: a shared `decode_limits()` (256 MiB max_alloc + 16384 dimension caps) is now applied at every decode site in `pipeline.rs` - `ImageReader::limits()` on the two still paths and `GifDecoder::set_limits()` on the GIF path. 256 MiB admits a ~64 MP RGBA image (covers real photos under the 10 MiB cap) while rejecting bombs; the bound no longer depends on the image-crate default and the GIF hole is closed.

Tests (`uploads_pipeline.rs`): a VALID PNG decompression bomb (12000x12000, ~410 KB on disk under the 10 MiB byte cap, ~432 MB decoded) is rejected specifically with `PipelineError::Decode(ImageError::Limits(_))` - asserting the LIMIT fires, not malformedness, which a small-dimensions control of the same builder confirms by decoding fine. The existing animated-GIF test still passes, proving `set_limits` does not break legit GIFs. The stale "no default Limits" spike comment is corrected, and CLAUDE.md gains an "Image decoders need explicit Limits" gotcha (the #284 follow-up note). `flate2` added as a dev-dependency (already in-tree transitively) to stream the bomb fixture.

just test + just test-saas + just check all green.

Operator-Action: Security fix present in shipped versions - upgrade promptly. The desktop/bridge avatar proxy and the upload pipeline decoded GIFs through an unbounded decoder; an attacker-supplied GIF decompression/animation bomb (small file, huge decoded size) could exhaust server memory and crash the process (remote DoS via the foreign-avatar fetch path). No configuration change is required; the fix is the explicit decode Limits in this release.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>