From 8ea98385cef8e68317f9249f3e85ee2d6b40f352 Mon Sep 17 00:00:00 2001 From: Brian Popow Date: Thu, 16 Dec 2021 13:13:06 +0100 Subject: [PATCH 1/7] Verify CRC of IDAT chunk is correct --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index ffaa9d567f..f81cbca211 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -1206,16 +1206,16 @@ namespace SixLabors.ImageSharp.Formats.Png PngChunkType type = this.ReadChunkType(); - // NOTE: Reading the Data chunk is the responsible of the caller // If we're reading color metadata only we're only interested in the IHDR and tRNS chunks. // We can skip all other chunk data in the stream for better performance. - if (type == PngChunkType.Data || (this.colorMetadataOnly && type != PngChunkType.Header && type != PngChunkType.Transparency)) + if (this.colorMetadataOnly && type != PngChunkType.Header && type != PngChunkType.Transparency) { chunk = new PngChunk(length, type); return true; } + long pos = this.currentStream.Position; chunk = new PngChunk( length: length, type: type, @@ -1223,6 +1223,13 @@ namespace SixLabors.ImageSharp.Formats.Png this.ValidateChunk(chunk); + // Restore the stream position for IDAT chunks, because it will be decoded later and + // was only read to verifying the CRC is correct. + if (type == PngChunkType.Data) + { + this.currentStream.Position = pos; + } + return true; } From 3310fa6ffdfd283b834c91cdfa76436bdf07332e Mon Sep 17 00:00:00 2001 From: Brian Popow Date: Thu, 16 Dec 2021 13:21:27 +0100 Subject: [PATCH 2/7] Add test for wrong CRC in IDAT chunk --- .../Formats/Png/PngDecoderTests.cs | 33 ++++++++++++++----- tests/ImageSharp.Tests/TestImages.cs | 1 + tests/Images/Input/Png/xcsn0g01.png | 3 ++ 3 files changed, 28 insertions(+), 9 deletions(-) create mode 100644 tests/Images/Input/Png/xcsn0g01.png diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index d63dad8d4b..de1c47d417 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -255,7 +255,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png public void Decode_MissingDataChunk_ThrowsException(TestImageProvider provider) where TPixel : unmanaged, IPixel { - System.Exception ex = Record.Exception( + Exception ex = Record.Exception( () => { using Image image = provider.GetImage(PngDecoder); @@ -271,7 +271,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png public void Decode_InvalidBitDepth_ThrowsException(TestImageProvider provider) where TPixel : unmanaged, IPixel { - System.Exception ex = Record.Exception( + Exception ex = Record.Exception( () => { using Image image = provider.GetImage(PngDecoder); @@ -287,7 +287,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png public void Decode_InvalidColorType_ThrowsException(TestImageProvider provider) where TPixel : unmanaged, IPixel { - System.Exception ex = Record.Exception( + Exception ex = Record.Exception( () => { using Image image = provider.GetImage(PngDecoder); @@ -297,13 +297,28 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png Assert.Contains("Invalid or unsupported color type", ex.Message); } + [Theory] + [WithFile(TestImages.Png.Bad.WrongCrcDataChunk, PixelTypes.Rgba32)] + public void Decode_InvalidDataChunkCrc_ThrowsException(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + Exception ex = Record.Exception( + () => + { + using Image image = provider.GetImage(PngDecoder); + image.DebugSave(provider); + }); + Assert.NotNull(ex); + Assert.Contains("CRC Error. PNG IDAT chunk is corrupt!", ex.Message); + } + // https://github.com/SixLabors/ImageSharp/issues/1014 [Theory] [WithFileCollection(nameof(TestImagesIssue1014), PixelTypes.Rgba32)] public void Issue1014_DataSplitOverMultipleIDatChunks(TestImageProvider provider) where TPixel : unmanaged, IPixel { - System.Exception ex = Record.Exception( + Exception ex = Record.Exception( () => { using Image image = provider.GetImage(PngDecoder); @@ -319,7 +334,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png public void Issue1177_CRC_Omitted(TestImageProvider provider) where TPixel : unmanaged, IPixel { - System.Exception ex = Record.Exception( + Exception ex = Record.Exception( () => { using Image image = provider.GetImage(PngDecoder); @@ -335,7 +350,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png public void Issue1127(TestImageProvider provider) where TPixel : unmanaged, IPixel { - System.Exception ex = Record.Exception( + Exception ex = Record.Exception( () => { using Image image = provider.GetImage(PngDecoder); @@ -351,7 +366,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png public void Issue1047(TestImageProvider provider) where TPixel : unmanaged, IPixel { - System.Exception ex = Record.Exception( + Exception ex = Record.Exception( () => { using Image image = provider.GetImage(PngDecoder); @@ -372,7 +387,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png public void Issue1765(TestImageProvider provider) where TPixel : unmanaged, IPixel { - System.Exception ex = Record.Exception( + Exception ex = Record.Exception( () => { using Image image = provider.GetImage(PngDecoder); @@ -388,7 +403,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png public void Issue410_MalformedApplePng(TestImageProvider provider) where TPixel : unmanaged, IPixel { - System.Exception ex = Record.Exception( + Exception ex = Record.Exception( () => { using Image image = provider.GetImage(PngDecoder); diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index f43c0c9bd6..b87ffc7420 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -120,6 +120,7 @@ namespace SixLabors.ImageSharp.Tests public static class Bad { public const string MissingDataChunk = "Png/xdtn0g01.png"; + public const string WrongCrcDataChunk = "Png/xcsn0g01.png"; public const string CorruptedChunk = "Png/big-corrupted-chunk.png"; // Zlib errors. diff --git a/tests/Images/Input/Png/xcsn0g01.png b/tests/Images/Input/Png/xcsn0g01.png new file mode 100644 index 0000000000..6908a107c4 --- /dev/null +++ b/tests/Images/Input/Png/xcsn0g01.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:71e4b2826f61556eda39f3a93c8769b14d3ac90f135177b9373061199dbef39a +size 164 From f3a5ba0b4d94b2e8422416a733e3d7d272fa0786 Mon Sep 17 00:00:00 2001 From: Brian Popow <38701097+brianpopow@users.noreply.github.com> Date: Thu, 16 Dec 2021 15:45:23 +0100 Subject: [PATCH 3/7] Review suggestion Co-authored-by: Anton Firszov --- tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index de1c47d417..c29f8c5891 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -302,11 +302,10 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png public void Decode_InvalidDataChunkCrc_ThrowsException(TestImageProvider provider) where TPixel : unmanaged, IPixel { - Exception ex = Record.Exception( + InvalidImageContentException ex = Assert.Throws( () => { using Image image = provider.GetImage(PngDecoder); - image.DebugSave(provider); }); Assert.NotNull(ex); Assert.Contains("CRC Error. PNG IDAT chunk is corrupt!", ex.Message); From e045fbb023b561f9b327e434e7aae319772372b6 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 17 Dec 2021 13:53:46 +0300 Subject: [PATCH 4/7] Sampling factor validation --- .../Jpeg/Components/Decoder/JpegComponent.cs | 11 ------- .../Formats/Jpeg/JpegDecoderCore.cs | 29 +++++++++++++++++-- .../Formats/Jpg/JpegDecoderTests.Images.cs | 6 ++-- 3 files changed, 29 insertions(+), 17 deletions(-) diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegComponent.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegComponent.cs index 4da422e7f3..3804e1c6c0 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegComponent.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegComponent.cs @@ -19,21 +19,10 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder this.Frame = frame; this.Id = id; - // Validate sampling factors. - if (horizontalFactor == 0 || verticalFactor == 0) - { - JpegThrowHelper.ThrowBadSampling(); - } - this.HorizontalSamplingFactor = horizontalFactor; this.VerticalSamplingFactor = verticalFactor; this.SamplingFactors = new Size(this.HorizontalSamplingFactor, this.VerticalSamplingFactor); - if (quantizationTableIndex > 3) - { - JpegThrowHelper.ThrowBadQuantizationTableIndex(quantizationTableIndex); - } - this.QuantizationTableIndex = quantizationTableIndex; this.Index = index; } diff --git a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs index 4be6731cc3..bd221a357a 100644 --- a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs +++ b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs @@ -1022,10 +1022,26 @@ namespace SixLabors.ImageSharp.Formats.Jpeg int index = 0; for (int i = 0; i < componentCount; i++) { + // 1 byte: component identifier + byte componentId = this.temp[index]; + + // 1 byte: component sampling factors byte hv = this.temp[index + 1]; int h = (hv >> 4) & 15; int v = hv & 15; + // Validate: 1-4 range + if (h is < 1 or > 4) + { + JpegThrowHelper.ThrowInvalidImageContentException($"Bad horizontal sampling factor: {h}"); + } + + // Validate: 1-4 range + if (v is < 1 or > 4) + { + JpegThrowHelper.ThrowInvalidImageContentException($"Bad vertical sampling factor: {v}"); + } + if (maxH < h) { maxH = h; @@ -1036,10 +1052,19 @@ namespace SixLabors.ImageSharp.Formats.Jpeg maxV = v; } - var component = new JpegComponent(this.Configuration.MemoryAllocator, this.Frame, this.temp[index], h, v, this.temp[index + 2], i); + // 1 byte: quantization table destination selector + byte quantTableIndex = this.temp[index + 2]; + + // Validate: 0-3 range + if (quantTableIndex > 3) + { + JpegThrowHelper.ThrowBadQuantizationTableIndex(quantTableIndex); + } + + var component = new JpegComponent(this.Configuration.MemoryAllocator, this.Frame, componentId, h, v, quantTableIndex, i); this.Frame.Components[i] = component; - this.Frame.ComponentIds[i] = component.Id; + this.Frame.ComponentIds[i] = componentId; index += componentBytes; } diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Images.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Images.cs index ef817154d6..d82359e61a 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Images.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Images.cs @@ -40,9 +40,6 @@ namespace SixLabors.ImageSharp.Tests.Formats.Jpg // LibJpeg can open this despite incorrect colorspace metadata. TestImages.Jpeg.Issues.IncorrectColorspace855, - // LibJpeg can open this despite the invalid subsampling units. - TestImages.Jpeg.Issues.Fuzz.IndexOutOfRangeException824C, - // High depth images TestImages.Jpeg.Baseline.Testorig12bit, }; @@ -90,7 +87,8 @@ namespace SixLabors.ImageSharp.Tests.Formats.Jpg TestImages.Jpeg.Issues.Fuzz.AccessViolationException827, TestImages.Jpeg.Issues.Fuzz.ExecutionEngineException839, TestImages.Jpeg.Issues.Fuzz.IndexOutOfRangeException1693A, - TestImages.Jpeg.Issues.Fuzz.IndexOutOfRangeException1693B + TestImages.Jpeg.Issues.Fuzz.IndexOutOfRangeException1693B, + TestImages.Jpeg.Issues.Fuzz.IndexOutOfRangeException824C, }; private static readonly Dictionary CustomToleranceValues = From 1f9ef3e926372325ebc8d342729f03cb782e880a Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Sat, 18 Dec 2021 11:13:39 +0300 Subject: [PATCH 5/7] Removed unused methods, added new throw helper method --- .../Formats/Jpeg/JpegDecoderCore.cs | 4 ++-- .../Formats/Jpeg/JpegThrowHelper.cs | 20 +++---------------- 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs index bd221a357a..4543153def 100644 --- a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs +++ b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs @@ -1033,13 +1033,13 @@ namespace SixLabors.ImageSharp.Formats.Jpeg // Validate: 1-4 range if (h is < 1 or > 4) { - JpegThrowHelper.ThrowInvalidImageContentException($"Bad horizontal sampling factor: {h}"); + JpegThrowHelper.ThrowBadSampling(h); } // Validate: 1-4 range if (v is < 1 or > 4) { - JpegThrowHelper.ThrowInvalidImageContentException($"Bad vertical sampling factor: {v}"); + JpegThrowHelper.ThrowBadSampling(v); } if (maxH < h) diff --git a/src/ImageSharp/Formats/Jpeg/JpegThrowHelper.cs b/src/ImageSharp/Formats/Jpeg/JpegThrowHelper.cs index b6c7e76268..0292fbcab4 100644 --- a/src/ImageSharp/Formats/Jpeg/JpegThrowHelper.cs +++ b/src/ImageSharp/Formats/Jpeg/JpegThrowHelper.cs @@ -22,23 +22,6 @@ namespace SixLabors.ImageSharp.Formats.Jpeg [MethodImpl(InliningOptions.ColdPath)] public static void ThrowInvalidImageContentException(string errorMessage) => throw new InvalidImageContentException(errorMessage); - /// - /// Cold path optimization for throwing 's. - /// - /// The error message for the exception. - /// The exception that is the cause of the current exception, or a null reference - /// if no inner exception is specified. - [MethodImpl(InliningOptions.ColdPath)] - public static void ThrowInvalidImageContentException(string errorMessage, Exception innerException) => throw new InvalidImageContentException(errorMessage, innerException); - - /// - /// Cold path optimization for throwing 's - /// - /// The error message for the exception. - [MethodImpl(InliningOptions.ColdPath)] - public static void ThrowNotImplementedException(string errorMessage) - => throw new NotImplementedException(errorMessage); - [MethodImpl(InliningOptions.ColdPath)] public static void ThrowBadMarker(string marker, int length) => throw new InvalidImageContentException($"Marker {marker} has bad length {length}."); @@ -51,6 +34,9 @@ namespace SixLabors.ImageSharp.Formats.Jpeg [MethodImpl(InliningOptions.ColdPath)] public static void ThrowBadSampling() => throw new InvalidImageContentException("Bad sampling factor."); + [MethodImpl(InliningOptions.ColdPath)] + public static void ThrowBadSampling(int factor) => throw new InvalidImageContentException($"Bad sampling factor: {factor}"); + [MethodImpl(InliningOptions.ColdPath)] public static void ThrowBadProgressiveScan(int ss, int se, int ah, int al) => throw new InvalidImageContentException($"Invalid progressive parameters Ss={ss} Se={se} Ah={ah} Al={al}."); From f3b8e5cd8d1e00492367b40498de1a67f6da2375 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Sat, 18 Dec 2021 15:42:13 +0300 Subject: [PATCH 6/7] gfoidl IsOutOfRange --- src/ImageSharp/Common/Helpers/Numerics.cs | 9 +++++++++ .../Formats/Jpeg/JpegDecoderCore.cs | 4 ++-- .../ImageSharp.Tests/Common/NumericsTests.cs | 20 +++++++++++++++++++ 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/ImageSharp/Common/Helpers/Numerics.cs b/src/ImageSharp/Common/Helpers/Numerics.cs index 513db0c448..8f82476e13 100644 --- a/src/ImageSharp/Common/Helpers/Numerics.cs +++ b/src/ImageSharp/Common/Helpers/Numerics.cs @@ -963,5 +963,14 @@ namespace SixLabors.ImageSharp public static uint RotateRightSoftwareFallback(uint value, int offset) => (value >> offset) | (value << (32 - offset)); #endif + + /// + /// Tells whether input value is outside of the given range. + /// + /// Value. + /// Mininum value, inclusive. + /// Maximum value, inclusive. + public static bool IsOutOfRange(int value, int min, int max) + => (uint)(value - min) > (uint)(max - min); } } diff --git a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs index 4543153def..4b68c39ea9 100644 --- a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs +++ b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs @@ -1031,13 +1031,13 @@ namespace SixLabors.ImageSharp.Formats.Jpeg int v = hv & 15; // Validate: 1-4 range - if (h is < 1 or > 4) + if (Numerics.IsOutOfRange(h, 1, 4)) { JpegThrowHelper.ThrowBadSampling(h); } // Validate: 1-4 range - if (v is < 1 or > 4) + if (Numerics.IsOutOfRange(v, 1, 4)) { JpegThrowHelper.ThrowBadSampling(v); } diff --git a/tests/ImageSharp.Tests/Common/NumericsTests.cs b/tests/ImageSharp.Tests/Common/NumericsTests.cs index 62819af493..2a43b06a92 100644 --- a/tests/ImageSharp.Tests/Common/NumericsTests.cs +++ b/tests/ImageSharp.Tests/Common/NumericsTests.cs @@ -97,5 +97,25 @@ namespace SixLabors.ImageSharp.Tests.Common Assert.True(expected == actual, $"Expected: {expected}\nActual: {actual}\n{value} / {divisor} = {expected}"); } } + + private static bool IsOutOfRange_ReferenceImplementation(int value, int min, int max) => value < min || value > max; + + [Theory] + [InlineData(1, 100)] + public void IsOutOfRange(int seed, int count) + { + var rng = new Random(seed); + for (int i = 0; i < count; i++) + { + int value = rng.Next(); + int min = rng.Next(); + int max = rng.Next(min, int.MaxValue); + + bool expected = IsOutOfRange_ReferenceImplementation(value, min, max); + bool actual = Numerics.IsOutOfRange(value, min, max); + + Assert.True(expected == actual, $"IsOutOfRange({value}, {min}, {max})"); + } + } } } From 7ced49747e84744c93b6dca6138257e0d9395a11 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Sat, 18 Dec 2021 23:41:55 +0300 Subject: [PATCH 7/7] Fixed tests --- .../ImageSharp.Tests/Common/NumericsTests.cs | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/tests/ImageSharp.Tests/Common/NumericsTests.cs b/tests/ImageSharp.Tests/Common/NumericsTests.cs index 2a43b06a92..a829974778 100644 --- a/tests/ImageSharp.Tests/Common/NumericsTests.cs +++ b/tests/ImageSharp.Tests/Common/NumericsTests.cs @@ -16,6 +16,8 @@ namespace SixLabors.ImageSharp.Tests.Common this.Output = output; } + public static TheoryData IsOutOfRangeTestData = new() { int.MinValue, -1, 0, 1, 6, 7, 8, 91, 92, 93, int.MaxValue }; + private static int Log2_ReferenceImplementation(uint value) { int n = 0; @@ -101,21 +103,16 @@ namespace SixLabors.ImageSharp.Tests.Common private static bool IsOutOfRange_ReferenceImplementation(int value, int min, int max) => value < min || value > max; [Theory] - [InlineData(1, 100)] - public void IsOutOfRange(int seed, int count) + [MemberData(nameof(IsOutOfRangeTestData))] + public void IsOutOfRange(int value) { - var rng = new Random(seed); - for (int i = 0; i < count; i++) - { - int value = rng.Next(); - int min = rng.Next(); - int max = rng.Next(min, int.MaxValue); + const int min = 7; + const int max = 92; - bool expected = IsOutOfRange_ReferenceImplementation(value, min, max); - bool actual = Numerics.IsOutOfRange(value, min, max); + bool expected = IsOutOfRange_ReferenceImplementation(value, min, max); + bool actual = Numerics.IsOutOfRange(value, min, max); - Assert.True(expected == actual, $"IsOutOfRange({value}, {min}, {max})"); - } + Assert.True(expected == actual, $"IsOutOfRange({value}, {min}, {max})"); } } }