From dd944bf14f4a0fd555db32d5833325fa459d9565 Mon Sep 17 00:00:00 2001
From: Robert Speicher <robert@gitlab.com>
Date: Mon, 13 Feb 2017 22:42:46 +0000
Subject: [PATCH] Merge branch 'svg-xss-fix' into 'security'

Fix for XSS vulnerability in SVG attachments

See https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2059
---
 app/uploaders/file_uploader.rb              |  2 +-
 app/uploaders/uploader_helper.rb            |  9 ++++++++-
 changelogs/unreleased/fix-xss-svg.yml       |  4 ++++
 spec/controllers/uploads_controller_spec.rb | 22 +++++++++++++++++++++
 spec/factories/notes.rb                     |  6 +++++-
 5 files changed, 40 insertions(+), 3 deletions(-)
 create mode 100644 changelogs/unreleased/fix-xss-svg.yml

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