Merge pull request #2524 from SixLabors/js/backport-fix-jpeg-dos

Backport - Handle EOF in Jpeg bit reader when data is bad to prevent DOS attack.
SixLabors · Aug 30, 2023 · 688e242 · 688e242
2 parents d1b52a2 + 0f17a8b
commit 688e242
Show file tree

Hide file tree

Showing 6 changed files with 68 additions and 6 deletions.
diff --git a/src/ImageSharp/Advanced/ParallelRowIterator.cs b/src/ImageSharp/Advanced/ParallelRowIterator.cs
@@ -52,7 +52,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:
@@ -117,7 +117,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;
 
@@ -181,7 +181,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:
@@ -243,7 +243,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;
 
@@ -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)
         {

diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanBuffer.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanBuffer.cs
@@ -211,7 +211,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
@@ -261,5 +261,22 @@ public void Issue2133_DeduceColorSpace<TPixel>(TestImageProvider<TPixel> provide
                 this.Output.WriteLine($"Difference for PORT: {portReport.DifferencePercentageString}");
             }
         }
+
+        [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);
+            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
@@ -4,6 +4,7 @@
 using System;
 using System.Linq;
 using System.Numerics;
+using System.Runtime.CompilerServices;
 using System.Threading;
 using SixLabors.ImageSharp.Advanced;
 using SixLabors.ImageSharp.Memory;
@@ -410,6 +411,41 @@ 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(0);
+            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(int _) => this.MaxY = new StrongBox<int>();
+
+            public StrongBox<int> MaxY { get; }
+
+            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;

diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs
@@ -266,6 +266,7 @@ public static class Issues
                 public const string ExifNullArrayTag = "Jpg/issues/issue-2056-exif-null-array.jpg";
                 public const string ValidExifArgumentNullExceptionOnEncode = "Jpg/issues/Issue2087-exif-null-reference-on-encode.jpg";
                 public const string Issue2133DeduceColorSpace = "Jpg/issues/Issue2133.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