summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/uploaders/file_uploader.rb2
-rw-r--r--app/uploaders/uploader_helper.rb9
-rw-r--r--changelogs/unreleased/fix-xss-svg.yml4
-rw-r--r--spec/controllers/uploads_controller_spec.rb22
-rw-r--r--spec/factories/notes.rb6
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