Add missing decoder sanitation checks

[JimBobSquarePants]

Prerequisites

• I have written a descriptive pull-request title
• I have verified that there are no overlapping pull-requests open
• I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
• I have provided test coverage for my change (where applicable)

Description

A PR to add additional sanitation checks with a view to fix #2075

Any additional sanitation fixes added to other open branches should be added here.

[JimBobSquarePants]

Are there rules in WebP about where a these chunks should be. I.E are they always before the image data?

[brianpopow]

ICCP and ANIM chunks should always come before the image data, EXIF and XMP chunks should come after the image data.

https://developers.google.com/speed/webp/docs/riff_container#extended_file_format

[JimBobSquarePants]

Since EXIF and XMP chunks come after, I don't think we should throw for them and skip instead. I would consider those non-critical.

[br3aker]

I would suggest another sanitation check for jpeg decoder in general. Each marker has specific predefined size which we already have in each marker parser as a parameter - remaining. Checking whether stream has enough bytes via stream.Length - stream.Position >= remaining would eliminate any stream read checks inside marker parsers and would make code cleaner imo.

I can't contribute to this branch directly so I can make this as a separate PR.

[JimBobSquarePants]

Checking whether stream has enough bytes via stream.Length - stream.Position >= remaining would eliminate any stream read checks inside marker parsers and would make code cleaner imo.

I would agree. If you have the time to do that'd very much appreciate it.

[br3aker]

Sorry for late response, been studying scaled IDCT for other PR. Will try to do a PR for jpeg markers on this week.

[JimBobSquarePants]

I looks increasingly unlikely that we'll get any test images for this but I think we should still push on with the sanitation code.
I'm going to create a release/2.1.1 branch and open a second PR from this branch so we can push on with V3.

[br3aker]

Actually there's kind of a problem in current jpeg decoder. It shouldn't be a cause of mentioned issue but it's still a bug

Current decoding pipeline:

1. Gather jpeg info (components, tables, frame size, etc.) <- pixel buffer is allocated here
2. For each scan:
       For each mcu row:
           Decode: huffman -> spectral -> rgb -> TPixel
3. Create Image object using allocated pixel buffer

Any exceptions between step 1 and 3 would cause allocated pixel buffer to be gc'd leading to a false positive memory leak reported by memory diagnostics. Any invalid marker between scans can cause this, it shouldn't crash the entire process or even lead to an actual memory leak but it's still a thing.

I'll create a separate issue this week, posting this here just to let you know.

[br3aker]

I would suggest another sanitation check for jpeg decoder in general. Each marker has specific predefined size which we already have in each marker parser as a parameter - remaining. Checking whether stream has enough bytes via stream.Length - stream.Position >= remaining would eliminate any stream read checks inside marker parsers and would make code cleaner imo.

Extra jpeg sanity checks implemented at #2084.

[JimBobSquarePants]

@SixLabors/core I'm marking this as ready to review. I think with these checks and #2084 we've probably handled any potential issues during decode that trigger #2075

[br3aker]

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.

[JimBobSquarePants]

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

[br3aker]

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.

[JimBobSquarePants]

Cool. Will have a look.

[br3aker]

Sorry, I forgot to alter TIFF jpeg loading method here:

ImageSharp/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

Lines 257 to 264 in c5f14f7

	while (fileMarker.Marker != JpegConstants.Markers.EOI || (fileMarker.Marker == JpegConstants.Markers.EOI && fileMarker.Invalid))
	{
	if (!fileMarker.Invalid)
	{
	// Get the marker length.
	int remaining = this.ReadUint16(stream) - 2;
	switch (fileMarker.Marker)

It should be this:

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);
}

swtich(fileMarker.Marker)
{
    // ...
}

[JimBobSquarePants]

Peeps I'm gonna merge this. I want a build that can be tested in #2075 and it's blocking all other progress.