Merge remote-tracking branch 'upstream/pr/1934'

CHANGELOG.md

@@ -6,6 +6,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 ## [Unreleased]
 ### Changed
 * Replace mime-types dependency with mini_mime(@bradleypriest [#2292](https://github.com/carrierwaveuploader/carrierwave/pull/2292))
+* Use the MimeMagic gem to inspect file headers for the mime type. This allows for mitigation of CVE-2016-3714, in combination with a `content_type_whitelist`. (@locriani [#1934](https://github.com/carrierwaveuploader/carrierwave/pull/1934))
 
 ## 1.3.1 - 2018-12-29
 ### Fixed

README.md

@@ -255,6 +255,22 @@ class NoJsonUploader < CarrierWave::Uploader::Base
 end
 ```
 
+### CVE-2016-3714 (ImageTragick)
+This version of CarrierWave has the ability to mitigate CVE-2016-3714. However, you **MUST** set a content_type_whitelist in your uploaders for this protection to be effective, and you **MUST** either disable ImageMagick's default SVG delegate or use the RSVG delegate for SVG processing.
+
+
+A valid whitelist that will restrict your uploader to images only, and mitigate the CVE is:
+
+```ruby
+class MyUploader < CarrierWave::Uploader::Base
+  def content_type_whitelist
+    [/image\//]
+  end
+end
+```
+
+**WARNING**: A `content_type_whitelist` is the only form of whitelist or blacklist supported by CarrierWave that can effectively mitigate against CVE-2016-3714. Use of `extension_whitelist` will not inspect the file headers, and thus still leaves your application open to the vulnerability.
+
 ### Filenames and unicode chars
 
 Another security issue you should care for is the file names (see

carrierwave.gemspec

@@ -25,6 +25,7 @@ Gem::Specification.new do |s|
   s.add_dependency "activemodel", ">= 5.0.0"
   s.add_dependency "mini_mime", ">= 0.1.3"
   s.add_dependency "image_processing", "~> 1.1"
+  s.add_dependency "mimemagic", ">= 0.3.0"
   if RUBY_ENGINE == 'jruby'
     s.add_development_dependency 'activerecord-jdbcpostgresql-adapter'
   else

lib/carrierwave/sanitized_file.rb

@@ -1,6 +1,7 @@
 require 'pathname'
 require 'active_support/core_ext/string/multibyte'
 require 'mini_mime'
+require 'mimemagic'
 
 module CarrierWave
 
@@ -256,13 +257,10 @@ def to_file
     # [String] the content type of the file
     #
     def content_type
-      return @content_type if @content_type
-      if @file.respond_to?(:content_type) and @file.content_type
-        @content_type = @file.content_type.to_s.chomp
-      elsif path
-        mime_type = ::MiniMime.lookup_by_filename(path)
-        @content_type = (mime_type && mime_type.content_type).to_s
-      end
+      @content_type ||=
+        existing_content_type ||
+        mime_magic_content_type ||
+        mini_mime_content_type
     end
 
     ##
@@ -336,6 +334,24 @@ def try_uri(candidate)
     rescue URI::InvalidURIError
     end
 
+    def existing_content_type
+      if @file.respond_to?(:content_type) && @file.content_type
+        @file.content_type.to_s.chomp
+      end
+    end
+
+    def mime_magic_content_type
+      MimeMagic.by_magic(File.open(path)).try(:type) if path
+    rescue Errno::ENOENT
+      nil
+    end
+
+    def mini_mime_content_type
+      return unless path
+      mime_type = ::MiniMime.lookup_by_filename(path)
+      @content_type = (mime_type && mime_type.content_type).to_s
+    end
+
     def split_extension(filename)
       # regular expressions to try for identifying extensions
       extension_matchers = [

spec/sanitized_file_spec.rb

@@ -243,6 +243,21 @@
 
       expect(sanitized_file.content_type).to eq("image/jpeg")
     end
+
+    it "does not allow spoofing of the mime type" do
+      file = File.open(file_path("zip.png"))
+
+      sanitized_file = CarrierWave::SanitizedFile.new(file)
+      expect { sanitized_file.content_type }.not_to raise_error
+
+      expect(sanitized_file.content_type).to eq("application/zip")
+    end
+
+    it "does not raise an error if the path is not present" do
+      sanitized_file = CarrierWave::SanitizedFile.new(nil)
+
+      expect { sanitized_file.content_type }.not_to raise_error
+    end
   end
 
   describe "#content_type=" do

spec/uploader/content_type_blacklist_spec.rb

@@ -31,7 +31,7 @@
         end
 
         it "raises an integrity error if the file has a blacklisted content type" do
-          allow(uploader).to receive(:content_type_blacklist).and_return(['image/gif'])
+          allow(uploader).to receive(:content_type_blacklist).and_return(['image/png'])
 
           expect { uploader.cache!(ruby_file) }.to raise_error(CarrierWave::IntegrityError)
         end

spec/uploader/content_type_whitelist_spec.rb

@@ -25,7 +25,7 @@
         let(:bork_file) { File.open(file_path('bork.txt')) }
 
         it "does not raise an integrity error when the file has a whitelisted content type" do
-          allow(uploader).to receive(:content_type_whitelist).and_return(['image/gif'])
+          allow(uploader).to receive(:content_type_whitelist).and_return(['image/png'])
 
           expect { uploader.cache!(ruby_file) }.not_to raise_error
         end