fix(511): bound remote graphics-engine image fetches (timeout, size cap, decode cap, redirects, pooling) #518

Merged

timothy merged 4 commits from fix/511-remote-image-hardening into main 2026-07-21 01:47:20 +02:00

Conversation 1 Commits 4 Files Changed 13

+1358 -106

timothy commented 2026-07-21 00:26:57 +02:00

Owner

Copy Link

Copy Source

fixes #511

What

ImageElementBase.LoadImage fetched remote (http(s)) images with a throwaway new HttpClient() + GetStreamAsync — no timeout override (the 100s default), no size cap, unbounded redirects, no pooling — all inside stream startup, while ffmpeg waits on the pipe. #502 routed ordinary channel-logo watermarks onto that path, which is what made a pre-existing weakness worth hardening.

New IRemoteImageFetcher / HttpRemoteImageFetcher, deliberately modelled on the neighbouring IRemoteStreamProber (Core interface + Infrastructure impl over IHttpClientFactory + linked-CTS deadline):

Done-when	How
Bounded timeout threaded to the stream read	Linked CTS CancelAfter(10s) into GetAsync and every body read; client Timeout = InfiniteTimeSpan (set on the instance, not only in DI) — under ResponseHeadersRead the body read falls outside HttpClient.Timeout (#289 lesson)
Size cap + content-type check	10 MiB enforced during the copy (Content-Length is only a cheap early reject — it can be absent or lie); rejects a positively-non-image type, allows missing/octet-stream
Pooled client	IHttpClientFactory, named client; redirects capped at 3 (default 50)
Decision on caching + redirect/host policy	docs/decisions.md
Dead/slow/oversized URL degrades to "watermark disabled"	WatermarkElementRemoteImageTests

The decode bomb (found by adversarial review of the first version)

A byte cap does not bound decoding — a bomb is small on the wire by definition. A 4 KB PNG can declare 30000×30000 (~3.6 GB to decode) and sails through Content-Length, the content-type check and the 10 MiB cap in milliseconds. The first version of this change capped bytes and claimed that was decode-bomb protection, in a comment and in append-only decisions.md.

ImageElementBase.DecodeRemoteImage now reads declared dimensions + frame count from the header (Image.IdentifyAsync) and rejects before Image.LoadAsync allocates: 50 MP (8K is ~33 MP) and 600 frames (every frame is scaled to frame size and retained, so the real cost is frames × output resolution). Local images are exempt — files an operator put on disk, not bytes an arbitrary host returned.

Deliberately not done

• No caching — re-fetched per element init (per playout item). With pooling, a 10s ceiling and a 10 MiB cap, one small GET per item transition doesn't justify a cache's invalidation policy, and it keeps #502's "external artwork passes through, it is not downloaded into the image cache" convention intact.
• SSRF accepted, not mitigated — ValidateLogo still only checks the scheme. The load-bearing reason is that the primitive is blind (the body is never returned to any user, only composited into a video frame; failures surface as a log line); admin-only is the weaker half of the argument. LAN-hosted logos are a legitimate setup here, so a private-IP denylist would break working installs. Revisit if logo URLs ever become settable through a lower-privilege path.

Verification

• Full solution: 3,981 tests pass, 0 failed (incl. NetArchTest layering).
• Negative-controlled the size caps: disabling both checks → build succeeded → exactly the 2 cap tests failed.
• Closed a vacuity hole the reviewer specifically probed for: the copy-cap test asserts Position > 0 so it cannot pass via the Content-Length early-reject path instead.
• Cold-context adversarial review before push (verdict MERGEABLE); its highest-value check — a missed construction site or second DI composition root — came back clean (2 sites, 2 subclasses, IGraphicsEngine registered only in Startup.cs; the Scanner never resolves it).
• Format gate run with CI's exact command (dotnet format whitespace . --folder --verify-no-changes --include) → pass; no BOMs.

Docs

docs/decisions.md — new entry + index line. No route/endpoint/SPA changes, so no other doc is owed.

fixes #511 ## What `ImageElementBase.LoadImage` fetched remote (`http(s)`) images with a throwaway `new HttpClient()` + `GetStreamAsync` — no timeout override (the 100s default), no size cap, unbounded redirects, no pooling — all **inside stream startup, while ffmpeg waits on the pipe**. #502 routed ordinary channel-logo watermarks onto that path, which is what made a pre-existing weakness worth hardening. New `IRemoteImageFetcher` / `HttpRemoteImageFetcher`, deliberately modelled on the neighbouring `IRemoteStreamProber` (Core interface + Infrastructure impl over `IHttpClientFactory` + linked-CTS deadline): | Done-when | How | |---|---| | Bounded timeout threaded to the stream read | Linked CTS `CancelAfter(10s)` into `GetAsync` **and** every body read; client `Timeout = InfiniteTimeSpan` (set on the instance, not only in DI) — under `ResponseHeadersRead` the body read falls outside `HttpClient.Timeout` (#289 lesson) | | Size cap + content-type check | 10 MiB enforced **during the copy** (`Content-Length` is only a cheap early reject — it can be absent or lie); rejects a positively-non-image type, allows missing/`octet-stream` | | Pooled client | `IHttpClientFactory`, named client; redirects capped at 3 (default 50) | | Decision on caching + redirect/host policy | `docs/decisions.md` | | Dead/slow/oversized URL degrades to "watermark disabled" | `WatermarkElementRemoteImageTests` | ## The decode bomb (found by adversarial review of the first version) A byte cap does **not** bound decoding — a bomb is small on the wire by definition. A 4 KB PNG can declare 30000×30000 (~3.6 GB to decode) and sails through `Content-Length`, the content-type check and the 10 MiB cap in milliseconds. The first version of this change capped bytes and *claimed* that was decode-bomb protection, in a comment **and** in append-only `decisions.md`. `ImageElementBase.DecodeRemoteImage` now reads declared dimensions + frame count from the header (`Image.IdentifyAsync`) and rejects before `Image.LoadAsync` allocates: **50 MP** (8K is ~33 MP) and **600 frames** (every frame is scaled to frame size and retained, so the real cost is frames × output resolution). Local images are exempt — files an operator put on disk, not bytes an arbitrary host returned. ## Deliberately not done - **No caching** — re-fetched per element init (per playout item). With pooling, a 10s ceiling and a 10 MiB cap, one small GET per item transition doesn't justify a cache's invalidation policy, and it keeps #502's "external artwork passes through, it is not downloaded into the image cache" convention intact. - **SSRF accepted, not mitigated** — `ValidateLogo` still only checks the scheme. The load-bearing reason is that the primitive is **blind** (the body is never returned to any user, only composited into a video frame; failures surface as a log line); admin-only is the weaker half of the argument. LAN-hosted logos are a legitimate setup here, so a private-IP denylist would break working installs. Revisit if logo URLs ever become settable through a lower-privilege path. ## Verification - Full solution: **3,981 tests pass**, 0 failed (incl. NetArchTest layering). - **Negative-controlled** the size caps: disabling both checks → build succeeded → exactly the 2 cap tests failed. - Closed a vacuity hole the reviewer specifically probed for: the copy-cap test asserts `Position > 0` so it cannot pass via the `Content-Length` early-reject path instead. - Cold-context adversarial review before push (verdict MERGEABLE); its highest-value check — a missed construction site or second DI composition root — came back clean (2 sites, 2 subclasses, `IGraphicsEngine` registered only in `Startup.cs`; the Scanner never resolves it). - Format gate run with CI's exact command (`dotnet format whitespace . --folder --verify-no-changes --include`) → pass; no BOMs. ## Docs `docs/decisions.md` — new entry + index line. No route/endpoint/SPA changes, so no other doc is owed.

timothy force-pushed fix/511-remote-image-hardening from 74dfc24d85 to 269fb2dcdd 2026-07-21 00:49:54 +02:00 Compare

timothy force-pushed fix/511-remote-image-hardening from 269fb2dcdd to d4e112f1e9 2026-07-21 01:05:31 +02:00 Compare

timothy added 1 commit 2026-07-21 01:28:17 +02:00

fix(511): don't let the header pre-pass break animated PNG logos ... 5616fa6de5

Fourth adversarial pass cleared the security design — all three earlier
bypasses are dead, DecoderOptions.MaxFrames is honored by every decoder
that can produce multiple frames (GIF/WebP/TIFF exactly N, APNG N-1),
and it bounds PEAK allocation, not just the final frame count (measured:
65 MiB capped vs 2.41 GiB uncapped on the same 600-frame GIF).

But it caught a functional regression this PR introduced: a *default*
`Image.IdentifyAsync` throws InvalidImageContentException on most APNGs
that `Image.Load` reads back perfectly — including files ImageSharp's
own PngEncoder wrote. Reproduced independently: 13 of 16 shapes throw,
and `MaxFrames = 1` on the Identify fixes all 16 with dimensions intact.
Since #502 routes ordinary channel-logo watermarks through this path, an
admin with an animated PNG logo would have silently lost their watermark
to a log line — a hardening change breaking working content.

The existing tests could not see it: they use 64x64, which happens to be
one of the few shapes a default Identify handles. Now pinned with a
288x288 shape that asserts the default Identify DOES fail and that
DecodeRemoteImage decodes it anyway, in full.

Also, from the same pass:
- document the REAL enforced peak (up to 3x the nominal 50 MP budget,
  since detecting "over the limit" means decoding past it) instead of
  restating the nominal number. Tightening the single-frame allowance to
  budget/3 would reject legitimate 8K stills, so the overshoot is
  deliberate; it is ~600 MB against the ~36 GiB it replaces
- correct the MaxFrames off-by-one claim: N-1 is APNG-specific, not
  universal, so the stated rationale for +2 was wrong for three of the
  four animated formats

timothy commented 2026-07-21 01:42:15 +02:00

Author

Owner

Copy Link

Copy Source

Independent adversarial review — five rounds

Review-verdict: MERGEABLE @ 5616fa6de5ce45b644cb02f4cdcc7d7f712aebf8

Cold-context independent review, re-run against each fix commit per the #242 convention. It found three genuine security bypasses and one functional regression; the fifth round found nothing.

Round	Verdict	Finding
1	MERGEABLE	Byte cap ≠ decode-bomb protection — a bomb is small on the wire by definition
2	BLOCKED	Independent caps don't compose: 2500×2500 × 600 = 60 KiB wire → ~14 GiB decode, passing both a 50 MP dimension check and a 600-frame check
3	BLOCKED	Budget sourced from the header: APNG Identify reports 0 frames while Load produces 600 → 4000×4000 × 600 = 134 KiB → ~36 GiB, charged as one frame
4	BLOCKED	Security design cleared, but the new Identify pre-pass threw on 13 of 16 APNG shapes that load fine — silently breaking animated-PNG logos that worked before this PR
5	MERGEABLE	No Blocker/High/Medium. Could not defeat it with crafted files from an independent encoder (PIL) or hand-patched chunk headers

The generalizable lesson (recorded in decisions.md, not just fixed)

A limit the decoder does not enforce is not a limit. Rounds 2 and 3 both failed the same way — the budget was checked against a number the decoder doesn't honor. The fix is structural: bound the decoder (DecoderOptions.MaxFrames) and re-verify against the decoded reality, so correctness no longer depends on any format's header being honest.

Round 5 verified that property directly: MaxFrames is honored by every decoder that can produce multiple frames (GIF/WebP/TIFF exactly N, APNG N-1), and it bounds peak allocation, not just the final frame count — measured 65 MiB capped vs 2.41 GiB uncapped on the same 600-frame GIF.

The dimension-vs-frame-count split was also attacked and held: PNG validates fcTL geometry against IHDR, GIF clamps to the logical screen, TIFF refuses non-uniform pages. A file cannot report small dimensions to the pre-pass and decode larger.

Round 4's regression is the one I'd highlight

A hardening change silently breaking working content is the failure mode worth naming. My tests couldn't see it because they used 64×64 — one of the few APNG shapes a default Identify handles. Now pinned with a 288×288 shape that asserts the default Identify does throw and that DecodeRemoteImage decodes it anyway, in full. If a future ImageSharp fixes the upstream bug, that test goes red and forces a deliberate decision rather than silently testing nothing.

Verification

• 4,058 tests pass; CI green.
• Negative-controlled both size caps and the retention-check wiring (build success verified first, so no stale-dll false pass — the #250 trap).
• Independently reproduced the APNG header lie, the MaxFrames off-by-one, and the Identify regression before fixing each, rather than taking the review's word for it.

Residual, accepted (documented)

• Enforced peak is up to 3× the nominal 50 MP budget (~600 MB), because detecting "over the limit" requires decoding past it. Tightening the single-frame allowance to budget/3 would reject legitimate 8K stills (33 MP), so the overshoot is deliberate.
• Retention budget is per element, no global ceiling across concurrent streams.
• 200 MP caps a full-frame 1080p animated overlay at ~96 frames — the one limit here that could bite a legitimate user rather than an attacker.
• L9 (informational, no action): multi-page TIFF safety is inherited from ImageSharp's hard "Images with different sizes are not supported". If that were ever relaxed, an 8×8 first page would let ~602 pages of 6000×6000 through. Worth re-checking on an ImageSharp bump.
• SSRF unmitigated by design — the primitive is blind (body never returned to any caller, only composited into a frame).

## Independent adversarial review — five rounds `Review-verdict: MERGEABLE @ 5616fa6de5ce45b644cb02f4cdcc7d7f712aebf8` Cold-context independent review, re-run against each fix commit per the #242 convention. It found **three genuine security bypasses and one functional regression**; the fifth round found nothing. | Round | Verdict | Finding | |---|---|---| | 1 | MERGEABLE | Byte cap ≠ decode-bomb protection — a bomb is small on the wire by definition | | 2 | **BLOCKED** | Independent caps don't compose: 2500×2500 × 600 = **60 KiB wire → ~14 GiB decode**, passing both a 50 MP dimension check and a 600-frame check | | 3 | **BLOCKED** | Budget sourced from the header: APNG `Identify` reports **0 frames** while `Load` produces 600 → 4000×4000 × 600 = **134 KiB → ~36 GiB**, charged as *one* frame | | 4 | **BLOCKED** | Security design cleared, but the new `Identify` pre-pass threw on **13 of 16 APNG shapes** that load fine — silently breaking animated-PNG logos that worked before this PR | | 5 | **MERGEABLE** | No Blocker/High/Medium. Could not defeat it with crafted files from an independent encoder (PIL) or hand-patched chunk headers | ### The generalizable lesson (recorded in `decisions.md`, not just fixed) **A limit the decoder does not enforce is not a limit.** Rounds 2 and 3 both failed the same way — the budget was checked against a number the decoder doesn't honor. The fix is structural: bound the **decoder** (`DecoderOptions.MaxFrames`) and **re-verify against the decoded reality**, so correctness no longer depends on any format's header being honest. Round 5 verified that property directly: `MaxFrames` is honored by every decoder that can produce multiple frames (GIF/WebP/TIFF exactly `N`, APNG `N-1`), and it bounds **peak allocation**, not just the final frame count — measured 65 MiB capped vs 2.41 GiB uncapped on the same 600-frame GIF. The dimension-vs-frame-count split was also attacked and held: PNG validates `fcTL` geometry against `IHDR`, GIF clamps to the logical screen, TIFF refuses non-uniform pages. A file cannot report small dimensions to the pre-pass and decode larger. ### Round 4's regression is the one I'd highlight A *hardening* change silently breaking working content is the failure mode worth naming. My tests couldn't see it because they used 64×64 — one of the few APNG shapes a default `Identify` handles. Now pinned with a 288×288 shape that asserts the default `Identify` **does** throw and that `DecodeRemoteImage` decodes it anyway, in full. If a future ImageSharp fixes the upstream bug, that test goes red and forces a deliberate decision rather than silently testing nothing. ### Verification - 4,058 tests pass; CI green. - Negative-controlled both size caps and the retention-check wiring (build success verified first, so no stale-dll false pass — the #250 trap). - Independently reproduced the APNG header lie, the `MaxFrames` off-by-one, and the `Identify` regression *before* fixing each, rather than taking the review's word for it. ### Residual, accepted (documented) - Enforced peak is up to **3× the nominal 50 MP** budget (~600 MB), because detecting "over the limit" requires decoding past it. Tightening the single-frame allowance to budget/3 would reject legitimate 8K stills (33 MP), so the overshoot is deliberate. - Retention budget is per element, no global ceiling across concurrent streams. - 200 MP caps a full-frame 1080p animated overlay at ~96 frames — the one limit here that could bite a legitimate user rather than an attacker. - **L9 (informational, no action):** multi-page TIFF safety is inherited from ImageSharp's hard `"Images with different sizes are not supported"`. If that were ever relaxed, an 8×8 first page would let ~602 pages of 6000×6000 through. Worth re-checking on an ImageSharp bump. - SSRF unmitigated by design — the primitive is *blind* (body never returned to any caller, only composited into a frame).

timothy referenced this pull request 2026-07-21 01:42:34 +02:00

Harden remote-image fetching in the graphics engine (timeout, size cap, redirects, pooling, caching, SSRF) #511

timothy merged commit be0e2b1f1f into main 2026-07-21 01:47:20 +02:00

timothy deleted branch fix/511-remote-image-hardening 2026-07-21 01:47:21 +02:00

timothy referenced this issue from a commit 2026-07-21 01:47:22 +02:00

Merge pull request 'fix(511): bound remote graphics-engine image fetches (timeout, size cap, decode cap, redirects, pooling)' (#518) from fix/511-remote-image-hardening into main

timothy referenced this pull request 2026-07-21 01:47:55 +02:00

Harden remote-image fetching in the graphics engine (timeout, size cap, redirects, pooling, caching, SSRF) #511

timothy referenced this pull request 2026-07-21 01:48:39 +02:00

ChicoryTV rewrite — queue tracker (goal, arc, session protocol) #237

timothy referenced this pull request 2026-07-21 02:01:59 +02:00

docs(511): correct the stale "external logo URL can't drive the bug" claim + document the new limits #522

timothy referenced this pull request 2026-07-21 02:42:02 +02:00

External channel-logo URLs: download + cache at save time instead of fetching at render time #525

timothy referenced this pull request 2026-07-21 03:01:35 +02:00

feat(521): decision-lifecycle machinery — keyed records, validator, generated catalog (PR1 of 2) #526

timothy referenced this pull request 2026-07-21 12:19:17 +02:00

feat(525): external channel-logo URLs download + cache at save time (not fetched at render) #528

timothy referenced this pull request 2026-07-21 15:37:11 +02:00

External channel-logo URLs: download + cache at save time instead of fetching at render time #525

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#518