summaryrefslogtreecommitdiff
path: root/workhorse
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2021-04-13 19:12:50 +0000
committerGitLab Bot <gitlab-bot@gitlab.com>2021-04-13 19:12:50 +0000
commitefe45a03bff39d238a0fda1c273beca0295c48c9 (patch)
tree80b799f58a747c2736fd7c66de162bf0df3315a5 /workhorse
parent09b628c32e2c0bf54347745cf7d165cf987731c0 (diff)
downloadgitlab-ce-efe45a03bff39d238a0fda1c273beca0295c48c9.tar.gz
Add latest changes from gitlab-org/security/gitlab@13-10-stable-ee
Diffstat (limited to 'workhorse')
-rw-r--r--workhorse/go.mod1
-rw-r--r--workhorse/internal/upload/exif/exif.go22
-rw-r--r--workhorse/internal/upload/exif/exif_test.go36
-rw-r--r--workhorse/internal/upload/exif/testdata/sample_exif.tiffbin0 -> 1039916 bytes
-rw-r--r--workhorse/internal/upload/exif/testdata/sample_exif_corrupted.jpgbin0 -> 2182 bytes
-rw-r--r--workhorse/internal/upload/exif/testdata/sample_exif_invalid.jpg1
-rw-r--r--workhorse/internal/upload/rewrite.go72
-rw-r--r--workhorse/internal/upload/rewrite_test.go43
-rw-r--r--workhorse/internal/upload/uploads_test.go77
9 files changed, 201 insertions, 51 deletions
diff --git a/workhorse/go.mod b/workhorse/go.mod
index 6396e419487..ee50ed690aa 100644
--- a/workhorse/go.mod
+++ b/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
diff --git a/workhorse/internal/upload/exif/exif.go b/workhorse/internal/upload/exif/exif.go
index a9307b1ca90..2f8218c3bc3 100644
--- a/workhorse/internal/upload/exif/exif.go
+++ b/workhorse/internal/upload/exif/exif.go
@@ -22,6 +22,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}
@@ -100,8 +108,16 @@ func (c *cleaner) startProcessing(stdin io.Reader) error {
return nil
}
-func IsExifFile(filename string) bool {
- filenameMatch := regexp.MustCompile(`(?i)\.(jpg|jpeg|tiff)$`)
+func FileTypeFromSuffix(filename string) FileType {
+ 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
+ }
- return filenameMatch.MatchString(filename)
+ return TypeUnknown
}
diff --git a/workhorse/internal/upload/exif/exif_test.go b/workhorse/internal/upload/exif/exif_test.go
index 373d97f7fce..ee5883d9e08 100644
--- a/workhorse/internal/upload/exif/exif_test.go
+++ b/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))
})
}
}
diff --git a/workhorse/internal/upload/exif/testdata/sample_exif.tiff b/workhorse/internal/upload/exif/testdata/sample_exif.tiff
new file mode 100644
index 00000000000..6671d818edb
--- /dev/null
+++ b/workhorse/internal/upload/exif/testdata/sample_exif.tiff
Binary files differ
diff --git a/workhorse/internal/upload/exif/testdata/sample_exif_corrupted.jpg b/workhorse/internal/upload/exif/testdata/sample_exif_corrupted.jpg
new file mode 100644
index 00000000000..3b5c692de54
--- /dev/null
+++ b/workhorse/internal/upload/exif/testdata/sample_exif_corrupted.jpg
Binary files differ
diff --git a/workhorse/internal/upload/exif/testdata/sample_exif_invalid.jpg b/workhorse/internal/upload/exif/testdata/sample_exif_invalid.jpg
new file mode 100644
index 00000000000..9f8a284c64f
--- /dev/null
+++ b/workhorse/internal/upload/exif/testdata/sample_exif_invalid.jpg
@@ -0,0 +1 @@
+invalid data
diff --git a/workhorse/internal/upload/rewrite.go b/workhorse/internal/upload/rewrite.go
index e51604c6ed9..ba6bd0e501a 100644
--- a/workhorse/internal/upload/rewrite.go
+++ b/workhorse/internal/upload/rewrite.go
@@ -8,12 +8,15 @@ import (
"io/ioutil"
"mime/multipart"
"net/http"
+ "os"
"strings"
"github.com/prometheus/client_golang/prometheus"
"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"
@@ -122,9 +125,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
}
@@ -164,12 +169,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
}
@@ -177,6 +218,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,
diff --git a/workhorse/internal/upload/rewrite_test.go b/workhorse/internal/upload/rewrite_test.go
new file mode 100644
index 00000000000..6fc41c3fefd
--- /dev/null
+++ b/workhorse/internal/upload/rewrite_test.go
@@ -0,0 +1,43 @@
+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))
+ })
+ }
+}
diff --git a/workhorse/internal/upload/uploads_test.go b/workhorse/internal/upload/uploads_test.go
index fc1a1ac57ef..0885f31d5a4 100644
--- a/workhorse/internal/upload/uploads_test.go
+++ b/workhorse/internal/upload/uploads_test.go
@@ -358,26 +358,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")
- require.NoError(t, err)
+ 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)
- require.NoError(t, err)
+ 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)
@@ -389,30 +391,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)
@@ -420,17 +428,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)
@@ -451,7 +458,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 {