CVE-2015-2963: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: The thoughtbot paperclip gem before 4.2.2 for Ruby does not consider the content-type value during media-type validation, which allows remote attackers to upload HTML documents and conduct cross-site scripting (XSS) attacks via a spoofed value, as demonstrated by image/jpeg.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
Published: 2015-07-10T17:59Z
Repository: https://github.com/thoughtbot/paperclip
Language: Ruby
Files changed: NEWS, lib/paperclip/locales/en.yml, lib/paperclip/media_type_spoof_detector.rb, lib/paperclip/validators/media_type_spoof_detection_validator.rb, spec/paperclip/media_type_spoof_detector_spec.rb, spec/paperclip/validators/media_type_spoof_detection_validator_spec.rb
Commit date: 2015-06-05 11:25:07 -0400
Commit message: Fix a possible security issue with spoofing

Thanks to MORI Shingo of DeNA Co., Ltd. for reporting this.

There is an issue where if an HTML file is uploaded with a .html
extension, but the content type is listed as being `image/jpeg`, this
will bypass a validation checking for images. But it will also pass the
spoof check, because a file named .html and containing actual HTML
passes the spoof check.

This change makes it so that we also check the supplied content type. So
even if the file contains HTML and ends with .html, it doesn't match the
content type of `image/jpeg` and so it fails.

Diff: diff --git a/NEWS b/NEWS
index a3953c2..41923fc 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,9 @@
+New in 4.2.2:
+
+* Security fix: Fix a potential security issue with spoofing
+
 New in 4.2.1:
 
 * Improvement: Added `validate_media_type` options to allow/bypass spoof check
 * Improvement: Added incremental backoff when AWS gives us a SlowDown error.
 * Improvement: Stream downloads when usign aws-sdk.
diff --git a/lib/paperclip/locales/en.yml b/lib/paperclip/locales/en.yml
index 43fd583..2709a02 100644
--- a/lib/paperclip/locales/en.yml
+++ b/lib/paperclip/locales/en.yml
@@ -1,10 +1,10 @@
 en:
   errors:
     messages:
       in_between: "must be in between %{min} and %{max}"
-      spoofed_media_type: "has an extension that does not match its contents"
+      spoofed_media_type: "has contents that are not what they are reported to be"
 
   number:
     human:
       storage_units:
         format: "%n %u"
diff --git a/lib/paperclip/media_type_spoof_detector.rb b/lib/paperclip/media_type_spoof_detector.rb
index 222efe5..9a57857 100644
--- a/lib/paperclip/media_type_spoof_detector.rb
+++ b/lib/paperclip/media_type_spoof_detector.rb
@@ -1,20 +1,23 @@
 module Paperclip
   class MediaTypeSpoofDetector
-    def self.using(file, name)
-      new(file, name)
+    def self.using(file, name, content_type)
+      new(file, name, content_type)
     end
 
-    def initialize(file, name)
+    def initialize(file, name, content_type)
       @file = file
       @name = name
+      @content_type = content_type || ""
     end
 
     def spoofed?
       if has_name? && has_extension? && media_type_mismatch? && mapping_override_mismatch?
-        Paperclip.log("Content Type Spoof: Filename #{File.basename(@name)} (#{supplied_file_content_types}), content type discovered from file command: #{calculated_content_type}. See documentation to allow this combination.")
+        Paperclip.log("Content Type Spoof: Filename #{File.basename(@name)} (#{supplied_content_type} from Headers, #{content_types_from_name} from Extension), content type discovered from file command: #{calculated_content_type}. See documentation to allow this combination.")
         true
+      else
+        false
       end
     end
 
     private
 
@@ -25,45 +28,62 @@ module Paperclip
     def has_extension?
       File.extname(@name).present?
     end
 
     def media_type_mismatch?
-      ! supplied_file_media_types.include?(calculated_media_type)
+      supplied_type_mismatch? || calculated_type_mismatch?
+    end
+
+    def supplied_type_mismatch?
+      supplied_media_type.present? && !media_types_from_name.include?(supplied_media_type)
+    end
+
+    def calculated_type_mismatch?
+      !media_types_from_name.include?(calculated_media_type)
     end
 
     def mapping_override_mismatch?
       mapped_content_type != calculated_content_type
     end
 
