Commit dd944bf1 authored by Robert Speicher's avatar Robert Speicher Committed by Robert Speicher

Merge branch 'svg-xss-fix' into 'security'

Fix for XSS vulnerability in SVG attachments

See https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2059
parent 7e1f7a02
...@@ -36,7 +36,7 @@ class FileUploader < GitlabUploader ...@@ -36,7 +36,7 @@ class FileUploader < GitlabUploader
escaped_filename = filename.gsub("]", "\\]") escaped_filename = filename.gsub("]", "\\]")
markdown = "[#{escaped_filename}](#{self.secure_url})" markdown = "[#{escaped_filename}](#{self.secure_url})"
markdown.prepend("!") if image_or_video? markdown.prepend("!") if image_or_video? || dangerous?
{ {
alt: filename, alt: filename,
......
# Extra methods for uploader # Extra methods for uploader
module UploaderHelper 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 # 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 # 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 # proper MIME type video/mp4 and not video/quicktime or your videos won't play
# on IE >= 9. # on IE >= 9.
# http://archive.sublimevideo.info/20150912/docs.sublimevideo.net/troubleshooting.html # http://archive.sublimevideo.info/20150912/docs.sublimevideo.net/troubleshooting.html
VIDEO_EXT = %w[mp4 m4v mov webm ogv] 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? def image?
extension_match?(IMAGE_EXT) extension_match?(IMAGE_EXT)
...@@ -20,6 +23,10 @@ module UploaderHelper ...@@ -20,6 +23,10 @@ module UploaderHelper
image? || video? image? || video?
end end
def dangerous?
extension_match?(DANGEROUS_EXT)
end
def extension_match?(extensions) def extension_match?(extensions)
return false unless file return false unless file
......
---
title: Fix XSS vulnerability in SVG attachments
merge_request:
author:
...@@ -4,6 +4,28 @@ describe UploadsController do ...@@ -4,6 +4,28 @@ describe UploadsController do
let!(:user) { create(:user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) } let!(:user) { create(:user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) }
describe "GET show" do 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 viewing a user avatar" do
context "when signed in" do context "when signed in" do
before do before do
......
...@@ -97,7 +97,11 @@ FactoryGirl.define do ...@@ -97,7 +97,11 @@ FactoryGirl.define do
end end
trait :with_attachment do 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 end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment