From 5616fa6de5ce45b644cb02f4cdcc7d7f712aebf8 Mon Sep 17 00:00:00 2001 From: Timothy Date: Tue, 21 Jul 2026 01:28:13 +0200 Subject: [PATCH] fix(511): don't let the header pre-pass break animated PNG logos MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../Graphics/RemoteImageDecodeLimitTests.cs | 24 ++++++++++++ .../Graphics/Image/ImageElementBase.cs | 21 ++++++++--- docs/decisions.md | 37 ++++++++++++++----- 3 files changed, 67 insertions(+), 15 deletions(-) diff --git a/ErsatzTV.Infrastructure.Tests/Streaming/Graphics/RemoteImageDecodeLimitTests.cs b/ErsatzTV.Infrastructure.Tests/Streaming/Graphics/RemoteImageDecodeLimitTests.cs index b865b60bc..6154b62e3 100644 --- a/ErsatzTV.Infrastructure.Tests/Streaming/Graphics/RemoteImageDecodeLimitTests.cs +++ b/ErsatzTV.Infrastructure.Tests/Streaming/Graphics/RemoteImageDecodeLimitTests.cs @@ -183,6 +183,30 @@ public class RemoteImageDecodeLimitTests image.Frames.Count.ShouldBe(300); } + + // H2 regression: a default `Image.Identify` throws InvalidImageContentException on most APNGs + // (measured: 13 of 16 shapes, including ones ImageSharp's own encoder wrote) even though + // `Image.Load` reads them back perfectly. 288x288 is one of the throwing shapes; 64x64 x300 -- + // used by the tests above -- happens NOT to be, which is exactly why they could not see this. + // Without the MaxFrames=1 workaround on the Identify, every animated-PNG logo that worked + // before this change would be silently disabled. (ersatztv#511, fourth re-review.) + [Test] + public async Task Should_Decode_An_Apng_That_A_Default_Identify_Cannot_Read() + { + await using MemoryStream stream = Apng(288, 288, 60); + + // the premise: a default Identify really does fail on this file + stream.Position = 0; + await Should.ThrowAsync(() => Image.IdentifyAsync(stream)); + + stream.Position = 0; + using Image image = await ImageElementBase.DecodeRemoteImage(stream, ImageUri, CancellationToken.None); + + image.Width.ShouldBe(288); + image.Height.ShouldBe(288); + image.Frames.Count.ShouldBe(60); + } + /// A real multi-frame APNG. Small on the wire, many frames — the shape that matters. private static MemoryStream Apng(int width, int height, int frames) { diff --git a/ErsatzTV.Infrastructure/Streaming/Graphics/Image/ImageElementBase.cs b/ErsatzTV.Infrastructure/Streaming/Graphics/Image/ImageElementBase.cs index 32eb8da7a..d323027d4 100644 --- a/ErsatzTV.Infrastructure/Streaming/Graphics/Image/ImageElementBase.cs +++ b/ErsatzTV.Infrastructure/Streaming/Graphics/Image/ImageElementBase.cs @@ -155,7 +155,15 @@ public abstract class ImageElementBase(IRemoteImageFetcher remoteImageFetcher) : + "return a fully buffered, seekable stream"); } - ImageInfo info = await Image.IdentifyAsync(stream, cancellationToken); + // MaxFrames = 1 on the IDENTIFY is not a limit, it is a workaround: a default Identify + // throws InvalidImageContentException on most APNGs — including files ImageSharp's own + // PngEncoder wrote, which Image.Load then reads back perfectly (measured: 13 of 16 shapes). + // Without this, adding the header pre-pass would silently disable every animated-PNG logo + // that worked before this change. Only Width/Height are read below, and those stay correct. + ImageInfo info = await Image.IdentifyAsync( + new DecoderOptions { MaxFrames = 1 }, + stream, + cancellationToken); // DIMENSIONS from the header are trustworthy; the FRAME COUNT is not, and is deliberately // not used as a budget input. Measured on ImageSharp 3.1.12: an APNG reports @@ -168,11 +176,12 @@ public abstract class ImageElementBase(IRemoteImageFetcher remoteImageFetcher) : stream.Position = 0; - // MaxFrames is enforced BY THE DECODER, so it holds whatever the header claimed. Measured: - // MaxFrames = N yields N-1 frames (for N >= 2), so asking for affordableFrames + 2 decodes - // at most affordableFrames + 1 — one more than allowed, which is exactly what lets the - // post-decode check distinguish "exactly at the limit" from "over it" without truncating a - // legitimate animation by a frame. + // MaxFrames is enforced BY THE DECODER, so it holds whatever the header claimed — measured + // as honored by every animated decoder here (APNG, GIF, WebP, TIFF). Ask for two more than + // the budget allows so that an animation exactly AT the limit still decodes in full, while + // anything over it is present in the decoded image for the post-decode check below to + // reject. Slop is at most two frames: MaxFrames = N yields N frames for GIF/WebP/TIFF but + // N-1 for APNG, so the exact count varies by format and only the upper bound matters. var decoderOptions = new DecoderOptions { MaxFrames = (uint)(affordableFrames + 2) }; Image image = await Image.LoadAsync(decoderOptions, stream, cancellationToken); diff --git a/docs/decisions.md b/docs/decisions.md index ceb4f3992..c36b173fc 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -2642,10 +2642,13 @@ So `DecodeRemoteImage` now: checks the header **dimensions** (which are trustwor image descriptor exceeds its logical screen is clamped by the decoder, verified); derives how many frames of that size the budget affords; passes that to **`DecoderOptions.MaxFrames`**, which the decoder enforces regardless of what the header claimed; and then **re-verifies the real -`image.Frames.Count`** after decoding, disposing and rejecting if it is over. Note -`MaxFrames = N` yields `N-1` frames (measured), so the code asks for `affordable + 2` — decoding -one more than allowed is what distinguishes "exactly at the limit" from "over it" without silently -truncating a legitimate animation. +`image.Frames.Count`** after decoding, disposing and rejecting if it is over. `MaxFrames` was +measured as honored by every animated decoder in play (APNG, GIF, WebP, TIFF), which is what makes +it a real bound rather than another advisory one. The code asks for `affordable + 2` so an +animation exactly at the limit still decodes in full while anything over it is visible to the +post-decode check; the slop is at most two frames, since `MaxFrames = N` yields `N` frames for +GIF/WebP/TIFF but `N-1` for APNG — the exact count varies by format, so only the upper bound is +relied on. - **Decode budget** — `width x height x frames <= 50 MP`, verified against the decoded image. - **Retention budget** — `frames x scaledWidth x scaledHeight <= 200 MP` (~800 MB at 4 bytes/px), @@ -2653,11 +2656,27 @@ truncating a legitimate animation. source is trivial to decode but retains ~5 GB of `SKBitmap` at 600 frames scaled to 1920x1080, because `LoadImage` clones and resizes every frame to output resolution and keeps them. -Caveats worth knowing: the budgets are in pixels, but a 16-bit PNG decodes to `Rgba64` (8 B/px), so -the byte cost doubles there; the retention budget is per element, with no global ceiling across -concurrent streams; and 200 MP caps a full-frame 1080p animated overlay at ~96 frames (~3.2s at -30fps), which is the one limit here that could plausibly bite a legitimate user rather than an -attacker. +**The enforced peak is up to 3x the nominal decode budget, and that is deliberate.** Detecting +"over the limit" requires actually decoding more frames than the limit allows, so the ceiling is +`(affordable + 2) x perFrame`. In the pathological case — one frame that alone fills the budget, +so `affordable = 1` — that is 150 MP (~600 MB at Rgba32, ~1.2 GB for a 16-bit TIFF at Rgba64) +rather than 50 MP. Bounded and survivable, against the ~36 GiB it replaces, and the alternative +(tightening the single-frame allowance to budget/3) would reject legitimate 8K stills at 33 MP. +Stated here because the previous three versions of this entry each claimed a bound the code did +not actually enforce. + +Other caveats: the budgets are in pixels, but a 16-bit PNG decodes to `Rgba64` (8 B/px), so the +byte cost doubles; the retention budget is per element, with no global ceiling across concurrent +streams; and 200 MP caps a full-frame 1080p animated overlay at ~96 frames (~3.2s at 30fps), which +is the one limit here that could plausibly bite a legitimate user rather than an attacker. + +**A workaround rides along with the header pre-pass.** `Image.IdentifyAsync` is called with +`MaxFrames = 1` — not as a limit, but because a *default* `Identify` throws +`InvalidImageContentException` on most APNGs (measured: 13 of 16 shapes, including files +ImageSharp's own `PngEncoder` wrote) that `Image.Load` reads back perfectly. Adding the pre-pass +without it would have silently disabled every animated-PNG logo that worked before this change — +a functional regression introduced *by* a hardening change, caught only because the reviewer swept +shapes rather than trusting the one the tests happened to use. Both budgets are enforced by pure functions (`EnsureDecodeAffordable`, `EnsureScaledFramesAffordable`) so the arithmetic is tested at every boundary without materializing multi-gigabyte images, and both