Skip to content

Add missing decoder sanitation checks #2077

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 16 commits into from
Apr 20, 2022
Merged

Add missing decoder sanitation checks #2077

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d3936e2
Add check for App1 XMP marker length
JimBobSquarePants Mar 27, 2022
96f6fd4
Add check, if enough data is read in LoadTables
brianpopow Mar 27, 2022
588a6d4
Fix lossy image with not enough exif bytes
brianpopow Mar 27, 2022
f820b0b
ParseOptional chunks now checks, if enough data is read
brianpopow Mar 27, 2022
853b222
Add test for lossy with exif which does not contain enough data
brianpopow Mar 27, 2022
5d04734
Add checks in ReadVp8Header() to make sure enough data is read
brianpopow Mar 27, 2022
8cc5a6b
Remove code duplication when reading the profile data
brianpopow Mar 27, 2022
8e4129a
Make sure enough data is read in ReadVp8XHeader()
brianpopow Mar 27, 2022
aac680f
Add check, if enough data was read for progressive scan decoding data
brianpopow Mar 27, 2022
e6ad467
Add check in ReadUint16 for enough data
brianpopow Mar 27, 2022
d22e50c
Ignore invalid EXIF or XMP chunks
brianpopow Mar 28, 2022
ced9887
Add checks, if enough data was read
brianpopow Apr 11, 2022
4a32c75
Merge branch 'main' into js/decode-sanitation
JimBobSquarePants Apr 13, 2022
c5f14f7
Merge branch 'main' into js/decode-sanitation
JimBobSquarePants Apr 15, 2022
2ac18d8
Optimize tiff/jpeg checks
JimBobSquarePants Apr 16, 2022
911d8d7
Merge branch 'main' into js/decode-sanitation
JimBobSquarePants Apr 16, 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
18 changes: 15 additions & 3 deletions src/ImageSharp/Formats/Gif/GifDecoderCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,11 @@ public IImageInfo Identify(BufferedReadStream stream, CancellationToken cancella
/// </summary>
private void ReadGraphicalControlExtension()
{
this.stream.Read(this.buffer, 0, 6);
int bytesRead = this.stream.Read(this.buffer, 0, 6);
if (bytesRead != 6)
{
GifThrowHelper.ThrowInvalidImageContentException("Not enough data to read the graphic control extension");
}

this.graphicsControlExtension = GifGraphicControlExtension.Parse(this.buffer);
}
Expand All @@ -231,7 +235,11 @@ private void ReadGraphicalControlExtension()
/// </summary>
private void ReadImageDescriptor()
{
this.stream.Read(this.buffer, 0, 9);
int bytesRead = this.stream.Read(this.buffer, 0, 9);
if (bytesRead != 9)
{
GifThrowHelper.ThrowInvalidImageContentException("Not enough data to read the image descriptor");
}

this.imageDescriptor = GifImageDescriptor.Parse(this.buffer);
if (this.imageDescriptor.Height == 0 || this.imageDescriptor.Width == 0)
Expand All @@ -245,7 +253,11 @@ private void ReadImageDescriptor()
/// </summary>
private void ReadLogicalScreenDescriptor()
{
this.stream.Read(this.buffer, 0, 7);
int bytesRead = this.stream.Read(this.buffer, 0, 7);
if (bytesRead != 7)
{
GifThrowHelper.ThrowInvalidImageContentException("Not enough data to read the logical screen descriptor");
}

this.logicalScreenDescriptor = GifLogicalScreenDescriptor.Parse(this.buffer);
}
Expand Down
45 changes: 38 additions & 7 deletions src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,15 +232,25 @@ public void LoadTables(byte[] tableBytes, HuffmanScanDecoder huffmanScanDecoder)
using var stream = new BufferedReadStream(this.Configuration, ms);

// Check for the Start Of Image marker.
stream.Read(this.markerBuffer, 0, 2);
int bytesRead = stream.Read(this.markerBuffer, 0, 2);
if (bytesRead != 2)
{
JpegThrowHelper.ThrowInvalidImageContentException("Not enough data to read the SOI marker");
}
Copy link
Contributor

@br3aker br3aker Apr 15, 2022

Choose a reason for hiding this comment

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

I think we can remove this and similar checks from other marker parsing methods as it's now guaranteed that stream has at least remaining bytes available.

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Apr 15, 2022

Choose a reason for hiding this comment

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

Ah... That method is actually called from TIFF only.

Copy link
Contributor

@br3aker br3aker Apr 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

But we still can transform 2 read checks to 1 AvailableBytes check :)
I can't comment on non diff lines but that method creates stream:

using var ms = new MemoryStream(tableBytes);

and then do 2 reads of 2 bytes with sequential if checks.

We can do this check:

if(tableBytes.Length < 4)  throw ...

before even creating a stream and remove 2 checks after reads as those would always be true.

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Apr 15, 2022

Choose a reason for hiding this comment

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

Cool. Will have a look.


var fileMarker = new JpegFileMarker(this.markerBuffer[1], 0);
if (fileMarker.Marker != JpegConstants.Markers.SOI)
{
JpegThrowHelper.ThrowInvalidImageContentException("Missing SOI marker.");
}

// Read next marker.
stream.Read(this.markerBuffer, 0, 2);
bytesRead = stream.Read(this.markerBuffer, 0, 2);
if (bytesRead != 2)
{
JpegThrowHelper.ThrowInvalidImageContentException("Not enough data to read marker");
}

byte marker = this.markerBuffer[1];
fileMarker = new JpegFileMarker(marker, (int)stream.Position - 2);

Expand Down Expand Up @@ -273,7 +283,12 @@ public void LoadTables(byte[] tableBytes, HuffmanScanDecoder huffmanScanDecoder)
}

