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

@@ -252,7 +252,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"

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;

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

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
+      # add a "content disposition: attachment" header with the original
-      # for everything but images. audio and video will still stream correctly in
+      # filename for everything but safe images (not SVG). audio and video will
-      # HTML players, and when a direct link is provided to any file but an image
+      # still stream correctly in HTML players, and when a direct link is
-      # it will download correctly in the browser.
+      # provided to any file but an image it will download correctly in the
-      if !FileHelper.is_supported_image?(filename)
+      # browser.
+      if !FileHelper.is_inline_image?(filename)
         options[:content_disposition] = ActionDispatch::Http::ContentDisposition.format(
           disposition: "attachment", filename: filename
         )

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

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