Check content type before running exiftool

Assures that exiftool runs for jpeg/tiff images only.

changelogs/unreleased/content-type-check.yml

+---
+title: Check image content type before running exiftool in workhorse
+merge_request:
+author:
+type: security

workhorse/go.mod

@@ -31,6 +31,7 @@ require (
 	gitlab.com/gitlab-org/gitaly v1.74.0
 	gitlab.com/gitlab-org/labkit v1.0.0
 	gocloud.dev v0.21.1-0.20201223184910-5094f54ed8bb
+	golang.org/x/image v0.0.0-20191009234506-e7c1f5e7dbb8
 	golang.org/x/lint v0.0.0-20200302205851-738671d3881b
 	golang.org/x/net v0.0.0-20201224014010-6772e930b67b
 	golang.org/x/sys v0.0.0-20210110051926-789bb1bd4061 // indirect

workhorse/internal/upload/exif/exif.go

@@ -23,6 +23,14 @@ type cleaner struct {
 	eof    bool
 }
 
+type FileType int
+
+const (
+	TypeUnknown FileType = iota
+	TypeJPEG
+	TypeTIFF
+)
+
 func NewCleaner(ctx context.Context, stdin io.Reader) (io.ReadCloser, error) {
 	c := &cleaner{ctx: ctx}
 
@@ -101,11 +109,20 @@ func (c *cleaner) startProcessing(stdin io.Reader) error {
 	return nil
 }
 
-func IsExifFile(filename string) bool {
+func FileTypeFromSuffix(filename string) FileType {
 	if os.Getenv("SKIP_EXIFTOOL") == "1" {
-		return false
+		return TypeUnknown
+	}
+
+	jpegMatch := regexp.MustCompile(`(?i)^[^\n]*\.(jpg|jpeg)$`)
+	if jpegMatch.MatchString(filename) {
+		return TypeJPEG
+	}
+
+	tiffMatch := regexp.MustCompile(`(?i)^[^\n]*\.tiff$`)
+	if tiffMatch.MatchString(filename) {
+		return TypeTIFF
 	}
-	filenameMatch := regexp.MustCompile(`(?i)\.(jpg|jpeg|tiff)$`)
 
-	return filenameMatch.MatchString(filename)
+	return TypeUnknown
 }

workhorse/internal/upload/exif/exif_test.go

@@ -11,39 +11,57 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestIsExifFile(t *testing.T) {
+func TestFileTypeFromSuffix(t *testing.T) {
 	tests := []struct {
 		name     string
-		expected bool
+		expected FileType
 	}{
 		{
 			name:     "/full/path.jpg",
-			expected: true,
+			expected: TypeJPEG,
 		},
 		{
 			name:     "path.jpeg",
-			expected: true,
+			expected: TypeJPEG,
 		},
 		{
 			name:     "path.tiff",
-			expected: true,
+			expected: TypeTIFF,
 		},
 		{
 			name:     "path.JPG",
-			expected: true,
+			expected: TypeJPEG,
 		},
 		{
 			name:     "path.tar",
-			expected: false,
+			expected: TypeUnknown,
 		},
 		{
 			name:     "path",
-			expected: false,
+			expected: TypeUnknown,
+		},
+		{
+			name:     "something.jpg.py",
+			expected: TypeUnknown,
+		},
+		{
+			name:     "something.py.jpg",
+			expected: TypeJPEG,
+		},
+		{
+			name: `something.jpg
+			.py`,
+			expected: TypeUnknown,
+		},
+		{
+			name: `something.something
+			.jpg`,
+			expected: TypeUnknown,
 		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			require.Equal(t, test.expected, IsExifFile(test.name))
+			require.Equal(t, test.expected, FileTypeFromSuffix(test.name))
 		})
 	}
 }

workhorse/internal/upload/exif/testdata/sample_exif_invalid.jpg

+invalid data

workhorse/internal/upload/rewrite.go

@@ -8,6 +8,7 @@ import (
 	"io/ioutil"
 	"mime/multipart"
 	"net/http"
+	"os"
 	"path/filepath"
 	"strings"
 
@@ -15,6 +16,8 @@ import (
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	"gitlab.com/gitlab-org/labkit/log"
 
+	"golang.org/x/image/tiff"
+
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/lsif_transformer/parser"
@@ -127,9 +130,11 @@ func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipa
 
 	var inputReader io.ReadCloser
 	var err error
+
+	imageType := exif.FileTypeFromSuffix(filename)
 	switch {
-	case exif.IsExifFile(filename):
-		inputReader, err = handleExifUpload(ctx, p, filename)
+	case imageType != exif.TypeUnknown:
+		inputReader, err = handleExifUpload(ctx, p, filename, imageType)
 		if err != nil {
 			return err
 		}
@@ -169,12 +174,48 @@ func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipa
 	return rew.filter.ProcessFile(ctx, name, fh, rew.writer)
 }
 
-func handleExifUpload(ctx context.Context, r io.Reader, filename string) (io.ReadCloser, error) {
+func handleExifUpload(ctx context.Context, r io.Reader, filename string, imageType exif.FileType) (io.ReadCloser, error) {
+	tmpfile, err := ioutil.TempFile("", "exifremove")
+	if err != nil {
+		return nil, err
+	}
+	go func() {
+		<-ctx.Done()
+		tmpfile.Close()
+	}()
+	if err := os.Remove(tmpfile.Name()); err != nil {
+		return nil, err
+	}
+
+	_, err = io.Copy(tmpfile, r)
+	if err != nil {
+		return nil, err
+	}
+
+	tmpfile.Seek(0, io.SeekStart)
+	isValidType := false
+	switch imageType {
+	case exif.TypeJPEG:
+		isValidType = isJPEG(tmpfile)
+	case exif.TypeTIFF:
+		isValidType = isTIFF(tmpfile)
+	}
+
+	tmpfile.Seek(0, io.SeekStart)
+	if !isValidType {
+		log.WithContextFields(ctx, log.Fields{
+			"filename":  filename,
+			"imageType": imageType,
+		}).Print("invalid content type, not running exiftool")
+
+		return tmpfile, nil
+	}
+
 	log.WithContextFields(ctx, log.Fields{
 		"filename": filename,
 	}).Print("running exiftool to remove any metadata")
 
-	cleaner, err := exif.NewCleaner(ctx, r)
+	cleaner, err := exif.NewCleaner(ctx, tmpfile)
 	if err != nil {
 		return nil, err
 	}
@@ -182,6 +223,29 @@ func handleExifUpload(ctx context.Context, r io.Reader, filename string) (io.Rea
 	return cleaner, nil
 }
 
+func isTIFF(r io.Reader) bool {
+	_, err := tiff.Decode(r)
+	if err == nil {
+		return true
+	}
+
+	if _, unsupported := err.(tiff.UnsupportedError); unsupported {
+		return true
+	}
+
+	return false
+}
+
+func isJPEG(r io.Reader) bool {
+	// Only the first 512 bytes are used to sniff the content type.
+	buf, err := ioutil.ReadAll(io.LimitReader(r, 512))
+	if err != nil {
+		return false
+	}
+
+	return http.DetectContentType(buf) == "image/jpeg"
+}
+
 func handleLsifUpload(ctx context.Context, reader io.Reader, tempPath, filename string, preauth *api.Response) (io.ReadCloser, error) {
 	parserConfig := parser.Config{
 		TempPath: tempPath,

workhorse/internal/upload/rewrite_test.go

+package upload
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestImageTypeRecongition(t *testing.T) {
+	tests := []struct {
+		filename string
+		isJPEG   bool
+		isTIFF   bool
+	}{
+		{
+			filename: "exif/testdata/sample_exif.jpg",
+			isJPEG:   true,
+			isTIFF:   false,
+		}, {
+			filename: "exif/testdata/sample_exif.tiff",
+			isJPEG:   false,
+			isTIFF:   true,
+		}, {
+			filename: "exif/testdata/sample_exif_corrupted.jpg",
+			isJPEG:   true,
+			isTIFF:   false,
+		}, {
+			filename: "exif/testdata/sample_exif_invalid.jpg",
+			isJPEG:   false,
+			isTIFF:   false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.filename, func(t *testing.T) {
+			input, err := os.Open(test.filename)
+			require.NoError(t, err)
+			require.Equal(t, test.isJPEG, isJPEG(input))
+			require.Equal(t, test.isTIFF, isTIFF(input))
+		})
+	}
+}

workhorse/internal/upload/uploads_test.go

@@ -366,26 +366,28 @@ func TestInvalidFileNames(t *testing.T) {
 }
 
 func TestUploadHandlerRemovingExif(t *testing.T) {
-	tempPath, err := ioutil.TempDir("", "uploads")
-	require.NoError(t, err)
-	defer os.RemoveAll(tempPath)
-
-	var buffer bytes.Buffer
-
 	content, err := ioutil.ReadFile("exif/testdata/sample_exif.jpg")
 	require.NoError(t, err)
 
-	writer := multipart.NewWriter(&buffer)
-	file, err := writer.CreateFormFile("file", "test.jpg")
+	runUploadTest(t, content, "sample_exif.jpg", 200, func(w http.ResponseWriter, r *http.Request) {
+		err := r.ParseMultipartForm(100000)
 		require.NoError(t, err)
 
-	_, err = file.Write(content)
+		size, err := strconv.Atoi(r.FormValue("file.size"))
 		require.NoError(t, err)
+		require.True(t, size < len(content), "Expected the file to be smaller after removal of exif")
+		require.True(t, size > 0, "Expected to receive not empty file")
 
-	err = writer.Close()
+		w.WriteHeader(200)
+		fmt.Fprint(w, "RESPONSE")
+	})
+}
+
+func TestUploadHandlerRemovingExifTiff(t *testing.T) {
+	content, err := ioutil.ReadFile("exif/testdata/sample_exif.tiff")
 	require.NoError(t, err)
 
-	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`/url/path\z`), func(w http.ResponseWriter, r *http.Request) {
+	runUploadTest(t, content, "sample_exif.tiff", 200, func(w http.ResponseWriter, r *http.Request) {
 		err := r.ParseMultipartForm(100000)
 		require.NoError(t, err)
 
@@ -397,30 +399,36 @@ func TestUploadHandlerRemovingExif(t *testing.T) {
 		w.WriteHeader(200)
 		fmt.Fprint(w, "RESPONSE")
 	})
-	defer ts.Close()
+}
 
-	httpRequest, err := http.NewRequest("POST", ts.URL+"/url/path", &buffer)
+func TestUploadHandlerRemovingExifInvalidContentType(t *testing.T) {
+	content, err := ioutil.ReadFile("exif/testdata/sample_exif_invalid.jpg")
 	require.NoError(t, err)
 
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
+	runUploadTest(t, content, "sample_exif_invalid.jpg", 200, func(w http.ResponseWriter, r *http.Request) {
+		err := r.ParseMultipartForm(100000)
+		require.NoError(t, err)
 
-	httpRequest = httpRequest.WithContext(ctx)
-	httpRequest.ContentLength = int64(buffer.Len())
-	httpRequest.Header.Set("Content-Type", writer.FormDataContentType())
-	response := httptest.NewRecorder()
+		output, err := ioutil.ReadFile(r.FormValue("file.path"))
+		require.NoError(t, err)
+		require.Equal(t, content, output, "Expected the file to be same as before")
 
-	handler := newProxy(ts.URL)
-	apiResponse := &api.Response{TempPath: tempPath}
-	preparer := &DefaultPreparer{}
-	opts, _, err := preparer.Prepare(apiResponse)
+		w.WriteHeader(200)
+		fmt.Fprint(w, "RESPONSE")
+	})
+}
+
+func TestUploadHandlerRemovingExifCorruptedFile(t *testing.T) {
+	content, err := ioutil.ReadFile("exif/testdata/sample_exif_corrupted.jpg")
 	require.NoError(t, err)
 
-	HandleFileUploads(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts)
-	require.Equal(t, 200, response.Code)
+	runUploadTest(t, content, "sample_exif_corrupted.jpg", 422, func(w http.ResponseWriter, r *http.Request) {
+		err := r.ParseMultipartForm(100000)
+		require.Error(t, err)
+	})
 }
 
-func TestUploadHandlerRemovingInvalidExif(t *testing.T) {
+func runUploadTest(t *testing.T, image []byte, filename string, httpCode int, tsHandler func(http.ResponseWriter, *http.Request)) {
 	tempPath, err := ioutil.TempDir("", "uploads")
 	require.NoError(t, err)
 	defer os.RemoveAll(tempPath)
@@ -428,17 +436,16 @@ func TestUploadHandlerRemovingInvalidExif(t *testing.T) {
 	var buffer bytes.Buffer
 
 	writer := multipart.NewWriter(&buffer)
-	file, err := writer.CreateFormFile("file", "test.jpg")
+	file, err := writer.CreateFormFile("file", filename)
+	require.NoError(t, err)
+
+	_, err = file.Write(image)
 	require.NoError(t, err)
 
-	fmt.Fprint(file, "this is not valid image data")
 	err = writer.Close()
 	require.NoError(t, err)
 
-	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`/url/path\z`), func(w http.ResponseWriter, r *http.Request) {
-		err := r.ParseMultipartForm(100000)
-		require.Error(t, err)
-	})
+	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`/url/path\z`), tsHandler)
 	defer ts.Close()
 
 	httpRequest, err := http.NewRequest("POST", ts.URL+"/url/path", &buffer)
@@ -459,7 +466,7 @@ func TestUploadHandlerRemovingInvalidExif(t *testing.T) {
 	require.NoError(t, err)
 
 	HandleFileUploads(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts)
-	require.Equal(t, 422, response.Code)
+	require.Equal(t, httpCode, response.Code)
 }
 
 func newProxy(url string) *proxy.Proxy {