// Read next marker.
stream.Read(this.markerBuffer, 0, 2);
bytesRead = stream.Read(this.markerBuffer, 0, 2);
if (bytesRead != 2)
{
JpegThrowHelper.ThrowInvalidImageContentException("Not enough data to read marker");
}

fileMarker = new JpegFileMarker(this.markerBuffer[1], 0);
}
}
Expand Down Expand Up @@ -730,7 +745,14 @@ private void ProcessApp1Marker(BufferedReadStream stream, int remaining)

if (ProfileResolver.IsProfile(this.temp, ProfileResolver.XmpMarker.Slice(0, ExifMarkerLength)))
{
int remainingXmpMarkerBytes = XmpMarkerLength - ExifMarkerLength;
const int remainingXmpMarkerBytes = XmpMarkerLength - ExifMarkerLength;
if (remaining < remainingXmpMarkerBytes || this.IgnoreMetadata)
{
// Skip the application header length.
stream.Skip(remaining);
return;
}

stream.Read(this.temp, ExifMarkerLength, remainingXmpMarkerBytes);
remaining -= remainingXmpMarkerBytes;
if (ProfileResolver.IsProfile(this.temp, ProfileResolver.XmpMarker))
Expand Down Expand Up @@ -1320,8 +1342,12 @@ private void ProcessStartOfScanMarker(BufferedReadStream stream, int remaining)
component.ACHuffmanTableId = acTableIndex;
}

// 3 bytes: Progressive scan decoding data
stream.Read(this.temp, 0, 3);
// 3 bytes: Progressive scan decoding data.
int bytesRead = stream.Read(this.temp, 0, 3);
if (bytesRead != 3)
{
JpegThrowHelper.ThrowInvalidImageContentException("Not enough data to read progressive scan decoding data");
}

int spectralStart = this.temp[0];
this.scanDecoder.SpectralStart = spectralStart;
Expand All @@ -1344,7 +1370,12 @@ private void ProcessStartOfScanMarker(BufferedReadStream stream, int remaining)
[MethodImpl(InliningOptions.ShortMethod)]
private ushort ReadUint16(BufferedReadStream stream)
{
stream.Read(this.markerBuffer, 0, 2);
int bytesRead = stream.Read(this.markerBuffer, 0, 2);
if (bytesRead != 2)
{
JpegThrowHelper.ThrowInvalidImageContentException("jpeg stream does not contain enough data, could not read ushort.");
}

return BinaryPrimitives.ReadUInt16BigEndian(this.markerBuffer);
}
}
Expand Down
Loading