Commit ca5fdac0 authored by Georges Racinet's avatar Georges Racinet 🦑
Browse files

Merged upstream v13.10.3

--HG--
branch : heptapod-0-21
......@@ -2,6 +2,15 @@
documentation](doc/development/changelog.md) for instructions on adding your own
entry.
## 13.10.3 (2021-04-13)
### Security (3 changes)
- Check image content type before running exiftool in workhorse.
- Clean only legitimate JPG and TIFF files.
- Update ruby-saml and rexml gems.
## 13.10.2 (2021-04-01)
### Fixed (1 change)
......
13.10.2
\ No newline at end of file
13.10.3
\ No newline at end of file
......@@ -28,6 +28,8 @@ gem 'devise', '~> 4.7.2'
gem 'bcrypt', '~> 3.1', '>= 3.1.14'
gem 'doorkeeper', '~> 5.5.0.rc2'
gem 'doorkeeper-openid_connect', '~> 1.7.5'
gem 'rexml', '~> 3.2.5'
gem 'ruby-saml', '~> 1.12.1'
gem 'omniauth', '~> 1.8'
gem 'omniauth-auth0', '~> 2.0.0'
gem 'omniauth-azure-activedirectory-v2', '~> 0.1'
......
......@@ -1042,7 +1042,7 @@ GEM
retriable (3.1.2)
reverse_markdown (1.4.0)
nokogiri
rexml (3.2.4)
rexml (3.2.5)
rinku (2.0.0)
rotp (2.1.2)
rouge (3.26.0)
......@@ -1116,8 +1116,9 @@ GEM
ruby-magic-static (0.3.4)
ruby-prof (1.3.1)
ruby-progressbar (1.11.0)
ruby-saml (1.7.2)
nokogiri (>= 1.5.10)
ruby-saml (1.12.1)
nokogiri (>= 1.10.5)
rexml
ruby-statistics (2.1.2)
ruby2_keywords (0.0.2)
ruby_parser (3.15.0)
......@@ -1552,6 +1553,7 @@ DEPENDENCIES
request_store (~> 1.5)
responders (~> 3.0)
retriable (~> 3.1.2)
rexml (~> 3.2.5)
rouge (~> 3.26.0)
rqrcode-rails3 (~> 0.1.7)
rspec-parameterized
......@@ -1563,6 +1565,7 @@ DEPENDENCIES
ruby-magic-static (~> 0.3.4)
ruby-prof (~> 1.3.0)
ruby-progressbar (~> 1.10)
ruby-saml (~> 1.12.1)
ruby_parser (~> 3.15)
rubyzip (~> 2.0.0)
rugged (~> 1.1)
......
13.10.2
\ No newline at end of file
13.10.3
\ No newline at end of file
......@@ -45,6 +45,7 @@ class Exif
ALLOWED_TAGS = WHITELISTED_TAGS + IGNORED_TAGS
EXCLUDE_PARAMS = WHITELISTED_TAGS.map { |tag| "-#{tag}" }
ALLOWED_MIME_TYPES = %w(image/jpeg image/tiff).freeze
attr_reader :logger
......@@ -96,12 +97,12 @@ def clean(uploader, dry_run: true)
end
end
private
def extra_tags(path)
exif_tags(path).keys - ALLOWED_TAGS
end
private
def remove_and_store(tmpdir, src_path, uploader)
exec_remove_exif!(src_path)
logger.info "#{upload_ref(uploader.upload)}: exif removed, storing"
......@@ -133,15 +134,26 @@ def fetch_upload_to_file(uploader, dir)
# upload is stored into the file with the original name - this filename
# is used by carrierwave when storing the file back to the storage
filename = File.join(dir, uploader.filename)
contents = uploader.read
check_for_allowed_types(contents)
File.open(filename, 'w') do |file|
file.binmode
file.write uploader.read
file.write contents
end
filename
end
def check_for_allowed_types(contents)
mime_type = Gitlab::Utils::MimeType.from_string(contents)
unless ALLOWED_MIME_TYPES.include?(mime_type)
raise "File type #{mime_type} not supported. Only supports #{ALLOWED_MIME_TYPES.join(", ")}."
end
end
def upload_ref(upload)
"#{upload.id}:#{upload.path}"
end
......
......@@ -4,6 +4,11 @@
RSpec.describe Gitlab::Sanitizers::Exif do
let(:sanitizer) { described_class.new }
let(:mime_type) { 'image/jpeg' }
before do
allow(Gitlab::Utils::MimeType).to receive(:from_string).and_return(mime_type)
end
describe '#batch_clean' do
context 'with image uploads' do
......@@ -43,7 +48,7 @@
end
end
it 'filters only jpg/tiff images' do
it 'filters only jpg/tiff images by filename' do
create(:upload, path: 'filename.jpg')
create(:upload, path: 'filename.jpeg')
create(:upload, path: 'filename.JPG')
......@@ -53,12 +58,16 @@
create(:upload, path: 'filename.txt')
expect(sanitizer).to receive(:clean).exactly(5).times
sanitizer.batch_clean
end
end
describe '#clean' do
let(:uploader) { create(:upload, :with_file, :issuable_upload).retrieve_uploader }
let(:dry_run) { false }
subject { sanitizer.clean(uploader, dry_run: dry_run) }
context "no dry run" do
it "removes exif from the image" do
......@@ -76,7 +85,7 @@
[expected_args, 0]
end
sanitizer.clean(uploader, dry_run: false)
subject
expect(uploader.upload.id).not_to eq(original_upload.id)
expect(uploader.upload.path).to eq(original_upload.path)
......@@ -89,23 +98,35 @@
expect(sanitizer).not_to receive(:exec_remove_exif!)
expect(uploader).not_to receive(:store!)
sanitizer.clean(uploader, dry_run: false)
subject
end
it "raises an error if the exiftool fails with an error" do
expect(Gitlab::Popen).to receive(:popen).and_return(["error", 1])
expect { sanitizer.clean(uploader, dry_run: false) }.to raise_exception(RuntimeError, "failed to get exif tags: error")
expect { subject }.to raise_exception(RuntimeError, "failed to get exif tags: error")
end
context 'for files that do not have the correct MIME type' do
let(:mime_type) { 'text/plain' }
it 'cleans only jpg/tiff images with the correct mime types' do
expect(sanitizer).not_to receive(:extra_tags)
expect { subject }.to raise_error(RuntimeError, /File type text\/plain not supported/)
end
end
end
context "dry run" do
let(:dry_run) { true }
it "doesn't change the image" do
expect(sanitizer).to receive(:extra_tags).and_return({ 'foo' => 'bar' })
expect(sanitizer).not_to receive(:exec_remove_exif!)
expect(uploader).not_to receive(:store!)
sanitizer.clean(uploader, dry_run: true)
subject
end
end
end
......@@ -119,7 +140,7 @@
expect(Gitlab::Popen).to receive(:popen).and_return([tags, 0])
expect(sanitizer.extra_tags('filename')).not_to be_empty
expect(sanitizer.send(:extra_tags, 'filename')).not_to be_empty
end
it "returns an empty list for file with only whitelisted and ignored tags" do
......@@ -130,7 +151,7 @@
expect(Gitlab::Popen).to receive(:popen).and_return([tags, 0])
expect(sanitizer.extra_tags('some file')).to be_empty
expect(sanitizer.send(:extra_tags, 'some file')).to be_empty
end
end
end
......@@ -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
......
......@@ -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
}
......@@ -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))
})
}
}
......
......@@ -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,
......
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))
})
}
}
......@@ -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)