Merge pull request #3119 from SixLabors/js/proper-segment-integrity-handling

Enhance decoders with segment integrity handling

Author: JimBobSquarePants

src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs

@@ -1431,12 +1431,8 @@ private void ReadInfoHeader(BufferedReadStream stream)
             this.infoHeader = BmpInfoHeader.ParseV5(buffer);
             if (this.infoHeader.ProfileData != 0 && this.infoHeader.ProfileSize != 0)
             {
-                // Read color profile.
                 long streamPosition = stream.Position;
-                byte[] iccProfileData = new byte[this.infoHeader.ProfileSize];
-                stream.Position = infoHeaderStart + this.infoHeader.ProfileData;
-                stream.Read(iccProfileData);
-                this.metadata.IccProfile = new IccProfile(iccProfileData);
+                this.ExecuteAncillarySegmentAction(() => this.ReadIccProfile(stream, this.metadata, infoHeaderStart));
                 stream.Position = streamPosition;
             }
         }
@@ -1470,6 +1466,33 @@ private void ReadInfoHeader(BufferedReadStream stream)
         this.Dimensions = new Size(this.infoHeader.Width, this.infoHeader.Height);
     }
 
+    /// <summary>
+    /// Reads the embedded ICC profile from the BMP V5 info header.
+    /// </summary>
+    /// <param name="stream">The <see cref="BufferedReadStream"/> containing image data.</param>
+    /// <param name="imageMetadata">The image metadata.</param>
+    /// <param name="infoHeaderStart">The stream position where the info header begins.</param>
+    private void ReadIccProfile(BufferedReadStream stream, ImageMetadata imageMetadata, long infoHeaderStart)
+    {
+        byte[] iccProfileData = new byte[this.infoHeader.ProfileSize];
+        stream.Position = infoHeaderStart + this.infoHeader.ProfileData;
+
+        if (stream.Read(iccProfileData) != iccProfileData.Length)
+        {
+            BmpThrowHelper.ThrowInvalidImageContentException("Not enough data to read BMP ICC profile.");
+        }
+
+        IccProfile profile = new(iccProfileData);
+        if (profile.CheckIsValid())
+        {
+            imageMetadata.IccProfile = profile;
+        }
+        else
+        {
+            throw new InvalidIccProfileException("Invalid BMP ICC profile.");
+        }
+    }
+
     /// <summary>
     /// Reads the <see cref="BmpFileHeader"/> from the stream.
     /// </summary>

src/ImageSharp/Formats/DecoderOptions.cs

@@ -60,7 +60,7 @@ public sealed class DecoderOptions
     /// <summary>
     /// Gets the segment error handling strategy to use during decoding.
     /// </summary>
-    public SegmentIntegrityHandling SegmentIntegrityHandling { get; init; } = SegmentIntegrityHandling.IgnoreNonCritical;
+    public SegmentIntegrityHandling SegmentIntegrityHandling { get; init; } = SegmentIntegrityHandling.IgnoreAncillary;
 
     /// <summary>
     /// Gets a value that controls how ICC profiles are handled during decode.

src/ImageSharp/Formats/Gif/GifDecoderCore.cs

