Merge branch '341-skip-iccp-chunk' into 'master'

PNG decoder proxy that skips iCCP chunks

Closes #341

See merge request gitlab-org/gitlab-workhorse!673

changelogs/unreleased/341-skip-iccp-chunk.yml

+---
+title: 'Image scaling: strip out iCCP chunks in PNG files'
+merge_request: 673
+author:
+type: fixed

cmd/gitlab-resize-image/main.go

@@ -7,6 +7,8 @@ import (
 	"strconv"
 
 	"github.com/disintegration/imaging"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/cmd/gitlab-resize-image/png"
 )
 
 func main() {
@@ -23,7 +25,12 @@ func _main() error {
 		return fmt.Errorf("GL_RESIZE_IMAGE_WIDTH: %w", err)
 	}
 
-	src, formatName, err := image.Decode(os.Stdin)
+	pngReader, err := png.NewReader(os.Stdin)
+	if err != nil {
+		return fmt.Errorf("construct PNG reader: %w", err)
+	}
+
+	src, formatName, err := image.Decode(pngReader)
 	if err != nil {
 		return fmt.Errorf("decode: %w", err)
 	}

cmd/gitlab-resize-image/png/reader.go

+package png
+
+import (
+	"bytes"
+	"encoding/binary"
+	"fmt"
+	"io"
+	"io/ioutil"
+	"os"
+)
+
+const (
+	pngMagicLen = 8
+	pngMagic    = "\x89PNG\r\n\x1a\n"
+)
+
+// Reader is an io.Reader decorator that skips certain PNG chunks known to cause problems.
+// If the image stream is not a PNG, it will yield all bytes unchanged to the underlying
+// reader.
+// See also https://gitlab.com/gitlab-org/gitlab/-/issues/287614
+type Reader struct {
+	underlying     io.Reader
+	chunk          io.Reader
+	bytesRemaining int64
+}
+
+func NewReader(r io.Reader) (io.Reader, error) {
+	magicBytes, err := readMagic(r)
+	if err != nil {
+		return nil, err
+	}
+
+	if string(magicBytes) != pngMagic {
+		debug("Not a PNG - read file unchanged")
+		return io.MultiReader(bytes.NewReader(magicBytes), r), nil
+	}
+
+	return io.MultiReader(bytes.NewReader(magicBytes), &Reader{underlying: r}), nil
+}
+
+func (r *Reader) Read(p []byte) (int, error) {
+	for r.bytesRemaining == 0 {
+		const (
+			headerLen = 8
+			crcLen    = 4
+		)
+		var header [headerLen]byte
+		_, err := io.ReadFull(r.underlying, header[:])
+		if err != nil {
+			return 0, err
+		}
+
+		chunkLen := int64(binary.BigEndian.Uint32(header[:4]))
+		if chunkType := string(header[4:]); chunkType == "iCCP" {
+			debug("!! iCCP chunk found; skipping")
+			if _, err := io.CopyN(ioutil.Discard, r.underlying, chunkLen+crcLen); err != nil {
+				return 0, err
+			}
+			continue
+		}
+
+		r.bytesRemaining = headerLen + chunkLen + crcLen
+		r.chunk = io.MultiReader(bytes.NewReader(header[:]), io.LimitReader(r.underlying, r.bytesRemaining-headerLen))
+	}
+
+	n, err := r.chunk.Read(p)
+	r.bytesRemaining -= int64(n)
+	return n, err
+}
+
+func debug(args ...interface{}) {
+	if os.Getenv("DEBUG") == "1" {
+		fmt.Fprintln(os.Stderr, args...)
+	}
+}
+
+// Consume PNG magic and proceed to reading the IHDR chunk.
+func readMagic(r io.Reader) ([]byte, error) {
+	var magicBytes []byte = make([]byte, pngMagicLen)
+	_, err := io.ReadFull(r, magicBytes)
+	if err != nil {
+		return nil, err
+	}
+
+	return magicBytes, nil
+}

cmd/gitlab-resize-image/png/reader_test.go

+package png
+
+import (
+	"bytes"
+	"hash/crc64"
+	"image"
+	"io"
+	"io/ioutil"
+	"os"
+	"testing"
+
+	_ "image/jpeg" // registers JPEG format for image.Decode
+	"image/png"    // registers PNG format for image.Decode
+
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	goodPNG     = "../../../testdata/image.png"
+	badPNG      = "../../../testdata/image_bad_iccp.png"
+	strippedPNG = "../../../testdata/image_stripped_iccp.png"
+	jpg         = "../../../testdata/image.jpg"
+)
+
+func TestReadImageUnchanged(t *testing.T) {
+	testCases := []struct {
+		desc      string
+		imagePath string
+		imageType string
+	}{
+		{
+			desc:      "image is not a PNG",
+			imagePath: jpg,
+			imageType: "jpeg",
+		},
+		{
+			desc:      "image is PNG without iCCP chunk",
+			imagePath: goodPNG,
+			imageType: "png",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			requireValidImage(t, pngReader(t, tc.imagePath), tc.imageType)
+			requireStreamUnchanged(t, pngReader(t, tc.imagePath), rawImageReader(t, tc.imagePath))
+		})
+	}
+}
+
+func TestReadPNGWithBadICCPChunkDecodesAndReEncodesSuccessfully(t *testing.T) {
+	badPNGBytes, fmt, err := image.Decode(pngReader(t, badPNG))
+	require.NoError(t, err)
+	require.Equal(t, "png", fmt)
+
+	strippedPNGBytes, fmt, err := image.Decode(pngReader(t, strippedPNG))
+	require.NoError(t, err)
+	require.Equal(t, "png", fmt)
+
+	buf1 := new(bytes.Buffer)
+	buf2 := new(bytes.Buffer)
+
+	require.NoError(t, png.Encode(buf1, badPNGBytes))
+	require.NoError(t, png.Encode(buf2, strippedPNGBytes))
+
+	requireStreamUnchanged(t, buf1, buf2)
+}
+
+func pngReader(t *testing.T, path string) io.Reader {
+	r, err := NewReader(rawImageReader(t, path))
+	require.NoError(t, err)
+	return r
+}
+
+func rawImageReader(t *testing.T, path string) io.Reader {
+	f, err := os.Open(path)
+	require.NoError(t, err)
+	return f
+}
+
+func requireValidImage(t *testing.T, r io.Reader, expected string) {
+	_, fmt, err := image.Decode(r)
+	require.NoError(t, err)
+	require.Equal(t, expected, fmt)
+}
+
+func requireStreamUnchanged(t *testing.T, actual io.Reader, expected io.Reader) {
+	actualBytes, err := ioutil.ReadAll(actual)
+	require.NoError(t, err)
+	expectedBytes, err := ioutil.ReadAll(expected)
+	require.NoError(t, err)
+
+	table := crc64.MakeTable(crc64.ISO)
+	sumActual := crc64.Checksum(actualBytes, table)
+	sumExpected := crc64.Checksum(expectedBytes, table)
+	require.Equal(t, sumExpected, sumActual)
+}