image/png: Add options to tolerate invalid checksums in ancillary chunks

[stanhu]

What version of Go are you using (go version)?

$ go version
go version go1.15.6 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/stanhu/Library/Caches/go-build"
GOENV="/Users/stanhu/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/stanhu/.gvm/pkgsets/go1.15.6/global/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/stanhu/.gvm/pkgsets/go1.15.6/global"
GOPRIVATE=""
GOPROXY="https://goproxy.io,direct"
GOROOT="/Users/stanhu/.gvm/gos/go1.15.6"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/stanhu/.gvm/gos/go1.15.6/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/stanhu/gdk-ee/gitlab-workhorse/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/6v/0wvd8sg951l59wy4242m3thh0000gn/T/go-build329049724=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Decoding the attached image that has valid data checksums but an invalid ICCP checksum fails a decode entirely.

Most image programs ignore the invalid ICC profile, but calling image.Decode fails to load this image:

decode: png: invalid format: invalid checksum

pngcheck confirms there is an invalid checksum in the iCCP chunk:

$ pngcheck -v /tmp/openconnect2.png
File: /tmp/openconnect2.png (67451 bytes)
  chunk IHDR at offset 0x0000c, length 13
    400 x 400 image, 32-bit RGB+alpha, non-interlaced
  chunk zTXt at offset 0x00025, length 13615, keyword: Raw profile type exif
  chunk iCCP at offset 0x03560, length 389
    profile name = ICC PROFILE, compression method = 0 (deflate)
    compressed profile = 376 bytes
  CRC error in chunk iCCP (computed f1449d51, expected c8439de9)
ERRORS DETECTED in /tmp/openconnect2.png

Since the ICC profile isn't absolutely necessary to render a PNG, we ought to be able to detect this error but not fail outright.

This is similar to #10447 and relates to #27830.

What did you expect to see?

The image decoded but the ICC profile skipped.

What did you see instead?

A hard failure.

[agnivade]

/cc @nigeltao

[stanhu]

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

[nigeltao]

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.

[stanhu]

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.

[stanhu]

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

[nigeltao]

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).

[nigeltao]

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.