@@ -146,10 +146,10 @@ protected override Image<TPixel> Decode<TPixel>(BufferedReadStream stream, Cance
                             this.ReadGraphicalControlExtension(stream);
                             break;
                         case GifConstants.CommentLabel:
-                            this.ReadComments(stream);
+                            this.ExecuteAncillarySegmentAction(() => this.ReadComments(stream));
                             break;
                         case GifConstants.ApplicationExtensionLabel:
-                            this.ReadApplicationExtension(stream);
+                            this.ExecuteAncillarySegmentAction(() => this.ReadApplicationExtension(stream));
                             break;
                         case GifConstants.PlainTextLabel:
                             SkipBlock(stream); // Not supported by any known decoder.
@@ -226,10 +226,10 @@ protected override ImageInfo Identify(BufferedReadStream stream, CancellationTok
                             this.ReadGraphicalControlExtension(stream);
                             break;
                         case GifConstants.CommentLabel:
-                            this.ReadComments(stream);
+                            this.ExecuteAncillarySegmentAction(() => this.ReadComments(stream));
                             break;
                         case GifConstants.ApplicationExtensionLabel:
-                            this.ReadApplicationExtension(stream);
+                            this.ExecuteAncillarySegmentAction(() => this.ReadApplicationExtension(stream));
                             break;
                         case GifConstants.PlainTextLabel:
                             SkipBlock(stream); // Not supported by any known decoder.
@@ -266,6 +266,13 @@ protected override ImageInfo Identify(BufferedReadStream stream, CancellationTok
             GifThrowHelper.ThrowNoHeader();
         }
 
+        // Ignoring a malformed ancillary extension must not let identify succeed for a file
+        // that never contained any readable image frame data.
+        if (previousFrame is null)
+        {
+            GifThrowHelper.ThrowNoData();
+        }
+
         return new ImageInfo(
             new Size(this.logicalScreenDescriptor.Width, this.logicalScreenDescriptor.Height),
             this.metadata,
@@ -331,51 +338,128 @@ private void ReadLogicalScreenDescriptor(BufferedReadStream stream)
     private void ReadApplicationExtension(BufferedReadStream stream)
     {
         int appLength = stream.ReadByte();
+        if (appLength == -1)
+        {
+            GifThrowHelper.ThrowInvalidImageContentException("Unexpected end of stream while reading gif application extension");
+        }
+
+        if (appLength != GifConstants.ApplicationBlockSize)
+        {
+            this.ThrowOrIgnoreNonStrictSegmentError($"Gif application extension length '{appLength}' is invalid");
+            SkipBlock(stream, appLength);
+            return;
+        }
 
         // If the length is 11 then it's a valid extension and most likely
         // a NETSCAPE, XMP or ANIMEXTS extension. We want the loop count from this.
         long position = stream.Position;
-        if (appLength == GifConstants.ApplicationBlockSize)
+        int bytesRead = stream.Read(this.buffer.Span, 0, GifConstants.ApplicationBlockSize);
+        if (bytesRead != GifConstants.ApplicationBlockSize)
         {
-            stream.Read(this.buffer.Span, 0, GifConstants.ApplicationBlockSize);
-            bool isXmp = this.buffer.Span.StartsWith(GifConstants.XmpApplicationIdentificationBytes);
-            if (isXmp && !this.skipMetadata)
-            {
-                GifXmpApplicationExtension extension = GifXmpApplicationExtension.Read(stream, this.memoryAllocator);
-                if (extension.Data.Length > 0)
-                {
-                    this.metadata!.XmpProfile = new XmpProfile(extension.Data);
-                }
-                else
-                {
-                    // Reset the stream position and continue.
-                    stream.Position = position;
-                    SkipBlock(stream, appLength);
-                }
+            GifThrowHelper.ThrowInvalidImageContentException("Unexpected end of stream while reading gif application extension");
+        }
 
-                return;
-            }
+        bool isXmp = this.buffer.Span.StartsWith(GifConstants.XmpApplicationIdentificationBytes);
+        if (isXmp)
+        {
+            this.ReadXmpApplicationExtension(stream, position, appLength);
+            return;
+        }
 
-            int subBlockSize = stream.ReadByte();
+        int subBlockSize = stream.ReadByte();
+        if (subBlockSize == -1)
+        {
+            GifThrowHelper.ThrowInvalidImageContentException("Unexpected end of stream while reading gif application extension");
+        }
 
-            // TODO: There's also a NETSCAPE buffer extension.
-            // http://www.vurdalakov.net/misc/gif/netscape-buffering-application-extension
-            if (subBlockSize == GifConstants.NetscapeLoopingSubBlockSize)
-            {
-                stream.Read(this.buffer.Span, 0, GifConstants.NetscapeLoopingSubBlockSize);
-                this.gifMetadata!.RepeatCount = GifNetscapeLoopingApplicationExtension.Parse(this.buffer.Span[1..]).RepeatCount;
-                stream.Skip(1); // Skip the terminator.
-                return;
-            }
+        // TODO: There's also a NETSCAPE buffer extension.
+        // http://www.vurdalakov.net/misc/gif/netscape-buffering-application-extension
+        if (subBlockSize == GifConstants.NetscapeLoopingSubBlockSize)
+        {
+            this.ReadNetscapeApplicationExtension(stream);
+            return;
+        }
+
+        // Could be something else not supported yet.
+        // Skip the subblock and terminator.
+        SkipBlock(stream, subBlockSize);
+    }
+
+    /// <summary>
+    /// Reads the GIF XMP application extension.
+    /// </summary>
+    /// <param name="stream">The <see cref="BufferedReadStream"/> containing image data.</param>
+    /// <param name="applicationPosition">The stream position where the application identifier begins.</param>
+    /// <param name="appLength">The application block length.</param>
+    private void ReadXmpApplicationExtension(BufferedReadStream stream, long applicationPosition, int appLength)
+    {
+        if (this.skipMetadata)
+        {
+            stream.Position = applicationPosition;
+            SkipBlock(stream, appLength);
+            return;
+        }
 
-            // Could be something else not supported yet.
-            // Skip the subblock and terminator.
-            SkipBlock(stream, subBlockSize);
+        bool completed = false;
+        this.ExecuteAncillarySegmentAction(
+            () =>
+        {
+            this.ReadXmpApplicationExtensionData(stream, applicationPosition, appLength);
+            completed = true;
+        });
+
+        if (!completed)
+        {
+            stream.Position = applicationPosition;
+            SkipBlock(stream, appLength);
+        }
+    }
 
+    /// <summary>
+    /// Reads the GIF XMP application extension data.
+    /// </summary>
+    /// <param name="stream">The <see cref="BufferedReadStream"/> containing image data.</param>
+    /// <param name="applicationPosition">The stream position where the application identifier begins.</param>
+    /// <param name="appLength">The application block length.</param>
+    private void ReadXmpApplicationExtensionData(BufferedReadStream stream, long applicationPosition, int appLength)
+    {
+        GifXmpApplicationExtension extension = GifXmpApplicationExtension.Read(stream, this.memoryAllocator);
+        if (extension.Data.Length > 0)
+        {
+            this.metadata!.XmpProfile = new XmpProfile(extension.Data);
             return;
         }
 
-        SkipBlock(stream, appLength); // Not supported by any known decoder.
+        stream.Position = applicationPosition;
+        SkipBlock(stream, appLength);
+    }
+
+    /// <summary>
+    /// Reads the GIF NETSCAPE looping application extension.
+    /// </summary>
+    /// <param name="stream">The <see cref="BufferedReadStream"/> containing image data.</param>
+    private void ReadNetscapeApplicationExtension(BufferedReadStream stream) =>
+        this.ExecuteAncillarySegmentAction(() => this.ReadNetscapeApplicationExtensionData(stream));
+
+    /// <summary>
+    /// Reads the GIF NETSCAPE looping application extension data.
+    /// </summary>
+    /// <param name="stream">The <see cref="BufferedReadStream"/> containing image data.</param>
+    private void ReadNetscapeApplicationExtensionData(BufferedReadStream stream)
+    {
+        int bytesRead = stream.Read(this.buffer.Span, 0, GifConstants.NetscapeLoopingSubBlockSize);
+        if (bytesRead != GifConstants.NetscapeLoopingSubBlockSize)
+        {
+            throw new InvalidImageContentException("Unexpected end of stream while reading gif application extension");
+        }
+
+        this.gifMetadata!.RepeatCount = GifNetscapeLoopingApplicationExtension.Parse(this.buffer.Span[1..]).RepeatCount;
+
+        int terminator = stream.ReadByte();
+        if (terminator == -1)
+        {
+            throw new InvalidImageContentException("Unexpected end of stream while reading gif application extension");
+        }
     }
 
     /// <summary>
@@ -428,7 +512,12 @@ private void ReadComments(BufferedReadStream stream)
             using IMemoryOwner<byte> commentsBuffer = this.memoryAllocator.Allocate<byte>(length);
             Span<byte> commentsSpan = commentsBuffer.GetSpan();
 
-            stream.Read(commentsSpan);
+            int bytesRead = stream.Read(commentsSpan);
+            if (bytesRead != length)
+            {
+                GifThrowHelper.ThrowInvalidImageContentException("Unexpected end of stream while reading gif comment");
+            }
+
             string commentPart = GifConstants.Encoding.GetString(commentsSpan);
             stringBuilder.Append(commentPart);
         }

src/ImageSharp/Formats/Gif/Sections/GifXmpApplicationExtension.cs

@@ -30,7 +30,11 @@ namespace SixLabors.ImageSharp.Formats.Gif;
     /// <returns>The XMP metadata</returns>
     public static GifXmpApplicationExtension Read(Stream stream, MemoryAllocator allocator)
     {
-        byte[] xmpBytes = ReadXmpData(stream, allocator);
+        byte[] xmpBytes = ReadXmpData(stream, allocator, out bool terminated);
+        if (!terminated)
+        {
+            throw new InvalidImageContentException("Unexpected end of stream while reading gif XMP data");
+        }
 
         // Exclude the "magic trailer", see XMP Specification Part 3, 1.1.2 GIF
         int xmpLength = xmpBytes.Length - 256; // 257 - unread 0x0
@@ -71,7 +75,7 @@ public int WriteTo(Span<byte> buffer)
         return this.ContentLength;
     }
 
-    private static byte[] ReadXmpData(Stream stream, MemoryAllocator allocator)
+    private static byte[] ReadXmpData(Stream stream, MemoryAllocator allocator, out bool terminated)
     {
         using ChunkedMemoryStream bytes = new(allocator);
 
@@ -83,8 +87,15 @@ private static byte[] ReadXmpData(Stream stream, MemoryAllocator allocator)
         while (true)
         {
             int b = stream.ReadByte();
-            if (b <= 0)
+            if (b == 0)
+            {
+                terminated = true;
+                return bytes.ToArray();
+            }
+
+            if (b < 0)
             {
+                terminated = false;
                 return bytes.ToArray();
             }
 

src/ImageSharp/Formats/ImageDecoderCore.cs

@@ -33,6 +33,74 @@ protected ImageDecoderCore(DecoderOptions options)
     /// </summary>
     public Size Dimensions { get; protected internal set; }
 
+    /// <summary>
+    /// Executes a known ancillary segment parsing action using the configured integrity policy.
+    /// </summary>
+    /// <param name="action">The action.</param>
+    protected void ExecuteAncillarySegmentAction(Action action)
+    {
+        if (this.Options.SegmentIntegrityHandling is SegmentIntegrityHandling.Strict)
+        {
+            action();
+            return;
+        }
+
+        try
+        {
+            action();
+        }
+        catch (Exception ex) when (ex
+            is ImageFormatException
+            or InvalidIccProfileException
+            or InvalidImageContentException
+            or InvalidOperationException
+            or NotSupportedException)
+        {
+            // Intentionally ignored in non-strict segment integrity modes.
+        }
+    }
+
+    /// <summary>
+    /// Executes a known image data segment parsing action using the configured integrity policy.
+    /// </summary>
+    /// <param name="action">The action.</param>
+    protected void ExecuteImageDataSegmentAction(Action action)
+    {
+        if (this.Options.SegmentIntegrityHandling is not SegmentIntegrityHandling.IgnoreImageData)
+        {
+            action();
+            return;
+        }
+
+        try
+        {
+            action();
+        }
+        catch (Exception ex) when (ex
+            is ImageFormatException
+            or InvalidIccProfileException
+            or InvalidImageContentException
+            or InvalidOperationException
+            or NotSupportedException)
+        {
+            // Intentionally ignored when image data integrity handling is set to IgnoreImageData.
+        }
+    }
+
+    /// <summary>
+    /// Throws unless the decoder is running in a non-strict segment integrity mode.
+    /// Use this only from within <see cref="ExecuteAncillarySegmentAction"/> when local control flow
+    /// must continue after the error.
+    /// </summary>
+    /// <param name="message">The exception message.</param>
+    protected void ThrowOrIgnoreNonStrictSegmentError(string message)
+    {
+        if (this.Options.SegmentIntegrityHandling is SegmentIntegrityHandling.Strict)
+        {
+            throw new InvalidImageContentException(message);
+        }
+    }
+
     /// <summary>
     /// Reads the raw image information from the specified stream.
     /// </summary>