Merge pull request #2084 from br3aker/dp/jpeg-marker-validation

Added sanity check for every jpeg marker
SixLabors · Apr 14, 2022 · 5d0c684 · 5d0c684
2 parents 7db4792 + 64d9146
commit 5d0c684
Show file tree

Hide file tree

Showing 6 changed files with 41 additions and 16 deletions.
diff --git a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs
@@ -315,14 +315,22 @@ internal void ParseStream(BufferedReadStream stream, HuffmanScanDecoder scanDeco
                 if (!fileMarker.Invalid)
                 {
                     // Get the marker length.
-                    int remaining = this.ReadUint16(stream) - 2;
+                    int markerContentByteSize = this.ReadUint16(stream) - 2;
+
+                    // Check whether stream actually has enought bytes to read
+                    // markerContentByteSize is always positive so we cast
+                    // to uint to avoid sign extension
+                    if (stream.RemainingBytes < (uint)markerContentByteSize)
+                    {
+                        JpegThrowHelper.ThrowNotEnoughBytesForMarker(fileMarker.Marker);
+                    }
 
                     switch (fileMarker.Marker)
                     {
                         case JpegConstants.Markers.SOF0:
                         case JpegConstants.Markers.SOF1:
                         case JpegConstants.Markers.SOF2:
-                            this.ProcessStartOfFrameMarker(stream, remaining, fileMarker, metadataOnly);
+                            this.ProcessStartOfFrameMarker(stream, markerContentByteSize, fileMarker, metadataOnly);
                             break;
 
                         case JpegConstants.Markers.SOF5:
@@ -350,7 +358,7 @@ internal void ParseStream(BufferedReadStream stream, HuffmanScanDecoder scanDeco
                         case JpegConstants.Markers.SOS:
                             if (!metadataOnly)
                             {
-                                this.ProcessStartOfScanMarker(stream, remaining);
+                                this.ProcessStartOfScanMarker(stream, markerContentByteSize);
                                 break;
                             }
                             else
@@ -364,41 +372,41 @@ internal void ParseStream(BufferedReadStream stream, HuffmanScanDecoder scanDeco
 
                             if (metadataOnly)
                             {
-                                stream.Skip(remaining);
+                                stream.Skip(markerContentByteSize);
                             }
                             else
                             {
-                                this.ProcessDefineHuffmanTablesMarker(stream, remaining);
+                                this.ProcessDefineHuffmanTablesMarker(stream, markerContentByteSize);
                             }
 
                             break;
 
                         case JpegConstants.Markers.DQT:
-                            this.ProcessDefineQuantizationTablesMarker(stream, remaining);
+                            this.ProcessDefineQuantizationTablesMarker(stream, markerContentByteSize);
                             break;
 
                         case JpegConstants.Markers.DRI:
                             if (metadataOnly)
                             {
-                                stream.Skip(remaining);
+                                stream.Skip(markerContentByteSize);
                             }
                             else
                             {
-                                this.ProcessDefineRestartIntervalMarker(stream, remaining);
+                                this.ProcessDefineRestartIntervalMarker(stream, markerContentByteSize);
                             }
 
                             break;
 
                         case JpegConstants.Markers.APP0:
-                            this.ProcessApplicationHeaderMarker(stream, remaining);
+                            this.ProcessApplicationHeaderMarker(stream, markerContentByteSize);
                             break;
 
                         case JpegConstants.Markers.APP1:
-                            this.ProcessApp1Marker(stream, remaining);
+                            this.ProcessApp1Marker(stream, markerContentByteSize);
                             break;
 
                         case JpegConstants.Markers.APP2:
-                            this.ProcessApp2Marker(stream, remaining);
+                            this.ProcessApp2Marker(stream, markerContentByteSize);
                             break;
 
                         case JpegConstants.Markers.APP3:
@@ -411,20 +419,20 @@ internal void ParseStream(BufferedReadStream stream, HuffmanScanDecoder scanDeco
                         case JpegConstants.Markers.APP10:
                         case JpegConstants.Markers.APP11:
                         case JpegConstants.Markers.APP12:
-                            stream.Skip(remaining);
+                            stream.Skip(markerContentByteSize);
                             break;
 
                         case JpegConstants.Markers.APP13:
-                            this.ProcessApp13Marker(stream, remaining);
+                            this.ProcessApp13Marker(stream, markerContentByteSize);
                             break;
 
                         case JpegConstants.Markers.APP14:
-                            this.ProcessApp14Marker(stream, remaining);
+                            this.ProcessApp14Marker(stream, markerContentByteSize);
                             break;
 
                         case JpegConstants.Markers.APP15:
                         case JpegConstants.Markers.COM:
-                            stream.Skip(remaining);
+                            stream.Skip(markerContentByteSize);
                             break;
 
                         case JpegConstants.Markers.DAC:
@@ -1260,7 +1268,7 @@ private void ProcessStartOfScanMarker(BufferedReadStream stream, int remaining)
             int selectorsBytes = selectorsCount * 2;
             if (remaining != 4 + selectorsBytes)
             {
-                JpegThrowHelper.ThrowBadMarker("SOS", remaining);
+                JpegThrowHelper.ThrowBadMarker(nameof(JpegConstants.Markers.SOS), remaining);
             }
 
             // selectorsCount*2 bytes: component index + huffman tables indices

diff --git a/src/ImageSharp/Formats/Jpeg/JpegThrowHelper.cs b/src/ImageSharp/Formats/Jpeg/JpegThrowHelper.cs
@@ -25,6 +25,9 @@ internal static class JpegThrowHelper
         [MethodImpl(InliningOptions.ColdPath)]
         public static void ThrowBadMarker(string marker, int length) => throw new InvalidImageContentException($"Marker {marker} has bad length {length}.");
 
+        [MethodImpl(InliningOptions.ColdPath)]
+        public static void ThrowNotEnoughBytesForMarker(byte marker) => throw new InvalidImageContentException($"Input stream does not have enough bytes to parse declared contents of the {marker:X2} marker.");
+
         [MethodImpl(InliningOptions.ColdPath)]
         public static void ThrowBadQuantizationTableIndex(int index) => throw new InvalidImageContentException($"Bad Quantization Table index {index}.");
 

diff --git a/src/ImageSharp/IO/BufferedReadStream.cs b/src/ImageSharp/IO/BufferedReadStream.cs
@@ -114,6 +114,15 @@ public override long Position
         /// <inheritdoc/>
         public override bool CanWrite { get; } = false;
 
+        /// <summary>
+        /// Gets remaining byte count available to read.
+        /// </summary>
+        public long RemainingBytes
+        {
+            [MethodImpl(MethodImplOptions.AggressiveInlining)]
+            get => this.Length - this.Position;
+        }
+
         /// <summary>
         /// Gets the underlying stream.
         /// </summary>

diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Images.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Images.cs
@@ -105,6 +105,7 @@ public partial class JpegDecoderTests
             TestImages.Jpeg.Issues.Fuzz.IndexOutOfRangeException1693A,
             TestImages.Jpeg.Issues.Fuzz.IndexOutOfRangeException1693B,
             TestImages.Jpeg.Issues.Fuzz.IndexOutOfRangeException824C,
+            TestImages.Jpeg.Issues.Fuzz.NullReferenceException2085,
         };
 
         private static readonly Dictionary<string, float> CustomToleranceValues = new()

diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs
@@ -292,6 +292,7 @@ public static class Fuzz
                     public const string AccessViolationException922 = "Jpg/issues/fuzz/Issue922-AccessViolationException.jpg";
                     public const string IndexOutOfRangeException1693A = "Jpg/issues/fuzz/Issue1693-IndexOutOfRangeException-A.jpg";
                     public const string IndexOutOfRangeException1693B = "Jpg/issues/fuzz/Issue1693-IndexOutOfRangeException-B.jpg";
+                    public const string NullReferenceException2085 = "Jpg/issues/fuzz/Issue2085-NullReferenceException.jpg";
                 }
             }
 

diff --git a/tests/Images/Input/Jpg/issues/fuzz/Issue2085-NullReferenceException.jpg b/tests/Images/Input/Jpg/issues/fuzz/Issue2085-NullReferenceException.jpg