Skip to content

PNG: Verify CRC of IDAT chunk is correct #1898

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 4 commits into from
Dec 19, 2021
Merged

PNG: Verify CRC of IDAT chunk is correct #1898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8ea9838
Verify CRC of IDAT chunk is correct
brianpopow Dec 16, 2021
3310fa6
Add test for wrong CRC in IDAT chunk
brianpopow Dec 16, 2021
f3a5ba0
Review suggestion
brianpopow Dec 16, 2021
112585c
Merge branch 'master' into bp/png_idatcrc
brianpopow Dec 18, 2021
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
11 changes: 9 additions & 2 deletions src/ImageSharp/Formats/Png/PngDecoderCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1206,23 +1206,30 @@ private bool TryReadChunk(out PngChunk chunk)

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,
data: this.ReadChunkData(length));

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;
}
Comment on lines +1226 to +1231
Copy link
Member

@antonfirsov antonfirsov Dec 16, 2021

Choose a reason for hiding this comment

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

Why do we need to read the IDAT chunk twice? Upon rewinding the stream, this means creating & filling PngChunk.Data twice. Can't we avoid this?

Copy link
Collaborator Author

@brianpopow brianpopow Dec 17, 2021

Choose a reason for hiding this comment

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

I dont see how we can avoid reading the stream twice. We need to read the stream completely once to be able to calculate the CRC. Then we are sure the byte stream is not corrupted and can start decoding it in ZlibInflateStream. Otherwise we would need to introduce PNG specific logic into ZlibInflateStream, which i dont think would be good.
Also the compressed stream can span over multiple IDAT chunks, which would make it even more complicated.

Note also the PngChunk.Data is not filled twice, just once. Before this PR the PngChunk.Data was left empty.
So we have now with this PR an additional allocation of the size of IDAT chunk.

Copy link
Member

@JimBobSquarePants JimBobSquarePants Dec 18, 2021

Choose a reason for hiding this comment

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

Does libpng check it? I know it’s optional but haven’t had the chance to dig through the code

Copy link
Collaborator Author

@brianpopow brianpopow Dec 18, 2021

Choose a reason for hiding this comment

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

yes, libpng does check it

Copy link
Collaborator Author

@brianpopow brianpopow Dec 18, 2021

Choose a reason for hiding this comment

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

One way to possible avoid a large allocation here for the IDAT chunk just to calculate the CRC could be to read the IDAT data in smaller portions from the stream (lets say 8192 bytes) and calculate the CRC on those smaller chunk's of data.

Is it worth introducing such special handling for calculating the CRC of IDAT?

Copy link
Member

@antonfirsov antonfirsov Dec 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ZlibInflateStream operates over BufferedReadStream, so we can consider making CRC32 calculation as an optional part of BufferedReadStream buffer fills. Although it's definitely not a 2.0 optimization, I would open a separate issue to investigate this.

read the IDAT data in smaller portions from the stream (lets say 8192 bytes) and calculate the CRC on those smaller chunk's of data.

If my understanding is correct, PNG-s with single very large IDAT chunks are rare, so it's probably not worth it, I would better investigate the BufferedReadStream approach, since it also avoids stream rewinding.


return true;
}

Expand Down
32 changes: 23 additions & 9 deletions tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ public void Identify(string imagePath, int expectedPixelSize)
public void Decode_MissingDataChunk_ThrowsException<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
System.Exception ex = Record.Exception(
Exception ex = Record.Exception(
() =>
{
using Image<TPixel> image = provider.GetImage(PngDecoder);
Expand All @@ -271,7 +271,7 @@ public void Decode_MissingDataChunk_ThrowsException<TPixel>(TestImageProvider<TP
public void Decode_InvalidBitDepth_ThrowsException<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
System.Exception ex = Record.Exception(
Exception ex = Record.Exception(
() =>
{
using Image<TPixel> image = provider.GetImage(PngDecoder);
Expand All @@ -287,7 +287,7 @@ public void Decode_InvalidBitDepth_ThrowsException<TPixel>(TestImageProvider<TPi
public void Decode_InvalidColorType_ThrowsException<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
System.Exception ex = Record.Exception(
Exception ex = Record.Exception(
() =>
{
using Image<TPixel> image = provider.GetImage(PngDecoder);
Expand All @@ -297,13 +297,27 @@ public void Decode_InvalidColorType_ThrowsException<TPixel>(TestImageProvider<TP
Assert.Contains("Invalid or unsupported color type", ex.Message);
}

[Theory]
[WithFile(TestImages.Png.Bad.WrongCrcDataChunk, PixelTypes.Rgba32)]
public void Decode_InvalidDataChunkCrc_ThrowsException<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
InvalidImageContentException ex = Assert.Throws<InvalidImageContentException>(
() =>
{
using Image<TPixel> image = provider.GetImage(PngDecoder);
});
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<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
System.Exception ex = Record.Exception(
Exception ex = Record.Exception(
() =>
{
using Image<TPixel> image = provider.GetImage(PngDecoder);
Expand All @@ -319,7 +333,7 @@ public void Issue1014_DataSplitOverMultipleIDatChunks<TPixel>(TestImageProvider<
public void Issue1177_CRC_Omitted<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
System.Exception ex = Record.Exception(
Exception ex = Record.Exception(
() =>
{
using Image<TPixel> image = provider.GetImage(PngDecoder);
Expand All @@ -335,7 +349,7 @@ public void Issue1177_CRC_Omitted<TPixel>(TestImageProvider<TPixel> provider)
public void Issue1127<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
System.Exception ex = Record.Exception(
Exception ex = Record.Exception(
() =>
{
using Image<TPixel> image = provider.GetImage(PngDecoder);
Expand All @@ -351,7 +365,7 @@ public void Issue1127<TPixel>(TestImageProvider<TPixel> provider)
public void Issue1047<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
System.Exception ex = Record.Exception(
Exception ex = Record.Exception(
() =>
{
using Image<TPixel> image = provider.GetImage(PngDecoder);
Expand All @@ -372,7 +386,7 @@ public void Issue1047<TPixel>(TestImageProvider<TPixel> provider)
public void Issue1765<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
System.Exception ex = Record.Exception(
Exception ex = Record.Exception(
() =>
{
using Image<TPixel> image = provider.GetImage(PngDecoder);
Expand All @@ -388,7 +402,7 @@ public void Issue1765<TPixel>(TestImageProvider<TPixel> provider)
public void Issue410_MalformedApplePng<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
System.Exception ex = Record.Exception(
Exception ex = Record.Exception(
() =>
{
using Image<TPixel> image = provider.GetImage(PngDecoder);
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 @@ -120,6 +120,7 @@ public static class Png
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.
Expand Down
3 changes: 3 additions & 0 deletions tests/Images/Input/Png/xcsn0g01.png
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.