-    def supplied_file_media_types
-      @supplied_file_media_types ||= MIME::Types.type_for(@name).collect(&:media_type)
+
+    def supplied_content_type
+      @content_type
     end
 
-    def calculated_media_type
-      @calculated_media_type ||= calculated_content_type.split("/").first
+    def supplied_media_type
+      @content_type.split("/").first
     end
 
-    def supplied_file_content_types
-      @supplied_file_content_types ||= MIME::Types.type_for(@name).collect(&:content_type)
+    def content_types_from_name
+      @content_types_from_name ||= MIME::Types.type_for(@name)
     end
 
-    def calculated_content_type
-      @calculated_content_type ||= type_from_file_command.chomp
+    def media_types_from_name
+      @media_types_from_name ||= content_types_from_name.collect(&:media_type)
     end
 
-    def mapped_content_type
-      Paperclip.options[:content_type_mappings][filename_extension]
+    def calculated_content_type
+      @calculated_content_type ||= type_from_file_command.chomp
     end
 
-    def filename_extension
-      File.extname(@name.to_s.downcase).sub(/^\./, '').to_sym
+    def calculated_media_type
+      @calculated_media_type ||= calculated_content_type.split("/").first
     end
 
     def type_from_file_command
       begin
         Paperclip.run("file", "-b --mime :file", :file => @file.path).split(/[:;]\s+/).first
       rescue Cocaine::CommandLineError
         ""
       end
     end
+
+    def mapped_content_type
+      Paperclip.options[:content_type_mappings][filename_extension]
+    end
+
+    def filename_extension
+      File.extname(@name.to_s.downcase).sub(/^\./, '').to_sym
+    end
   end
 end
diff --git a/lib/paperclip/validators/media_type_spoof_detection_validator.rb b/lib/paperclip/validators/media_type_spoof_detection_validator.rb
index 851c5ff..d3699ac 100644
--- a/lib/paperclip/validators/media_type_spoof_detection_validator.rb
+++ b/lib/paperclip/validators/media_type_spoof_detection_validator.rb
@@ -3,11 +3,11 @@ require 'active_model/validations/presence'
 module Paperclip
   module Validators
     class MediaTypeSpoofDetectionValidator < ActiveModel::EachValidator
       def validate_each(record, attribute, value)
         adapter = Paperclip.io_adapters.for(value)
-        if Paperclip::MediaTypeSpoofDetector.using(adapter, value.original_filename).spoofed?
+        if Paperclip::MediaTypeSpoofDetector.using(adapter, value.original_filename, value.content_type).spoofed?
           record.errors.add(attribute, :spoofed_media_type)
         end
       end
     end
 
diff --git a/spec/paperclip/media_type_spoof_detector_spec.rb b/spec/paperclip/media_type_spoof_detector_spec.rb
index 134d6d2..b11095b 100644
--- a/spec/paperclip/media_type_spoof_detector_spec.rb
+++ b/spec/paperclip/media_type_spoof_detector_spec.rb
@@ -41,6 +41,16 @@ describe Paperclip::MediaTypeSpoofDetector do
       assert ! Paperclip::MediaTypeSpoofDetector.using(adapter, adapter.original_filename).spoofed?
     ensure
       Paperclip.options[:content_type_mappings] = {}
     end
   end
+
+  it "rejects a file if named .html and is as HTML, but we're told JPG" do
+    file = File.open(fixture_file("empty.html"))
+    assert Paperclip::MediaTypeSpoofDetector.using(file, "empty.html", "image/jpg").spoofed?
+  end
+
+  it "does not reject is content_type is empty but otherwise checks out" do
+    file = File.open(fixture_file("empty.html"))
+    assert ! Paperclip::MediaTypeSpoofDetector.using(file, "empty.html", "").spoofed?
+  end
 end
