From 4bf3bf678630338289bc8bc3a78aacdaf15560e5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Wed, 25 Jul 2018 22:00:04 +0200
Subject: [PATCH] SECURITY: force IM decoder based on file extension

---
 app/models/optimized_image.rb       | 17 ++++++++++++
 lib/upload_creator.rb               | 42 +++++++++++++++++++----------
 spec/models/optimized_image_spec.rb |  2 +-
 3 files changed, 46 insertions(+), 15 deletions(-)

diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb
index 576cfff508b..501efb401e1 100644
--- a/app/models/optimized_image.rb
+++ b/app/models/optimized_image.rb
@@ -118,6 +118,14 @@ class OptimizedImage < ActiveRecord::Base
     end
   end
 
+  IM_DECODERS ||= /\A(jpe?g|png|tiff?|bmp|ico)\z/i
+
+  def self.prepend_decoder!(path)
+    extension = File.extname(path)[1..-1]
+    raise Discourse::InvalidAccess unless extension[IM_DECODERS]
+    "#{extension}:#{path}"
+  end
+
   def self.thumbnail_or_resize
     SiteSetting.strip_image_metadata ? "thumbnail" : "resize"
   end
@@ -125,6 +133,9 @@ class OptimizedImage < ActiveRecord::Base
   def self.resize_instructions(from, to, dimensions, opts = {})
     ensure_safe_paths!(from, to)
 
+    prepend_decoder!(from)
+    prepend_decoder!(to)
+
     # NOTE: ORDER is important!
     %W{
       convert
@@ -159,6 +170,9 @@ class OptimizedImage < ActiveRecord::Base
   def self.crop_instructions(from, to, dimensions, opts = {})
     ensure_safe_paths!(from, to)
 
+    prepend_decoder!(from)
+    prepend_decoder!(to)
+
     %W{
       convert
       #{from}[0]
@@ -191,6 +205,9 @@ class OptimizedImage < ActiveRecord::Base
   def self.downsize_instructions(from, to, dimensions, opts = {})
     ensure_safe_paths!(from, to)
 
+    prepend_decoder!(from)
+    prepend_decoder!(to)
+
     %W{
       convert
       #{from}[0]
diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb
index 85f8ca242d7..207859dc515 100644
--- a/lib/upload_creator.rb
+++ b/lib/upload_creator.rb
@@ -135,13 +135,19 @@ class UploadCreator
   def convert_to_jpeg!
     jpeg_tempfile = Tempfile.new(["image", ".jpg"])
 
-    OptimizedImage.ensure_safe_paths!(@file.path, jpeg_tempfile.path)
+    from = @file.path
+    to = jpeg_tempfile.path
+
+    OptimizedImage.ensure_safe_paths!(from, to)
+
+    OptimizedImage.prepend_decoder!(from)
+    OptimizedImage.prepend_decoder!(to)
 
     begin
-      execute_convert(@file, jpeg_tempfile)
+      execute_convert(from, to)
     rescue
       # retry with debugging enabled
-      execute_convert(@file, jpeg_tempfile, true)
+      execute_convert(from, to, true)
     end
 
     # keep the JPEG if it's at least 15% smaller
@@ -155,15 +161,18 @@ class UploadCreator
     end
   end
 
-  def execute_convert(input_file, output_file, debug = false)
-    command = ['convert', input_file.path,
-               '-auto-orient',
-               '-background', 'white',
-               '-interlace', 'none',
-               '-flatten',
-               '-quality', SiteSetting.png_to_jpg_quality.to_s]
-    command << '-debug' << 'all' if debug
-    command << output_file.path
+  def execute_convert(from, to, debug = false)
+    command = [
+      "convert",
+      from,
+      "-auto-orient",
+      "-background white",
+      "-interlace none",
+      "-flatten",
+      "-quality #{SiteSetting.png_to_jpg_quality}"
+    ]
+    command << "-debug all" if debug
+    command << to
 
     Discourse::Utils.execute_command(*command, failure_message: I18n.t("upload.png_to_jpg_conversion_failure_message"))
   end
@@ -208,8 +217,13 @@ class UploadCreator
   end
 
   def fix_orientation!
-    OptimizedImage.ensure_safe_paths!(@file.path)
-    Discourse::Utils.execute_command('convert', @file.path, '-auto-orient', @file.path)
+    path = @file.path
+
+    OptimizedImage.ensure_safe_paths!(path)
+    OptimizedImage.prepend_decoder!(path)
+
+    Discourse::Utils.execute_command('convert', path, '-auto-orient', path)
+
     extract_image_info!
   end
 
diff --git a/spec/models/optimized_image_spec.rb b/spec/models/optimized_image_spec.rb
index 17c2fc20db2..23268160927 100644
--- a/spec/models/optimized_image_spec.rb
+++ b/spec/models/optimized_image_spec.rb
@@ -93,7 +93,7 @@ describe OptimizedImage do
       }.not_to raise_error
     end
 
-    it "raises nothing on paths" do
+    it "raises InvalidAccess error on paths" do
       expect {
         OptimizedImage.ensure_safe_paths!("/a.png", "/b.png", "c.png")
       }.to raise_error(Discourse::InvalidAccess)