From 3f7658cc6e85d3df1e3ac135f1a1d82a285583a4 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Thu, 9 Jul 2020 13:31:48 +1000
Subject: [PATCH] SECURITY: Add content-disposition: attachment for SVG uploads

* strip out the href and xlink:href attributes from use element that
  are _not_ anchors in svgs which can be used for XSS
* adding the content-disposition: attachment ensures that
  uploaded SVGs cannot be opened and executed using the XSS exploit.
  svgs embedded using an img tag do not suffer from the same exploit
---
 app/controllers/uploads_controller.rb |  2 +-
 config/nginx.sample.conf              |  5 ++++-
 lib/file_helper.rb                    | 13 +++++++++++++
 lib/file_store/s3_store.rb            | 11 ++++++-----
 lib/file_store/to_s3_migration.rb     |  5 +++++
 lib/upload_creator.rb                 |  8 ++++++++
 6 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
index 64fcd795b58..e8d1a152db2 100644
--- a/app/controllers/uploads_controller.rb
+++ b/app/controllers/uploads_controller.rb
@@ -249,7 +249,7 @@ class UploadsController < ApplicationController
       content_type: MiniMime.lookup_by_filename(upload.original_filename)&.content_type
     }
 
-    if !FileHelper.is_supported_image?(upload.original_filename)
+    if !FileHelper.is_inline_image?(upload.original_filename)
       opts[:disposition] = "attachment"
     elsif params[:inline]
       opts[:disposition] = "inline"
diff --git a/config/nginx.sample.conf b/config/nginx.sample.conf
index 310e8c84e2d..18e16a20e25 100644
--- a/config/nginx.sample.conf
+++ b/config/nginx.sample.conf
@@ -185,9 +185,12 @@ server {
           try_files $uri =404;
       }
       # this allows us to bypass rails
-      location ~* \.(gif|png|jpg|jpeg|bmp|tif|tiff|svg|ico|webp)$ {
+      location ~* \.(gif|png|jpg|jpeg|bmp|tif|tiff|ico|webp)$ {
           try_files $uri =404;
       }
+      # SVG needs an extra header attached
+      location ~* \.(svg)$ {
+      }
       # thumbnails & optimized images
       location ~ /_?optimized/ {
           try_files $uri =404;
diff --git a/lib/file_helper.rb b/lib/file_helper.rb
index cbf11eebbea..162de9a40b5 100644
--- a/lib/file_helper.rb
+++ b/lib/file_helper.rb
@@ -17,6 +17,10 @@ class FileHelper
     (filename =~ supported_images_regexp).present?
   end
 
+  def self.is_inline_image?(filename)
+    (filename =~ inline_images_regexp).present?
+  end
+
   def self.is_supported_media?(filename)
     (filename =~ supported_media_regexp).present?
   end
@@ -136,6 +140,11 @@ class FileHelper
     @@supported_images ||= Set.new %w{jpg jpeg png gif svg ico webp}
   end
 
+  def self.inline_images
+    # SVG cannot safely be shown as a document
+    @@inline_images ||= supported_images - %w{svg}
+  end
+
   def self.supported_audio
     @@supported_audio ||= Set.new %w{mp3 ogg wav m4a}
   end
@@ -148,6 +157,10 @@ class FileHelper
     @@supported_images_regexp ||= /\.(#{supported_images.to_a.join("|")})$/i
   end
 
+  def self.inline_images_regexp
+    @@inline_images_regexp ||= /\.(#{inline_images.to_a.join("|")})$/i
+  end
+
   def self.supported_media_regexp
     media = supported_images | supported_audio | supported_video
     @@supported_media_regexp ||= /\.(#{media.to_a.join("|")})$/i
diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb
index 6dd62140c7f..873bafc710e 100644
--- a/lib/file_store/s3_store.rb
+++ b/lib/file_store/s3_store.rb
@@ -54,11 +54,12 @@ module FileStore
         content_type: opts[:content_type].presence || MiniMime.lookup_by_filename(filename)&.content_type
       }
 
-      # add a "content disposition: attachment" header with the original filename
-      # for everything but images. audio and video will still stream correctly in
-      # HTML players, and when a direct link is provided to any file but an image
-      # it will download correctly in the browser.
-      if !FileHelper.is_supported_image?(filename)
+      # add a "content disposition: attachment" header with the original
+      # filename for everything but safe images (not SVG). audio and video will
+      # still stream correctly in HTML players, and when a direct link is
+      # provided to any file but an image it will download correctly in the
+      # browser.
+      if !FileHelper.is_inline_image?(filename)
         options[:content_disposition] = ActionDispatch::Http::ContentDisposition.format(
           disposition: "attachment", filename: filename
         )
diff --git a/lib/file_store/to_s3_migration.rb b/lib/file_store/to_s3_migration.rb
index 02353759287..85af5a58082 100644
--- a/lib/file_store/to_s3_migration.rb
+++ b/lib/file_store/to_s3_migration.rb
@@ -232,6 +232,11 @@ module FileStore
           if upload&.secure
             options[:acl] = "private"
           end
+        elsif !FileHelper.is_inline_image?(name)
+          upload = Upload.find_by(url: "/#{file}")
+          options[:content_disposition] = ActionDispatch::Http::ContentDisposition.format(
+            disposition: "attachment", filename: upload&.original_filename || name
+          )
         end
 
         etag ||= Digest::MD5.file(path).hexdigest
diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb
index 797328f19c6..7716af2f318 100644
--- a/lib/upload_creator.rb
+++ b/lib/upload_creator.rb
@@ -278,6 +278,14 @@ class UploadCreator
     doc = Nokogiri::XML(@file)
     doc.xpath(svg_whitelist_xpath).remove
     doc.xpath("//@*[starts-with(name(), 'on')]").remove
+    doc.css('use').each do |use_el|
+      if use_el.attr('href')
+        use_el.remove_attribute('href') unless use_el.attr('href').starts_with?('#')
+      end
+      if use_el.attr('xlink:href')
+        use_el.remove_attribute('xlink:href') unless use_el.attr('xlink:href').starts_with?('#')
+      end
+    end
     File.write(@file.path, doc.to_s)
     @file.rewind
   end