diff options
| -rw-r--r-- | app/uploaders/file_uploader.rb | 2 | ||||
| -rw-r--r-- | app/uploaders/uploader_helper.rb | 9 | ||||
| -rw-r--r-- | changelogs/unreleased/fix-xss-svg.yml | 4 | ||||
| -rw-r--r-- | spec/controllers/uploads_controller_spec.rb | 22 | ||||
| -rw-r--r-- | spec/factories/notes.rb | 6 |
5 files changed, 40 insertions, 3 deletions
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 47bef7cd1e4..23b7318827c 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -36,7 +36,7 @@ class FileUploader < GitlabUploader escaped_filename = filename.gsub("]", "\\]") markdown = "[#{escaped_filename}](#{self.secure_url})" - markdown.prepend("!") if image_or_video? + markdown.prepend("!") if image_or_video? || dangerous? { alt: filename, diff --git a/app/uploaders/uploader_helper.rb b/app/uploaders/uploader_helper.rb index fbaea2744a3..35fd1ed23f8 100644 --- a/app/uploaders/uploader_helper.rb +++ b/app/uploaders/uploader_helper.rb @@ -1,12 +1,15 @@ # Extra methods for uploader module UploaderHelper - IMAGE_EXT = %w[png jpg jpeg gif bmp tiff svg] + IMAGE_EXT = %w[png jpg jpeg gif bmp tiff] # We recommend using the .mp4 format over .mov. Videos in .mov format can # still be used but you really need to make sure they are served with the # proper MIME type video/mp4 and not video/quicktime or your videos won't play # on IE >= 9. # http://archive.sublimevideo.info/20150912/docs.sublimevideo.net/troubleshooting.html VIDEO_EXT = %w[mp4 m4v mov webm ogv] + # These extension types can contain dangerous code and should only be embedded inline with + # proper filtering. They should always be tagged as "Content-Disposition: attachment", not "inline". + DANGEROUS_EXT = %w[svg] def image? extension_match?(IMAGE_EXT) @@ -20,6 +23,10 @@ module UploaderHelper image? || video? end + def dangerous? + extension_match?(DANGEROUS_EXT) + end + def extension_match?(extensions) return false unless file diff --git a/changelogs/unreleased/fix-xss-svg.yml b/changelogs/unreleased/fix-xss-svg.yml new file mode 100644 index 00000000000..dbb956c3910 --- /dev/null +++ b/changelogs/unreleased/fix-xss-svg.yml @@ -0,0 +1,4 @@ +--- +title: Fix XSS vulnerability in SVG attachments +merge_request: +author: diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index 570d9fa43f8..c9584ddf18c 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -4,6 +4,28 @@ describe UploadsController do let!(:user) { create(:user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) } describe "GET show" do + context 'Content-Disposition security measures' do + let(:project) { create(:empty_project, :public) } + + context 'for PNG files' do + it 'returns Content-Disposition: inline' do + note = create(:note, :with_attachment, project: project) + get :show, model: 'note', mounted_as: 'attachment', id: note.id, filename: 'image.png' + + expect(response['Content-Disposition']).to start_with('inline;') + end + end + + context 'for SVG files' do + it 'returns Content-Disposition: attachment' do + note = create(:note, :with_svg_attachment, project: project) + get :show, model: 'note', mounted_as: 'attachment', id: note.id, filename: 'image.svg' + + expect(response['Content-Disposition']).to start_with('attachment;') + end + end + end + context "when viewing a user avatar" do context "when signed in" do before do diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index a21da7074f9..5c50cd7f4ad 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -97,7 +97,11 @@ FactoryGirl.define do end trait :with_attachment do - attachment { fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "`/png") } + attachment { fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png") } + end + + trait :with_svg_attachment do + attachment { fixture_file_upload(Rails.root + "spec/fixtures/unsanitized.svg", "image/svg+xml") } end end end |