Merge pull request #2523 from SixLabors/js/jpeg-eof-dos

Handle EOF in Jpeg bit reader when data is bad to prevent DOS attack.…

.github/workflows/build-and-test.yml

@@ -9,6 +9,7 @@ on:
   pull_request:
     branches:
       - main
+      - release/*
     types: [ labeled, opened, synchronize, reopened ]
 jobs:
   Build:

src/ImageSharp/Advanced/ParallelRowIterator.cs

@@ -50,7 +50,7 @@ public static void IterateRows<T>(Configuration configuration, Rectangle rectang
         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<T>(Configuration configuration, Rectangle rectang
         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<T>(Configuration configuration, Rectangle
         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<T>(Configuration configuration, Rectangle
         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<T>(Configuration configuration, Rectangle
     }
 
     [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)
     {

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.

tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs

@@ -325,4 +325,21 @@ public void Issue2478_DecodeWorks<TPixel>(TestImageProvider<TPixel> provider)
         image.DebugSave(provider);
         image.CompareToOriginal(provider);
     }
+
+    [Theory]
+    [WithFile(TestImages.Jpeg.Issues.HangBadScan, PixelTypes.L8)]
+    public void DecodeHang<TPixel>(TestImageProvider<TPixel> provider)
+        where TPixel : unmanaged, IPixel<TPixel>
+    {
+        if (TestEnvironment.IsWindows &&
+            TestEnvironment.RunsOnCI)
+        {
+            // Windows CI runs consistently fail with OOM.
+            return;
+        }
+
+        using Image<TPixel> image = provider.GetImage(JpegDecoder.Instance);
+        Assert.Equal(65503, image.Width);
+        Assert.Equal(65503, image.Height);
+    }
 }

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<Rgba32> 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<Rgba32> memory) => intervalMaxY = Math.Max(rows.Max, intervalMaxY);
+
+        TestRowOperation operation = new();
+        TestRowIntervalOperation<Rgba32> intervalOperation = new(RowAction);
+
+        ParallelRowIterator.IterateRows(Configuration.Default, rect, in operation);
+        Assert.Equal(max - 1, operation.MaxY.Value);
+
+        ParallelRowIterator.IterateRowIntervals<TestRowIntervalOperation<Rgba32>, Rgba32>(rect, in parallelSettings, in intervalOperation);
+        Assert.Equal(max, intervalMaxY);
+    }
+
+    private readonly struct TestRowOperation : IRowOperation
+    {
+        public TestRowOperation()
+        {
+        }
+
+        public StrongBox<int> MaxY { get; } = new StrongBox<int>();
+
+        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<RowInterval> action;

tests/ImageSharp.Tests/TestImages.cs

@@ -291,6 +291,7 @@ public static class Issues
             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 Issue2478_JFXX = "Jpg/issues/issue-2478-jfxx.jpg";
+            public const string HangBadScan = "Jpg/issues/Hang_C438A851.jpg";
 
             public static class Fuzz
             {