From ccd66b6a5efc258b12966043f7fcc247d26a67ed Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 30 Aug 2023 12:01:07 +1000 Subject: [PATCH] Handle EOF in Jpeg bit reader when data is bad to prevent DOS attack. (#2516) * Handle EOF in bit reader when data is bad. * Allow parallel processing of multi-megapixel image * Stream seek can exceed the length of a stream * Try triggering on release branches * Update JpegBitReader.cs * Skin on Win .NET 6 * All Win OS is an issue * Address feedback * add validation to CanIterateWithoutIntOverflow --------- Co-authored-by: antonfirsov --- .github/workflows/build-and-test.yml | 1 + .../Advanced/ParallelRowIterator.cs | 10 ++--- .../Jpeg/Components/Decoder/JpegBitReader.cs | 7 +++- .../Formats/Jpg/JpegDecoderTests.cs | 17 ++++++++ .../Helpers/ParallelRowIteratorTests.cs | 39 +++++++++++++++++++ tests/ImageSharp.Tests/TestImages.cs | 1 + .../Images/Input/Jpg/issues/Hang_C438A851.jpg | 3 ++ 7 files changed, 72 insertions(+), 6 deletions(-) create mode 100644 tests/Images/Input/Jpg/issues/Hang_C438A851.jpg diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index b5cc5daca2..853cad738b 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -9,6 +9,7 @@ on: pull_request: branches: - main + - release/* types: [ labeled, opened, synchronize, reopened ] jobs: Build: diff --git a/src/ImageSharp/Advanced/ParallelRowIterator.cs b/src/ImageSharp/Advanced/ParallelRowIterator.cs index 0eb5952a63..657654a84b 100644 --- a/src/ImageSharp/Advanced/ParallelRowIterator.cs +++ b/src/ImageSharp/Advanced/ParallelRowIterator.cs @@ -50,7 +50,7 @@ public static void IterateRows( int width = rectangle.Width; int height = rectangle.Height; - int maxSteps = DivideCeil(width * height, parallelSettings.MinimumPixelsProcessedPerTask); + int maxSteps = DivideCeil(width * (long)height, parallelSettings.MinimumPixelsProcessedPerTask); int numOfSteps = Math.Min(parallelSettings.MaxDegreeOfParallelism, maxSteps); // Avoid TPL overhead in this trivial case: @@ -115,7 +115,7 @@ public static void IterateRows( int width = rectangle.Width; int height = rectangle.Height; - int maxSteps = DivideCeil(width * height, parallelSettings.MinimumPixelsProcessedPerTask); + int maxSteps = DivideCeil(width * (long)height, parallelSettings.MinimumPixelsProcessedPerTask); int numOfSteps = Math.Min(parallelSettings.MaxDegreeOfParallelism, maxSteps); MemoryAllocator allocator = parallelSettings.MemoryAllocator; int bufferLength = Unsafe.AsRef(operation).GetRequiredBufferLength(rectangle); @@ -180,7 +180,7 @@ public static void IterateRowIntervals( int width = rectangle.Width; int height = rectangle.Height; - int maxSteps = DivideCeil(width * height, parallelSettings.MinimumPixelsProcessedPerTask); + int maxSteps = DivideCeil(width * (long)height, parallelSettings.MinimumPixelsProcessedPerTask); int numOfSteps = Math.Min(parallelSettings.MaxDegreeOfParallelism, maxSteps); // Avoid TPL overhead in this trivial case: @@ -242,7 +242,7 @@ public static void IterateRowIntervals( int width = rectangle.Width; int height = rectangle.Height; - int maxSteps = DivideCeil(width * height, parallelSettings.MinimumPixelsProcessedPerTask); + int maxSteps = DivideCeil(width * (long)height, parallelSettings.MinimumPixelsProcessedPerTask); int numOfSteps = Math.Min(parallelSettings.MaxDegreeOfParallelism, maxSteps); MemoryAllocator allocator = parallelSettings.MemoryAllocator; int bufferLength = Unsafe.AsRef(operation).GetRequiredBufferLength(rectangle); @@ -270,7 +270,7 @@ public static void IterateRowIntervals( } [MethodImpl(InliningOptions.ShortMethod)] - private static int DivideCeil(int dividend, int divisor) => 1 + ((dividend - 1) / divisor); + private static int DivideCeil(long dividend, int divisor) => (int)Math.Min(1 + ((dividend - 1) / divisor), int.MaxValue); private static void ValidateRectangle(Rectangle rectangle) { diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs index d80679db69..0877dbc922 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs @@ -212,7 +212,12 @@ public bool FindNextMarker() private int ReadStream() { int value = this.badData ? 0 : this.stream.ReadByte(); - if (value == -1) + + // We've encountered the end of the file stream which means there's no EOI marker or the marker has been read + // during decoding of the SOS marker. + // When reading individual bits 'badData' simply means we have hit a marker, When data is '0' and the stream is exhausted + // we know we have hit the EOI and completed decoding the scan buffer. + if (value == -1 || (this.badData && this.data == 0 && this.stream.Position >= this.stream.Length)) { // We've encountered the end of the file stream which means there's no EOI marker // in the image or the SOS marker has the wrong dimensions set. diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs index 80789178d1..e24568ee02 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs @@ -314,4 +314,21 @@ public void Issue2315_DecodeWorks(TestImageProvider provider) image.DebugSave(provider); image.CompareToOriginal(provider); } + + [Theory] + [WithFile(TestImages.Jpeg.Issues.HangBadScan, PixelTypes.L8)] + public void DecodeHang(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + if (TestEnvironment.IsWindows && + TestEnvironment.RunsOnCI) + { + // Windows CI runs consistently fail with OOM. + return; + } + + using Image image = provider.GetImage(JpegDecoder.Instance); + Assert.Equal(65503, image.Width); + Assert.Equal(65503, image.Height); + } } diff --git a/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs b/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs index 1700b4a734..d393850d6b 100644 --- a/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs +++ b/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs @@ -2,6 +2,8 @@ // Licensed under the Six Labors Split License. using System.Numerics; +using System.Runtime.CompilerServices; +using Castle.Core.Configuration; using SixLabors.ImageSharp.Advanced; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.PixelFormats; @@ -406,6 +408,43 @@ void RowAction(RowInterval rows, Span memory) Assert.Contains(width <= 0 ? "Width" : "Height", ex.Message); } + [Fact] + public void CanIterateWithoutIntOverflow() + { + ParallelExecutionSettings parallelSettings = ParallelExecutionSettings.FromConfiguration(Configuration.Default); + const int max = 100_000; + + Rectangle rect = new(0, 0, max, max); + int intervalMaxY = 0; + void RowAction(RowInterval rows, Span memory) => intervalMaxY = Math.Max(rows.Max, intervalMaxY); + + TestRowOperation operation = new(); + TestRowIntervalOperation intervalOperation = new(RowAction); + + ParallelRowIterator.IterateRows(Configuration.Default, rect, in operation); + Assert.Equal(max - 1, operation.MaxY.Value); + + ParallelRowIterator.IterateRowIntervals, Rgba32>(rect, in parallelSettings, in intervalOperation); + Assert.Equal(max, intervalMaxY); + } + + private readonly struct TestRowOperation : IRowOperation + { + public TestRowOperation() + { + } + + public StrongBox MaxY { get; } = new StrongBox(); + + public void Invoke(int y) + { + lock (this.MaxY) + { + this.MaxY.Value = Math.Max(y, this.MaxY.Value); + } + } + } + private readonly struct TestRowIntervalOperation : IRowIntervalOperation { private readonly Action action; diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 589168f009..26d91cbc08 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -284,6 +284,7 @@ public static class Issues public const string Issue2315_NotEnoughBytes = "Jpg/issues/issue-2315.jpg"; public const string Issue2334_NotEnoughBytesA = "Jpg/issues/issue-2334-a.jpg"; public const string Issue2334_NotEnoughBytesB = "Jpg/issues/issue-2334-b.jpg"; + public const string HangBadScan = "Jpg/issues/Hang_C438A851.jpg"; public static class Fuzz { diff --git a/tests/Images/Input/Jpg/issues/Hang_C438A851.jpg b/tests/Images/Input/Jpg/issues/Hang_C438A851.jpg new file mode 100644 index 0000000000..97ab9ad0fb --- /dev/null +++ b/tests/Images/Input/Jpg/issues/Hang_C438A851.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:580760756f2e7e3ed0752a4ec53d6b6786a4f005606f3a50878f732b3b2a1bcb +size 413