diff --git a/spec/paperclip/validators/media_type_spoof_detection_validator_spec.rb b/spec/paperclip/validators/media_type_spoof_detection_validator_spec.rb
index 157702b..384a106 100644
--- a/spec/paperclip/validators/media_type_spoof_detection_validator_spec.rb
+++ b/spec/paperclip/validators/media_type_spoof_detection_validator_spec.rb
@@ -28,11 +28,11 @@ describe Paperclip::Validators::MediaTypeSpoofDetectionValidator do
 
     detector = mock("detector", :spoofed? => true)
     Paperclip::MediaTypeSpoofDetector.stubs(:using).returns(detector)
     @validator.validate(@dummy)
 
-    assert_equal "has an extension that does not match its contents", @dummy.errors[:avatar].first
+    assert_equal I18n.t("errors.messages.spoofed_media_type"), @dummy.errors[:avatar].first
   end
 
   it "runs when attachment is dirty" do
     build_validator
     file = File.new(fixture_file("5k.png"), "rb")

Vulnerable code:       spoofed_media_type: "has an extension that does not match its contents"
    def self.using(file, name)
      new(file, name)
    def initialize(file, name)
        Paperclip.log("Content Type Spoof: Filename #{File.basename(@name)} (#{supplied_file_content_types}), content type discovered from file command: #{calculated_content_type}. See documentation to allow this combination.")
      ! supplied_file_media_types.include?(calculated_media_type)
    def supplied_file_media_types
      @supplied_file_media_types ||= MIME::Types.type_for(@name).collect(&:media_type)
    def calculated_media_type
      @calculated_media_type ||= calculated_content_type.split("/").first
    def supplied_file_content_types
      @supplied_file_content_types ||= MIME::Types.type_for(@name).collect(&:content_type)
    def calculated_content_type
      @calculated_content_type ||= type_from_file_command.chomp
    def mapped_content_type
      Paperclip.options[:content_type_mappings][filename_extension]
    def filename_extension
      File.extname(@name.to_s.downcase).sub(/^\./, '').to_sym
        if Paperclip::MediaTypeSpoofDetector.using(adapter, value.original_filename).spoofed?
    assert_equal "has an extension that does not match its contents", @dummy.errors[:avatar].first
Fixed code: New in 4.2.2:

* Security fix: Fix a potential security issue with spoofing

      spoofed_media_type: "has contents that are not what they are reported to be"
    def self.using(file, name, content_type)
      new(file, name, content_type)
    def initialize(file, name, content_type)
      @content_type = content_type || ""
        Paperclip.log("Content Type Spoof: Filename #{File.basename(@name)} (#{supplied_content_type} from Headers, #{content_types_from_name} from Extension), content type discovered from file command: #{calculated_content_type}. See documentation to allow this combination.")
      else
        false
      supplied_type_mismatch? || calculated_type_mismatch?
    end

    def supplied_type_mismatch?
      supplied_media_type.present? && !media_types_from_name.include?(supplied_media_type)
    end

    def calculated_type_mismatch?
      !media_types_from_name.include?(calculated_media_type)

    def supplied_content_type
      @content_type
    def supplied_media_type
      @content_type.split("/").first
    def content_types_from_name
      @content_types_from_name ||= MIME::Types.type_for(@name)
    def media_types_from_name
      @media_types_from_name ||= content_types_from_name.collect(&:media_type)
    def calculated_content_type
      @calculated_content_type ||= type_from_file_command.chomp
    def calculated_media_type
      @calculated_media_type ||= calculated_content_type.split("/").first

    def mapped_content_type
      Paperclip.options[:content_type_mappings][filename_extension]
    end

    def filename_extension
      File.extname(@name.to_s.downcase).sub(/^\./, '').to_sym
    end
        if Paperclip::MediaTypeSpoofDetector.using(adapter, value.original_filename, value.content_type).spoofed?

  it "rejects a file if named .html and is as HTML, but we're told JPG" do
    file = File.open(fixture_file("empty.html"))
    assert Paperclip::MediaTypeSpoofDetector.using(file, "empty.html", "image/jpg").spoofed?
  end

  it "does not reject is content_type is empty but otherwise checks out" do
    file = File.open(fixture_file("empty.html"))
    assert ! Paperclip::MediaTypeSpoofDetector.using(file, "empty.html", "").spoofed?
  end
    assert_equal I18n.t("errors.messages.spoofed_media_type"), @dummy.errors[:avatar].first