Commit 9aee4112 by Jon Yurek Committed by Tute Costa

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.
parent ea1fc3c8
New in 4.2.2:
* Security fix: Fix a potential security issue with spoofing
New in 4.2.1: New in 4.2.1:
* Improvement: Added `validate_media_type` options to allow/bypass spoof check * Improvement: Added `validate_media_type` options to allow/bypass spoof check
......
...@@ -2,7 +2,7 @@ en: ...@@ -2,7 +2,7 @@ en:
errors: errors:
messages: messages:
in_between: "must be in between %{min} and %{max}" 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: number:
human: human:
......
module Paperclip module Paperclip
class MediaTypeSpoofDetector class MediaTypeSpoofDetector
def self.using(file, name) def self.using(file, name, content_type)
new(file, name) new(file, name, content_type)
end end
def initialize(file, name) def initialize(file, name, content_type)
@file = file @file = file
@name = name @name = name
@content_type = content_type || ""
end end
def spoofed? def spoofed?
if has_name? && has_extension? && media_type_mismatch? && mapping_override_mismatch? 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 true
else
false
end end
end end
...@@ -27,35 +30,44 @@ module Paperclip ...@@ -27,35 +30,44 @@ module Paperclip
end end
def media_type_mismatch? 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 end
def mapping_override_mismatch? def mapping_override_mismatch?
mapped_content_type != calculated_content_type mapped_content_type != calculated_content_type
end 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 end
def calculated_media_type def supplied_media_type
@calculated_media_type ||= calculated_content_type.split("/").first @content_type.split("/").first
end end
def supplied_file_content_types def content_types_from_name
@supplied_file_content_types ||= MIME::Types.type_for(@name).collect(&:content_type) @content_types_from_name ||= MIME::Types.type_for(@name)
end end
def calculated_content_type def media_types_from_name
@calculated_content_type ||= type_from_file_command.chomp @media_types_from_name ||= content_types_from_name.collect(&:media_type)
end end
def mapped_content_type def calculated_content_type
Paperclip.options[:content_type_mappings][filename_extension] @calculated_content_type ||= type_from_file_command.chomp
end end
def filename_extension def calculated_media_type
File.extname(@name.to_s.downcase).sub(/^\./, '').to_sym @calculated_media_type ||= calculated_content_type.split("/").first
end end
def type_from_file_command def type_from_file_command
...@@ -65,5 +77,13 @@ module Paperclip ...@@ -65,5 +77,13 @@ module Paperclip
"" ""
end end
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
end end
...@@ -5,7 +5,7 @@ module Paperclip ...@@ -5,7 +5,7 @@ module Paperclip
class MediaTypeSpoofDetectionValidator < ActiveModel::EachValidator class MediaTypeSpoofDetectionValidator < ActiveModel::EachValidator
def validate_each(record, attribute, value) def validate_each(record, attribute, value)
adapter = Paperclip.io_adapters.for(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) record.errors.add(attribute, :spoofed_media_type)
end end
end end
......
...@@ -43,4 +43,14 @@ describe Paperclip::MediaTypeSpoofDetector do ...@@ -43,4 +43,14 @@ describe Paperclip::MediaTypeSpoofDetector do
Paperclip.options[:content_type_mappings] = {} Paperclip.options[:content_type_mappings] = {}
end end
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 end
...@@ -30,7 +30,7 @@ describe Paperclip::Validators::MediaTypeSpoofDetectionValidator do ...@@ -30,7 +30,7 @@ describe Paperclip::Validators::MediaTypeSpoofDetectionValidator do
Paperclip::MediaTypeSpoofDetector.stubs(:using).returns(detector) Paperclip::MediaTypeSpoofDetector.stubs(:using).returns(detector)
@validator.validate(@dummy) @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 end
it "runs when attachment is dirty" do it "runs when attachment is dirty" do
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment