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