Skip to content

Added sanity check for every jpeg marker #2084

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 14, 2022
Merged

Added sanity check for every jpeg marker #2084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8cdcda3
Added sanity check for every jpeg marker
Apr 13, 2022
64d9146
Added malformed image
Apr 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 24 additions & 16 deletions src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

@br3aker br3aker Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this error message would contain hex value of the marker. While we can map byte to ITU spec name I don't think it's worth the effort.

}

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:
Expand Down Expand Up @@ -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
Expand All @@ -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:
Expand All @@ -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:
Expand Down Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions src/ImageSharp/Formats/Jpeg/JpegThrowHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}.");

Expand Down
9 changes: 9 additions & 0 deletions src/ImageSharp/IO/BufferedReadStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
1 change: 1 addition & 0 deletions tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Images.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
1 change: 1 addition & 0 deletions tests/ImageSharp.Tests/TestImages.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
}
}

Expand Down
3 changes: 3 additions & 0 deletions tests/Images/Input/Jpg/issues/fuzz/Issue2085-NullReferenceException.jpg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.