/cc @nigeltao

http://www.libpng.org/pub/png/spec/1.2/PNG-Structure.html says:

Ancillary chunks are not strictly necessary in order to meaningfully display the contents of the datastream, for example the time chunk (tIME). A decoder encountering an unknown chunk type in which the ancillary bit is 1 can safely ignore the chunk and proceed to display the image.

I've updated the issue description to reflect that we can ignore ancillary chunks (i.e. those that start with a lowercase letter). The following patch works around the problem:

diff --git a/src/image/png/reader.go b/src/image/png/reader.go
index 910520bd4b..734bf4757b 100644
--- a/src/image/png/reader.go
    b/src/image/png/reader.go
@@ -926,6  926,13 @@ func (d *decoder) parseChunk() error {
 		d.crc.Write(ignored[:n])
 		length -= uint32(n)
 	}
 
 	// If this is an ancillary chunk, we can ignore the checksum since it's not required
 	if d.tmp[4] >> 5 & 0x1 == 1 {
 		d.verifyChecksum()
 		return nil
 	}
 
 	return d.verifyChecksum()
 }

Ancilliary chunks can be ignored, but still, an invalid checksum means that it's an invalid PNG file. And if you're going to ignore some checksums (as per "be liberal in what you accept"), it'd arguably be simpler to just ignore all checksums. And while that's possible (and a supporting argument is that the attached file does seem to render in the Chrome web browser), I'd like to hold the line against "be liberal in what you accept" as much as possible. Part of the problem (with PNG but also with file formats generally) is that different implementations have decided to "be liberal in what you accept" in different, incompatible, undocumented and possibly insecure ways.

FWIW, the spec does not say "ancilliary" in this bullet point (despite discussing ancilliary and critical chunks in sibling bullet points):

A PNG signature mismatch, a CRC mismatch, or an unexpected end-of-stream indicates a corrupted datastream, and may be regarded as a fatal error.

Regarding the "add an option" part, that's easier said than done. It's an API design failure dating back to Go 1.0, but the png.Decode function doesn't have any way to pass options, and we can't change that due to the backwards compatibility promise. We could conceivably add a png.DecodeWithOptions function, but if we're going to do that, we effectively only have one shot to 'fix' the image/png API, for this and all future potential decoding options, ideally with some consistency and re-usability across the other image codec packages like image/gif, image/jpeg, etc. It's an API design problem that's a lot harder than just a six line patch, as your linked issues (#10447 and #27830) show.

For the immediate itch, where this (invalid) PNG file is being rejected, the png.Decode function takes an io.Reader. One workaround is to pass a wrapper io.Reader that just strips out iCCP chunks (which Go's image/png package doesn't do anything with anyway). The PNG file format at the chunk level is pretty straightforward: 8 bytes of magic followed by a sequence of chunks. Each chunk is 4 bytes LENGTH, 4 bytes chunk name, LENGTH bytes payload and 4 bytes checksum. The wrapper io.Reader would just have to detect and remove the N 12 bytes for any chunk named iCCP. Or, instead of just matching iCCP, your wrapper io.Reader could alternatively strip out any ancilliary chunks, or ancilliary chunks with a mismatched checksum.

Part of the problem (with PNG but also with file formats generally) is that different implementations have decided to "be liberal in what you accept" in different, incompatible, undocumented and possibly insecure ways.

Thanks for the feedback. This is a good point.

It's an API design problem that's a lot harder than just a six line patch, as your linked issues (#10447 and #27830) show.

Yeah, I realize that it's an involved issue.

One workaround is to pass a wrapper io.Reader that just strips out iCCP chunks (which Go's image/png package doesn't do anything with anyway).

Thanks. According to our logs, we see a lot of user-uploaded avatars with PNG decoding errors, usually with an invalid iCCP profile with a bad checksum. I'm not quite sure why this is happening in the first place. My colleague implemented such a wrapper in https://gitlab.com/gitlab-org/gitlab-workhorse/-/merge_requests/673/diffs. It's a little tricky to get right.

We did some analysis and found that all corrupted iCCP chunks were all saved by GIMP 2.10.x, which is the latest stable version. We suspect these issues are related:

1. Patch for broken ICC profile in PNG files Exiv2/exiv2#597
2. https://gitlab.gnome.org/GNOME/gimp/-/issues/2111

I have since learned that libpng, by default, treats invalid checksums as an error for critical chunks but a warning for ancillary chunks. In practice, invalid ancillary checksums are ignored.

https://github.com/glennrp/libpng/blob/dbe3e0c43e549a1602286144d94b0666549b18e6/png.h#L1436

I'd be open to Go's image/png therefore simply ignoring ancillary checksums altogether (Go doesn't really do warnings). Obviously such a behavior change would need to wait until we're out of the development freeze (https://github.com/golang/go/wiki/Go-Release-Cycle).

One workaround is to pass a wrapper io.Reader that just strips out iCCP chunks (which Go's image/png package doesn't do anything with anyway).

Here's my implementation:
https://github.com/google/wuffs/blob/414a011491ff513b86d8694c5d71800f3cb5a715/script/strip-png-ancillary-chunks.go

Feel free to copy/paste it.