test(929): pin by allocation that decode and clone do not parse a hostile ICC profile
Round-3 review: the record said every bound is enforced over compiled code, but half of the gwg2 (ICC CLUT) bound is ImageSharp 3.1.12's own runtime behaviour, which the IL scan cannot see. A 208-byte ICC profile declaring an mft2 CLUT (3 in, 15 out, 255 grid points), carried in PNG, JPEG and WebP, now goes through RemoteImageValidator.DecodeAndValidate + CloneFrame/Resize/CloneAs and through a real remote WatermarkElement. Each case asserts the profile is still attached and fails above 16 MiB allocated. The positive control reads Entries on the same profile and must allocate over 64 MiB. Measured 2026-10-28 on 3.1.12: the product paths allocate 38-94 KiB and the element 322-390 KiB; Entries allocates about 129,550 KiB. Mutation witnesses: a try/catch read of IccProfile.Entries injected after the validator's post-decode check reddens all 6 product-path cases on the allocation assertion. The same read after ImageElementBase's per-frame Resize reddens the 3 element cases. Both were restored from git. The record's gwg2 row and 'What holds the bounds' now name the two halves and which test holds each. docs/testing.md names the ICC cases. Decisions-Edit: yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: f9ce6bc6-5b36-491a-8d72-89c1f70dd3e1
This commit is contained in:
Claude Opus 5.5
@@ -4,8 +4,13 @@ using NUnit.Framework;
|
||||
using Shouldly;
|
||||
using SixLabors.ImageSharp;
|
||||
using SixLabors.ImageSharp.Formats;
|
||||
using SixLabors.ImageSharp.Formats.Jpeg;
|
||||
using SixLabors.ImageSharp.Formats.Png;
|
||||
using SixLabors.ImageSharp.Formats.Tiff;
|
||||
using SixLabors.ImageSharp.Formats.Webp;
|
||||
using SixLabors.ImageSharp.Metadata.Profiles.Icc;
|
||||
using SixLabors.ImageSharp.PixelFormats;
|
||||
using SixLabors.ImageSharp.Processing;
|
||||
using Image = SixLabors.ImageSharp.Image;
|
||||
|
||||
namespace ErsatzTV.Infrastructure.Tests.Images;
|
||||
@@ -89,6 +94,153 @@ public class ImageSharpDecodingTests
|
||||
() => RemoteImageValidator.DecodeAndValidate(stream, Uri, CancellationToken.None));
|
||||
}
|
||||
|
||||
// --- GHSA-gwg2-r3hj-4w44: the ICC CLUT parser allocates from unvalidated grid dimensions ---
|
||||
//
|
||||
// ErsatzTV never reads ICC entries (ImageSharpSurfaceTests bans the namespace), but decode,
|
||||
// CloneFrame, Resize and CloneAs all carry the profile along. Whether any of them parses it is
|
||||
// ImageSharp's own runtime behaviour, which an IL scan of ErsatzTV cannot see, so it is pinned
|
||||
// here by allocation. A version that parses the profile on decode or clone reddens these
|
||||
// instead of reopening the advisory silently.
|
||||
|
||||
private const long HostileIccEntriesFloor = 64L * 1024 * 1024;
|
||||
internal const long ProductPathCeiling = 16L * 1024 * 1024;
|
||||
|
||||
internal static readonly string[] IccCarriers = ["png", "jpeg", "webp"];
|
||||
|
||||
// positive control: the fixture really is hostile. Reading Entries runs the CLUT reader on the
|
||||
// declared 255-point, 3-input grid (about 127 MiB on 3.1.12, measured 2026-10-28); the catch
|
||||
// only tolerates the truncated table throwing afterwards.
|
||||
[TestCaseSource(nameof(IccCarriers))]
|
||||
[NonParallelizable]
|
||||
public async Task Hostile_Icc_Fixture_Allocates_When_Its_Entries_Are_Parsed(string format)
|
||||
{
|
||||
await using MemoryStream stream = await WithHostileIcc(format);
|
||||
using Image image = await Image.LoadAsync(ImageSharpDecoding.Options(), stream);
|
||||
IccProfile profile = image.Metadata.IccProfile.ShouldNotBeNull();
|
||||
|
||||
long allocated = AllocatedDuring(() =>
|
||||
{
|
||||
try
|
||||
{
|
||||
_ = profile.Entries;
|
||||
}
|
||||
catch (Exception)
|
||||
{
|
||||
// the allocation is what is measured, not whether the truncated table throws
|
||||
}
|
||||
});
|
||||
|
||||
TestContext.Out.WriteLine($"{format}: reading Entries allocated {allocated / 1024} KiB");
|
||||
allocated.ShouldBeGreaterThan(HostileIccEntriesFloor);
|
||||
}
|
||||
|
||||
// the product path: the validator's two decodes, then the per-frame CloneFrame + Resize +
|
||||
// CloneAs that ImageElementBase runs. The profile must survive the path (so the bound is not
|
||||
// vacuous because a decoder dropped it) and nothing on it may size the CLUT.
|
||||
[TestCaseSource(nameof(IccCarriers))]
|
||||
[NonParallelizable]
|
||||
public async Task Decode_Clone_And_Resize_Do_Not_Parse_A_Hostile_Icc_Profile(string format)
|
||||
{
|
||||
await using MemoryStream stream = await WithHostileIcc(format);
|
||||
|
||||
long allocated = await AllocatedDuringAsync(async () =>
|
||||
{
|
||||
using Image image = await RemoteImageValidator.DecodeAndValidate(stream, Uri, CancellationToken.None);
|
||||
image.Metadata.IccProfile.ShouldNotBeNull();
|
||||
|
||||
using Image frame = image.Frames.CloneFrame(0);
|
||||
frame.Mutate(ctx => ctx.Resize(8, 8));
|
||||
frame.Metadata.IccProfile.ShouldNotBeNull();
|
||||
|
||||
using Image<Rgba32> rgba = frame.CloneAs<Rgba32>();
|
||||
rgba.Metadata.IccProfile.ShouldNotBeNull();
|
||||
});
|
||||
|
||||
TestContext.Out.WriteLine($"{format}: decode, clone and resize allocated {allocated / 1024} KiB");
|
||||
allocated.ShouldBeLessThan(ProductPathCeiling);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A 16x16 image in <paramref name="format" /> carrying an ICC profile whose A2B0 tag is an
|
||||
/// <c>mft2</c> (lut16) with 3 inputs, 15 outputs and 255 grid points, truncated before the
|
||||
/// CLUT: a 208-byte profile.
|
||||
/// </summary>
|
||||
internal static async Task<MemoryStream> WithHostileIcc(string format)
|
||||
{
|
||||
using var image = new Image<Rgba32>(16, 16);
|
||||
image.Metadata.IccProfile = new IccProfile(HostileIccBytes());
|
||||
|
||||
IImageEncoder encoder = format switch
|
||||
{
|
||||
"png" => new PngEncoder(),
|
||||
"jpeg" => new JpegEncoder(),
|
||||
"webp" => new WebpEncoder { FileFormat = WebpFileFormatType.Lossless },
|
||||
_ => throw new ArgumentOutOfRangeException(nameof(format), format, null)
|
||||
};
|
||||
|
||||
var stream = new MemoryStream();
|
||||
await image.SaveAsync(stream, encoder);
|
||||
stream.Position = 0;
|
||||
return stream;
|
||||
}
|
||||
|
||||
private static byte[] HostileIccBytes()
|
||||
{
|
||||
// mft2: type, reserved, in/out/grid/pad, 3x3 matrix, input/output table entry counts,
|
||||
// then the three 2-entry input tables, and no CLUT
|
||||
var tag = new List<byte>();
|
||||
tag.AddRange("mft2"u8.ToArray());
|
||||
tag.AddRange(new byte[4]);
|
||||
tag.AddRange([3, 15, 255, 0]);
|
||||
for (var i = 0; i < 9; i++)
|
||||
{
|
||||
tag.AddRange(BigEndian(i % 4 == 0 ? 0x00010000u : 0u));
|
||||
}
|
||||
|
||||
tag.AddRange([0, 2, 0, 2]);
|
||||
tag.AddRange(new byte[3 * 2 * 2]);
|
||||
|
||||
const int TagOffset = 128 + 4 + 12;
|
||||
int size = TagOffset + tag.Count;
|
||||
|
||||
var bytes = new byte[size];
|
||||
BinaryPrimitives.WriteUInt32BigEndian(bytes.AsSpan(0), (uint)size);
|
||||
BinaryPrimitives.WriteUInt32BigEndian(bytes.AsSpan(8), 0x02100000u);
|
||||
"mntr"u8.CopyTo(bytes.AsSpan(12));
|
||||
"RGB "u8.CopyTo(bytes.AsSpan(16));
|
||||
"XYZ "u8.CopyTo(bytes.AsSpan(20));
|
||||
"acsp"u8.CopyTo(bytes.AsSpan(36));
|
||||
BinaryPrimitives.WriteUInt32BigEndian(bytes.AsSpan(128), 1);
|
||||
"A2B0"u8.CopyTo(bytes.AsSpan(132));
|
||||
BinaryPrimitives.WriteUInt32BigEndian(bytes.AsSpan(136), TagOffset);
|
||||
BinaryPrimitives.WriteUInt32BigEndian(bytes.AsSpan(140), (uint)tag.Count);
|
||||
tag.CopyTo(bytes, TagOffset);
|
||||
return bytes;
|
||||
}
|
||||
|
||||
private static byte[] BigEndian(uint value)
|
||||
{
|
||||
var bytes = new byte[4];
|
||||
BinaryPrimitives.WriteUInt32BigEndian(bytes, value);
|
||||
return bytes;
|
||||
}
|
||||
|
||||
// process-wide and precise, because the async path may resume on another thread; the callers
|
||||
// are [NonParallelizable] so no other test's allocations land in the count
|
||||
private static long AllocatedDuring(Action action)
|
||||
{
|
||||
long before = GC.GetTotalAllocatedBytes(precise: true);
|
||||
action();
|
||||
return GC.GetTotalAllocatedBytes(precise: true) - before;
|
||||
}
|
||||
|
||||
private static async Task<long> AllocatedDuringAsync(Func<Task> action)
|
||||
{
|
||||
long before = GC.GetTotalAllocatedBytes(precise: true);
|
||||
await action();
|
||||
return GC.GetTotalAllocatedBytes(precise: true) - before;
|
||||
}
|
||||
|
||||
internal static async Task<MemoryStream> RealTiff(int width, int height)
|
||||
{
|
||||
using var image = new Image<Rgba32>(width, height);
|
||||
|
||||
@@ -172,6 +172,27 @@ public class WatermarkElementRemoteImageTests
|
||||
}
|
||||
}
|
||||
|
||||
// GHSA-gwg2-r3hj-4w44 over the element itself rather than a replica of its calls: decode,
|
||||
// CloneFrame, Resize and CloneAs carry a hostile ICC profile through without sizing its CLUT.
|
||||
// The fixture's hostility is proven by ImageSharpDecodingTests'
|
||||
// Hostile_Icc_Fixture_Allocates_When_Its_Entries_Are_Parsed.
|
||||
// (ersatztv#929, security.imagesharp-3x-tiff-decoder-disabled)
|
||||
[TestCaseSource(typeof(ImageSharpDecodingTests), nameof(ImageSharpDecodingTests.IccCarriers))]
|
||||
[NonParallelizable]
|
||||
public async Task Should_Render_A_Remote_Watermark_Without_Parsing_Its_Icc_Profile(string format)
|
||||
{
|
||||
MemoryStream source = await ImageSharpDecodingTests.WithHostileIcc(format);
|
||||
var element = new WatermarkElement(RemoteWatermarkOptions(), FetcherReturning(source), Substitute.For<ILogger>());
|
||||
|
||||
long before = GC.GetTotalAllocatedBytes(precise: true);
|
||||
await element.InitializeAsync(Context(), CancellationToken.None);
|
||||
long allocated = GC.GetTotalAllocatedBytes(precise: true) - before;
|
||||
|
||||
TestContext.Out.WriteLine($"{format}: watermark initialization allocated {allocated / 1024} KiB");
|
||||
element.IsFinished.ShouldBeFalse();
|
||||
allocated.ShouldBeLessThan(ImageSharpDecodingTests.ProductPathCeiling);
|
||||
}
|
||||
|
||||
private static IRemoteImageFetcher FetcherReturning(Stream stream)
|
||||
{
|
||||
var fetcher = Substitute.For<IRemoteImageFetcher>();
|
||||
|
||||
@@ -33,7 +33,7 @@ rather than satisfying it.
|
||||
| GHSA-wmxv-xphr-5c9g (Moderate) | TIFF decoder, BigTIFF IFD loop | same — and the Identify pass, which walks the IFD, is the pass the tests pin |
|
||||
| GHSA-jjfr-hcj7-qf5w, GHSA-j9gm-c75j-xc9q (High) | TIFF CCITT T6/T4 **encoder** | production code never encodes TIFF (only test fixtures do) |
|
||||
| GHSA-j3p4-wp97-rph4 (High) | `HistogramEqualization` processor | never called; the only processor is `Resize` |
|
||||
| GHSA-gwg2-r3hj-4w44 (Moderate) | ICC CLUT parser | on 3.x reached only through `ImageMetadata.IccProfile.Entries`, which nothing reads; decode alone does not parse the CLUT |
|
||||
| GHSA-gwg2-r3hj-4w44 (Moderate) | ICC CLUT parser | two halves. ErsatzTV never reads `IccProfile.Entries`, the accessor that runs the parser. And on 3.1.12 decode, `CloneFrame`, `Resize` and `CloneAs` carry a profile through without parsing it — ImageSharp's own runtime behaviour, measured 2026-10-28 |
|
||||
|
||||
The ImageSharp decode surface is exactly `RemoteImageValidator.DecodeAndValidate` (remote logos at
|
||||
render time, logo URLs saved via #525, uploaded artwork) and the local-path load in
|
||||
@@ -41,8 +41,9 @@ render time, logo URLs saved via #525, uploaded artwork) and the local-path load
|
||||
clamped to PNG/JPEG/GIF/WebP by `ImageContentTypes`, so the only input this rejects is a remote-URL
|
||||
or on-disk TIFF graphic, which now disables its element the same way an unreachable URL does.
|
||||
|
||||
**What holds the bounds.** Every bound in the table is a property of how ErsatzTV calls
|
||||
ImageSharp, so each is enforced over the compiled code rather than left to review.
|
||||
**What holds the bounds.** Every bound in the table except one half of gwg2 is a property of how
|
||||
ErsatzTV calls ImageSharp, so each is enforced over the compiled code rather than left to review;
|
||||
the gwg2 runtime half is pinned by allocation (below).
|
||||
`ImageSharpSurfaceTests` (`ErsatzTV.Architecture.Tests`) reads the IL of every non-test project in
|
||||
`ErsatzTV.sln` whose project-reference closure contains the ImageSharp package — the population is
|
||||
derived from the solution and csproj graph, and a project that joins it without being referenced
|
||||
@@ -74,7 +75,18 @@ header-only TIFF declaring 30000x30000 exercises the Identify pass (a TIFF-aware
|
||||
report the size and fail the pixel budget instead). The watermark fixture exercises the local and
|
||||
remote element paths with PNG twins. Identify rejects a TIFF before Load runs, so these
|
||||
behavioural fixtures exercise the Identify pass; that the Load pass uses the same options is held
|
||||
by `ImageSharpSurfaceTests`. One shape is outside every check: a `Save(path)` that picks its
|
||||
by `ImageSharpSurfaceTests`.
|
||||
|
||||
The gwg2 runtime half is ImageSharp code an IL scan of ErsatzTV cannot see, so it is pinned by
|
||||
behaviour. A 208-byte ICC profile declares an `mft2` CLUT of 255 grid points over 3 inputs, and it is
|
||||
carried in a PNG, a JPEG and a WebP. `ImageSharpDecodingTests` sends it through the validator plus
|
||||
`CloneFrame`/`Resize`/`CloneAs`, and the watermark fixture through a real remote `WatermarkElement`.
|
||||
Each fails above 16 MiB allocated, and each asserts that the profile is still attached, so a decoder
|
||||
that drops it cannot pass vacuously. The positive control reads `Entries` on the same profile and
|
||||
must allocate over 64 MiB (about 127 MiB on 3.1.12). So a version that starts parsing profiles on
|
||||
decode or clone reddens.
|
||||
|
||||
One shape is outside every check: a `Save(path)` that picks its
|
||||
encoder from the file extension. There is no ImageSharp encode in production code (2026-10-27).
|
||||
|
||||
**The scheduled scan stays red on ImageSharp until the revisit.** `NuGetAuditSuppress` silences the
|
||||
|
||||
+1
-1
@@ -18,7 +18,7 @@ before adding tests, not just `docs/contributing.md` §8 (which now just points
|
||||
| `ErsatzTV.Tests` | API controllers + MediatR handlers, plus the SkiaSharp text-overlay rasteriser (`Infrastructure/Graphics/`) | In-memory SQLite fixture: a shared `SqliteConnection("Data Source=:memory:;Foreign Keys=False")` kept open + `EnsureCreatedAsync()` (**not** full migration replay) + `PRAGMA foreign_keys=OFF`, then seed; a tiny `IDbContextFactory` wraps `new TvContext(...)`. ~1,870 tests (approximate on purpose — an exact count goes stale on every PR that adds one; the previous hardcoded 828 was off by over a thousand). |
|
||||
| `ErsatzTV.Core.Tests` | Domain logic, scheduling, IPTV/XMLTV generation | References `ErsatzTV.Application` directly — there is no separate `Application.Tests` project. ~650 tests + 1 skipped under `TZ=UTC` (the Block playout golden additionally skips under a non-UTC `TZ`; see Golden-file nets). |
|
||||
| `ErsatzTV.Scanner.Tests` | Library scanning: scan handlers, folder scanners, NFO readers | Handler tests substitute the folder scanners + `ILibraryRepository` and assert the resulting repository writes (e.g. `ScanLocalLibraryHandlerTests` pins which `LastScan` levels a scan records — ersatztv#264). Fakes/`Testably` back the file-system-facing scanners. ~1,485 tests (approximate on purpose — an exact count goes stale on every PR that adds one). Additionally contains `Core/FFmpeg/TranscodingTests` — `[Explicit]` + `[Combinatorial]`, so it never runs in CI or a plain `dotnet test` (it needs real ffmpeg/hardware) and contributes 0 to that count; run it by name when touching the transcoding pipeline. |
|
||||
| `ErsatzTV.Infrastructure.Tests` | Infrastructure services tested without a host: remote image fetching, decode bounds and logo caching, the ImageSharp decode configuration, remote stream probing, the Jellyfin API client, entity locking, local statistics, search query parsing, sequential schedule validation, graphics opacity expressions | The HTTP-facing fixtures use the in-process handler and stream doubles in `Streaming/HttpTestDoubles.cs`, never the network. `ImageSharpDecodingTests` derives its expected format list from `Configuration.Default`, so an ImageSharp bump that adds a format reddens (`security.imagesharp-3x-tiff-decoder-disabled`). |
|
||||
| `ErsatzTV.Infrastructure.Tests` | Infrastructure services tested without a host: remote image fetching, decode bounds and logo caching, the ImageSharp decode configuration, remote stream probing, the Jellyfin API client, entity locking, local statistics, search query parsing, sequential schedule validation, graphics opacity expressions | The HTTP-facing fixtures use the in-process handler and stream doubles in `Streaming/HttpTestDoubles.cs`, never the network. `ImageSharpDecodingTests` derives its expected format list from `Configuration.Default`, so an ImageSharp bump that adds a format reddens; its hostile-ICC cases (and the watermark fixture's) pin by allocation that decode and clone do not parse an ICC profile (`security.imagesharp-3x-tiff-decoder-disabled`). |
|
||||
| `ErsatzTV.Architecture.Tests` | Rules over compiled assemblies: layering (NetArchTest.eNhancedEdition), provider-statics wiring, and the ImageSharp call surface (Mono.Cecil IL scan) | `LayeringTests`: Core↛Infra/App/EF; FFmpeg↛all; App↛concrete providers — see `docs/contributing.md` §1. `ImageSharpSurfaceTests` reads the IL of every non-test assembly whose project graph reaches ImageSharp (derived from `ErsatzTV.sln` and the csproj references, so the project references the host `ErsatzTV` too) and rejects the call shapes `security.imagesharp-3x-tiff-decoder-disabled` names. No exact count on purpose — it goes stale on every PR that adds a rule. |
|
||||
| `ErsatzTV.FFmpeg.Tests` | FFmpeg command construction | Build a pipeline, assert the exact rendered arg string (`PipelineBuilderBaseTests.cs`). |
|
||||
| `web/` (vitest) | React SPA unit tests | Run alongside typecheck + build (see below). Collects `src/**`, `web/scripts/**` *and* `web/vite-plugins/**`, but deliberately **excludes** `web/e2e/**` (the Playwright specs — vitest's default `**/*.spec.*` glob would otherwise run them under jsdom). Some files have git prerequisites since ersatztv#819, and the set is not fixed — ersatztv#883 added one. `web/src/api/pageSizeCallSites.guard.test.ts` and `web/src/api/completeAnnotations.guard.test.ts` need a git **checkout** AND, through it, the **binary**: they derive their file population from `git ls-files` via `web/vite-plugins/trackedSourceFiles.ts` rather than a directory walk, and refuse rather than falling back. `web/vite-plugins/trackedSourceFiles.realgit.test.ts` needs the **binary** but no checkout — it builds its own temp repository to prove that derivation by executing it. So it is not checkout-versus-binary: supplying a `.git` alone would not let any of them run. **The suite therefore runs only where git is present, and `docker/Dockerfile` is not such a place** — its web-build stage builds the SPA and does not test it (ersatztv#887). Enumerating the git-dependent files as Docker `--exclude`s was tried and REVERSED: that list is a population nothing derives, it went stale the first time a guard was added, and the resulting red is unreachable on a PR — `Build & push image (amd64)` is `if: github.event_name != 'pull_request'` — so it landed on `main` and on the release tag instead. The image is gated on `docker-build.yml`'s `test` job running the whole suite on a real checkout, held by `scripts/tests/test_image_build_delegates_the_spa_suite.py`. |
|
||||
|
||||
Reference in New Issue
Block a user