From df14926e42dd6223c3ee73ff24e61140b83a50bc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Tue, 3 May 2016 21:54:07 +0200
Subject: [PATCH] SECURITY: check magic bytes before using ImageMagick tools

---
 Gemfile                    |  2 +-
 Gemfile.lock               |  4 ++--
 app/models/upload.rb       | 10 +++++++++-
 spec/models/upload_spec.rb |  2 +-
 4 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/Gemfile b/Gemfile
index 2c7d1298578..d5d1c061791 100644
--- a/Gemfile
+++ b/Gemfile
@@ -61,7 +61,7 @@ gem 'fast_xs'
 gem 'fast_xor'
 
 # while we sort out https://github.com/sdsykes/fastimage/pull/46
-gem 'fastimage_discourse', require: 'fastimage'
+gem 'discourse_fastimage', require: 'fastimage'
 gem 'aws-sdk', require: false
 gem 'excon', require: false
 gem 'unf', require: false
diff --git a/Gemfile.lock b/Gemfile.lock
index 0f8687054db..be5294173a9 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -73,6 +73,7 @@ GEM
     diff-lcs (1.2.5)
     discourse-qunit-rails (0.0.9)
       railties
+    discourse_fastimage (2.0.0)
     docile (1.1.5)
     domain_name (0.5.25)
       unf (>= 0.0.5, < 1.0.0)
@@ -107,7 +108,6 @@ GEM
       rake
       rake-compiler
     fast_xs (0.8.0)
-    fastimage_discourse (1.6.6)
     ffi (1.9.10)
     flamegraph (0.1.0)
       fast_stack
@@ -413,6 +413,7 @@ DEPENDENCIES
   byebug
   certified
   discourse-qunit-rails
+  discourse_fastimage
   email_reply_trimmer (= 0.1.3)
   ember-rails (= 0.18.5)
   ember-source (= 1.12.2)
@@ -422,7 +423,6 @@ DEPENDENCIES
   fast_blank
   fast_xor
   fast_xs
-  fastimage_discourse
   flamegraph
   foreman
   gctools
diff --git a/app/models/upload.rb b/app/models/upload.rb
index 8d2c0ffeb98..5783a94d2d8 100644
--- a/app/models/upload.rb
+++ b/app/models/upload.rb
@@ -70,7 +70,7 @@ class Upload < ActiveRecord::Base
   def self.create_for(user_id, file, filename, filesize, options = {})
     DistributedMutex.synchronize("upload_#{user_id}_#{filename}") do
       # do some work on images
-      if FileHelper.is_image?(filename) && system("identify '#{file.path}' >/dev/null 2>&1")
+      if FileHelper.is_image?(filename) && is_actual_image?(file)
         if filename =~ /\.svg$/i
           svg = Nokogiri::XML(file).at_css("svg")
           w = svg["width"].to_i
@@ -170,6 +170,14 @@ class Upload < ActiveRecord::Base
     end
   end
 
+  def self.is_actual_image?(file)
+    # due to ImageMagick CVE-2016–3714, use FastImage to check the magic bytes
+    # cf. https://meta.discourse.org/t/imagemagick-cve-2016-3714/43624
+    FastImage.size(file, raise_on_failure: true)
+  rescue
+    false
+  end
+
   LARGE_PNG_SIZE ||= 3.megabytes
 
   def self.should_optimize?(path)
diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb
index deddf2b795c..b7ff3ab8ded 100644
--- a/spec/models/upload_spec.rb
+++ b/spec/models/upload_spec.rb
@@ -63,7 +63,7 @@ describe Upload do
 
     it "computes width & height for images" do
       ImageSizer.expects(:resize)
-      image.expects(:rewind).twice
+      image.expects(:rewind).times(3)
       Upload.create_for(user_id, image, image_filename, image_filesize)
     end