Prioritize Magic-detected content type for spoof-tolerance

Fixes #2570

lib/carrierwave/sanitized_file.rb

@@ -27,7 +27,7 @@ def sanitize_regexp
 
     def initialize(file)
       self.file = file
-      @content = nil
+      @content = @content_type = nil
     end
 
     ##
@@ -163,11 +163,7 @@ def move_to(new_path, permissions=nil, directory_permissions=nil, keep_filename=
       mkdir!(new_path, directory_permissions)
       move!(new_path)
       chmod!(new_path, permissions)
-      if keep_filename
-        self.file = {:tempfile => new_path, :filename => original_filename, :content_type => @content_type}
-      else
-        self.file = {:tempfile => new_path, :content_type => @content_type}
-      end
+      self.file = {tempfile: new_path, filename: keep_filename ? original_filename : nil, content_type: declared_content_type}
       self
     end
 
@@ -244,9 +240,10 @@ def to_file
     #
     def content_type
       @content_type ||=
-        existing_content_type ||
-        marcel_magic_by_mime_type ||
-        marcel_magic_by_path
+        identified_content_type ||
+        declared_content_type ||
+        guessed_safe_content_type ||
+        Marcel::MimeType::BINARY
     end
 
     ##
@@ -277,11 +274,11 @@ def file=(file)
       if file.is_a?(Hash)
         @file = file["tempfile"] || file[:tempfile]
         @original_filename = file["filename"] || file[:filename]
-        @content_type = file["content_type"] || file[:content_type] || file["type"] || file[:type]
+        @declared_content_type = file["content_type"] || file[:content_type] || file["type"] || file[:type]
       else
         @file = file
         @original_filename = nil
-        @content_type = nil
+        @declared_content_type = nil
       end
     end
 
@@ -307,33 +304,39 @@ def sanitize(name)
       name.mb_chars.to_s
     end
 
-    def existing_content_type
-      if @file.respond_to?(:content_type) && @file.content_type
-        @file.content_type.to_s.chomp
-      end
+    def declared_content_type
+      @declared_content_type ||
+        if @file.respond_to?(:content_type) && @file.content_type
+          @file.content_type.to_s.chomp
+        end
     end
 
-    def marcel_magic_by_mime_type
+    # Guess content type from its file extension. Limit what to be returned to prevent spoofing.
+    def guessed_safe_content_type
       return unless path
 
-      type = File.open(path) do |file|
-        Marcel::Magic.by_magic(file).try(:type)
-      end
+      type = Marcel::Magic.by_path(original_filename).to_s
+      type if type.start_with?('text/') || type.start_with?('application/json')
+    end
 
-      if type.nil?
-        type = Marcel::Magic.by_path(path).try(:type)
-        type = 'invalid/invalid' unless type.nil? || type.start_with?('text/') || type.start_with?('application/json')
+    def identified_content_type
+      with_io do |io|
+        Marcel::Magic.by_magic(io).try(:type)
       end
-
-      type
     rescue Errno::ENOENT
       nil
     end
 
-    def marcel_magic_by_path
-      return unless path
-
-      Marcel::Magic.by_path(path).to_s
+    def with_io(&block)
+      if file.is_a?(IO)
+        begin
+          yield file
+        ensure
+          file.try(:rewind)
+        end
+      elsif path
+        File.open(path, &block)
+      end
     end
   end # SanitizedFile
 end # CarrierWave

spec/sanitized_file_spec.rb

@@ -220,10 +220,10 @@
       expect(sanitized_file.content_type).to eq("application/vnd.openxmlformats-officedocument.presentationml.presentation")
     end
 
-    it "reads content type from path if missing" do
+    it "does not raise an error if missing" do
       sanitized_file = CarrierWave::SanitizedFile.new("llama.jpg")
 
-      expect(sanitized_file.content_type).to eq("image/jpeg")
+      expect(sanitized_file.content_type).to eq("application/octet-stream")
     end
 
     it "reads content type of a CSV linked to a file" do
@@ -234,7 +234,24 @@
       expect(sanitized_file.content_type).to eq("text/csv")
     end
 
-    it "does not allow spoofing of the mime type" do
+    it "does not allow spoofing of the mime type passed by Hash" do
+      file = File.open(file_path("landscape.jpg"))
+
+      sanitized_file = CarrierWave::SanitizedFile.new(tempfile: file, content_type: 'image/png')
+
+      expect(sanitized_file.content_type).to eq("image/jpeg")
+    end
+
+    it "does not allow spoofing of the mime type by setting file's content type" do
+      file = File.open(file_path("landscape.jpg"))
+      allow(file).to receive(:content_type) { 'image/png' }
+
+      sanitized_file = CarrierWave::SanitizedFile.new(file)
+
+      expect(sanitized_file.content_type).to eq("image/jpeg")
+    end
+
+    it "does not allow spoofing of the mime type by file extension" do
       file = File.open(file_path("zip.png"))
 
       sanitized_file = CarrierWave::SanitizedFile.new(file)
@@ -251,7 +268,7 @@
       expect { sanitized_file.content_type }.not_to raise_error
 
       expect(sanitized_file.content_type).to_not eq 'image/png'
-      expect(sanitized_file.content_type).to eq 'invalid/invalid'
+      expect(sanitized_file.content_type).to eq 'application/octet-stream'
     end
 
     it "returns valid content type on text file" do
@@ -274,14 +291,14 @@
       expect(sanitized_file.content_type).to eq 'application/json'
     end
 
-    it "returns missing content type with unknown extension" do
+    it "returns binary content type with unknown extension" do
       file = File.open(file_path('bork.ABCDE'))
 
       sanitized_file = CarrierWave::SanitizedFile.new(file)
 
       expect { sanitized_file.content_type }.not_to raise_error
 
-      expect(sanitized_file.content_type).to eq ""
+      expect(sanitized_file.content_type).to eq "application/octet-stream"
     end
 
     it "does not raise an error if the path is not present" do
@@ -406,17 +423,18 @@
       end
 
       it "should preserve the file's content type" do
-        sanitized_file.content_type = 'application/octet-stream'
+        sanitized_file.content_type = 'application/x-something'
         sanitized_file.move_to(file_path("new_dir", "gurr.png"))
 
-        expect(sanitized_file.content_type).to eq("application/octet-stream")
+        expect(sanitized_file.content_type).to eq("application/x-something")
       end
 
-      it "should detect content type correctly using Marcel when content_type is not set" do
+      it "should detect content type using Magic when content_type is reset" do
         sanitized_file.content_type = nil
         sanitized_file.move_to(file_path("new_dir", "gurr.png"))
 
-        expect(sanitized_file.content_type).to eq("invalid/invalid")
+        expect(sanitized_file).to receive(:identified_content_type)
+        sanitized_file.content_type
       end
 
       context 'target path only differs by case' do
@@ -508,6 +526,14 @@
         expect(new_file.content_type).to eq(sanitized_file.content_type)
       end
     end
+
+    describe "#with_io" do
+      it "should yield an IO object" do
+        sanitized_file.send(:with_io) do |io|
+          expect(io).to respond_to(:read)
+        end
+      end
+    end
   end
 
   shared_examples_for "all valid sanitized files that are stored on disk" do
@@ -577,6 +603,15 @@
         expect(sanitized_file.instance_variable_get(:@file).closed?).to be_falsey
       end
     end
+
+    describe "#with_io" do
+      it "should try to rewind the IO object on finish" do
+        sanitized_file.send(:with_io) do |io|
+          io.read
+        end
+        expect(sanitized_file.file.pos).to eq 0
+      end
+    end
   end
 
   shared_examples_for "all valid sanitized files that are read from a non-StringIO object" do

spec/uploader/download_spec.rb

@@ -50,7 +50,7 @@
       end
 
       it "sets the content type" do
-        expect(uploader.content_type).to eq("image/jpeg")
+        expect(uploader.file.send(:declared_content_type)).to eq("image/jpeg")
       end
     end
 

spec/uploader/proxy_spec.rb

@@ -100,7 +100,7 @@ def filename
     context "when the file is empty" do
       before { uploader.retrieve_from_cache!(path) }
 
-      it { is_expected.to eq('image/jpeg') }
+      it { is_expected.to eq('application/octet-stream') }
     end
   end
 end