Commit edbdec94 authored by Alex Pooley's avatar Alex Pooley Committed by Michael Kozono

Ensure upload content matches filename extensions

Handle when extension is missing
Handle ambiguous file type checks
- Content of text files often don't define their type.
- A text file may be missing a file extension.
- We may encounter a file with no extension that we can't magically
type.
Optionally check integrity of specific mime types
Ensure upload integrity by file extension
Apply integrity check to AvatarUploader and FaviconUploader
parent c0b1e828
...@@ -5,6 +5,9 @@ class AvatarUploader < GitlabUploader ...@@ -5,6 +5,9 @@ class AvatarUploader < GitlabUploader
include RecordsUploads::Concern include RecordsUploads::Concern
include ObjectStorage::Concern include ObjectStorage::Concern
prepend ObjectStorage::Extension::RecordsUploads prepend ObjectStorage::Extension::RecordsUploads
include UploadTypeCheck::Concern
check_upload_type extensions: AvatarUploader::SAFE_IMAGE_EXT
def exists? def exists?
model.avatar.file && model.avatar.file.present? model.avatar.file && model.avatar.file.present?
......
# frozen_string_literal: true # frozen_string_literal: true
class FaviconUploader < AttachmentUploader class FaviconUploader < AttachmentUploader
include UploadTypeCheck::Concern
EXTENSION_WHITELIST = %w[png ico].freeze EXTENSION_WHITELIST = %w[png ico].freeze
check_upload_type extensions: EXTENSION_WHITELIST
def extension_whitelist def extension_whitelist
EXTENSION_WHITELIST EXTENSION_WHITELIST
end end
......
# frozen_string_literal: true
# Ensure that uploaded files are what they say they are for security and
# handling purposes. The checks are not 100% reliable so we err on the side of
# caution and allow by default, and deny when we're confident of a fail state.
#
# Include this concern, then call `check_upload_type` to check all
# uploads. Attach a `mime_type` or `extensions` parameter to only check
# specific upload types. Both parameters will be normalized to a MIME type and
# checked against the inferred MIME type of the upload content and filename
# extension.
#
# class YourUploader
# include UploadTypeCheck::Concern
# check_upload_type mime_types: ['image/png', /image\/jpe?g/]
#
# # or...
#
# check_upload_type extensions: ['png', 'jpg', 'jpeg']
# end
#
# The mime_types parameter can accept `NilClass`, `String`, `Regexp`,
# `Array[String, Regexp]`. This matches the CarrierWave `extension_whitelist`
# and `content_type_whitelist` family of behavior.
#
# The extensions parameter can accept `NilClass`, `String`, `Array[String]`.
module UploadTypeCheck
module Concern
extend ActiveSupport::Concern
class_methods do
def check_upload_type(mime_types: nil, extensions: nil)
define_method :check_upload_type_callback do |file|
magic_file = MagicFile.new(file.to_file)
# Map file extensions back to mime types.
if extensions
mime_types = Array(mime_types) +
Array(extensions).map { |e| MimeMagic::EXTENSIONS[e] }
end
if mime_types.nil? || magic_file.matches_mime_types?(mime_types)
check_content_matches_extension!(magic_file)
end
end
before :cache, :check_upload_type_callback
end
end
def check_content_matches_extension!(magic_file)
return if magic_file.ambiguous_type?
if magic_file.magic_type != magic_file.ext_type
raise CarrierWave::IntegrityError, 'Content type does not match file extension'
end
end
end
# Convenience class to wrap MagicMime objects.
class MagicFile
attr_reader :file
def initialize(file)
@file = file
end
def magic_type
@magic_type ||= MimeMagic.by_magic(file)
end
def ext_type
@ext_type ||= MimeMagic.by_path(file.path)
end
def magic_type_type
magic_type&.type
end
def ext_type_type
ext_type&.type
end
def matches_mime_types?(mime_types)
Array(mime_types).any? do |mt|
magic_type_type =~ /\A#{mt}\z/ || ext_type_type =~ /\A#{mt}\z/
end
end
# - Both types unknown or text/plain.
# - Ambiguous magic type with text extension. Plain text file.
# - Text magic type with ambiguous extension. TeX file missing extension.
def ambiguous_type?
(ext_type.to_s.blank? && magic_type.to_s.blank?) ||
(magic_type.to_s.blank? && ext_type_type == 'text/plain') ||
(ext_type.to_s.blank? && magic_type_type == 'text/plain')
end
end
end
---
title: Ensure content matches extension on image uploads
merge_request: 20697
author:
type: security
# frozen_string_literal: true
# Construct an `uploader` variable that is configured to `check_upload_type`
# with `mime_types` and `extensions`.
shared_context 'uploader with type check' do
let(:uploader_class) do
Class.new(GitlabUploader) do
include UploadTypeCheck::Concern
storage :file
end
end
let(:mime_types) { nil }
let(:extensions) { nil }
let(:uploader) do
uploader_class.class_exec(mime_types, extensions) do |mime_types, extensions|
check_upload_type mime_types: mime_types, extensions: extensions
end
uploader_class.new(build_stubbed(:user))
end
end
shared_context 'stubbed MimeMagic mime type detection' do
let(:mime_type) { '' }
let(:magic_mime) { mime_type }
let(:ext_mime) { mime_type }
before do
magic_mime_obj = MimeMagic.new(magic_mime)
ext_mime_obj = MimeMagic.new(ext_mime)
allow(MimeMagic).to receive(:by_magic).with(anything).and_return(magic_mime_obj)
allow(MimeMagic).to receive(:by_path).with(anything).and_return(ext_mime_obj)
end
end
# frozen_string_literal: true
def check_content_matches_extension!(file = double(read: nil, path: ''))
magic_file = UploadTypeCheck::MagicFile.new(file)
uploader.check_content_matches_extension!(magic_file)
end
shared_examples 'upload passes content type check' do
it 'does not raise error' do
expect { check_content_matches_extension! }.not_to raise_error
end
end
shared_examples 'upload fails content type check' do
it 'raises error' do
expect { check_content_matches_extension! }.to raise_error(CarrierWave::IntegrityError)
end
end
def upload_type_checked_filenames(filenames)
Array(filenames).each do |filename|
# Feed the uploader "some" content.
path = File.join('spec', 'fixtures', 'dk.png')
file = File.new(path, 'r')
# Rename the file with what we want.
allow(file).to receive(:path).and_return(filename)
# Force the content type to match the extension type.
mime_type = MimeMagic.by_path(filename)
allow(MimeMagic).to receive(:by_magic).and_return(mime_type)
uploaded_file = Rack::Test::UploadedFile.new(file, original_filename: filename)
uploader.cache!(uploaded_file)
end
end
def upload_type_checked_fixtures(upload_fixtures)
upload_fixtures = Array(upload_fixtures)
upload_fixtures.each do |upload_fixture|
path = File.join('spec', 'fixtures', upload_fixture)
uploader.cache!(fixture_file_upload(path))
end
end
shared_examples 'type checked uploads' do |upload_fixtures = nil, filenames: nil|
it 'check type' do
upload_fixtures = Array(upload_fixtures)
filenames = Array(filenames)
times = upload_fixtures.length + filenames.length
expect(uploader).to receive(:check_content_matches_extension!).exactly(times).times
upload_type_checked_fixtures(upload_fixtures) unless upload_fixtures.empty?
upload_type_checked_filenames(filenames) unless filenames.empty?
end
end
shared_examples 'skipped type checked uploads' do |upload_fixtures = nil, filenames: nil|
it 'skip type check' do
expect(uploader).not_to receive(:check_content_matches_extension!)
upload_type_checked_fixtures(upload_fixtures) if upload_fixtures
upload_type_checked_filenames(filenames) if filenames
end
end
...@@ -46,4 +46,16 @@ describe AvatarUploader do ...@@ -46,4 +46,16 @@ describe AvatarUploader do
expect(uploader.absolute_path).to eq(absolute_path) expect(uploader.absolute_path).to eq(absolute_path)
end end
end end
context 'upload type check' do
AvatarUploader::SAFE_IMAGE_EXT.each do |ext|
context "#{ext} extension" do
it_behaves_like 'type checked uploads', filenames: "image.#{ext}"
end
end
context 'skip image/svg+xml integrity check' do
it_behaves_like 'skipped type checked uploads', filenames: 'image.svg'
end
end
end end
# frozen_string_literal: true
require 'spec_helper'
describe FaviconUploader do
let_it_be(:model) { build_stubbed(:user) }
let_it_be(:uploader) { described_class.new(model, :favicon) }
context 'upload type check' do
FaviconUploader::EXTENSION_WHITELIST.each do |ext|
context "#{ext} extension" do
it_behaves_like 'type checked uploads', filenames: "image.#{ext}"
end
end
end
context 'upload non-whitelisted file extensions' do
it 'will deny upload' do
path = File.join('spec', 'fixtures', 'banana_sample.gif')
fixture_file = fixture_file_upload(path)
expect { uploader.cache!(fixture_file) }.to raise_exception(CarrierWave::IntegrityError)
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe UploadTypeCheck do
include_context 'uploader with type check'
def upload_fixture(filename)
fixture_file_upload(File.join('spec', 'fixtures', filename))
end
describe '#check_content_matches_extension! callback using file upload' do
context 'when extension matches contents' do
it 'not raise error on upload' do
expect { uploader.cache!(upload_fixture('banana_sample.gif')) }.not_to raise_error
end
end
context 'when extension does not match contents' do
it 'raise error' do
expect { uploader.cache!(upload_fixture('not_a_png.png')) }.to raise_error(CarrierWave::IntegrityError)
end
end
end
describe '#check_content_matches_extension! callback using stubs' do
include_context 'stubbed MimeMagic mime type detection'
context 'when no extension and with ambiguous/text content' do
let(:magic_mime) { '' }
let(:ext_mime) { '' }
it_behaves_like 'upload passes content type check'
end
context 'when no extension and with non-text content' do
let(:magic_mime) { 'image/gif' }
let(:ext_mime) { '' }
it_behaves_like 'upload fails content type check'
end
# Most text files will exhibit this behaviour.
context 'when ambiguous content with text extension' do
let(:magic_mime) { '' }
let(:ext_mime) { 'text/plain' }
it_behaves_like 'upload passes content type check'
end
context 'when text content with text extension' do
let(:magic_mime) { 'text/plain' }
let(:ext_mime) { 'text/plain' }
it_behaves_like 'upload passes content type check'
end
context 'when ambiguous content with non-text extension' do
let(:magic_mime) { '' }
let(:ext_mime) { 'application/zip' }
it_behaves_like 'upload fails content type check'
end
# These are the types when uploading a .dmg
context 'when content and extension do not match' do
let(:magic_mime) { 'application/x-bzip' }
let(:ext_mime) { 'application/x-apple-diskimage' }
it_behaves_like 'upload fails content type check'
end
end
describe '#check_content_matches_extension! mime_type filtering' do
context 'without mime types' do
let(:mime_types) { nil }
it_behaves_like 'type checked uploads', %w[doc_sample.txt rails_sample.jpg]
end
context 'with mime types string' do
let(:mime_types) { 'text/plain' }
it_behaves_like 'type checked uploads', %w[doc_sample.txt]
it_behaves_like 'skipped type checked uploads', %w[dk.png]
end
context 'with mime types regex' do
let(:mime_types) { [/image\/(gif|png)/] }
it_behaves_like 'type checked uploads', %w[banana_sample.gif dk.png]
it_behaves_like 'skipped type checked uploads', %w[doc_sample.txt]
end
context 'with mime types array' do
let(:mime_types) { ['text/plain', /image\/png/] }
it_behaves_like 'type checked uploads', %w[doc_sample.txt dk.png]
it_behaves_like 'skipped type checked uploads', %w[audio_sample.wav]
end
end
describe '#check_content_matches_extension! extensions filtering' do
context 'without extensions' do
let(:extensions) { nil }
it_behaves_like 'type checked uploads', %w[doc_sample.txt dk.png]
end
context 'with extensions string' do
let(:extensions) { 'txt' }
it_behaves_like 'type checked uploads', %w[doc_sample.txt]
it_behaves_like 'skipped type checked uploads', %w[rails_sample.jpg]
end
context 'with extensions array of strings' do
let(:extensions) { %w[txt png] }
it_behaves_like 'type checked uploads', %w[doc_sample.txt dk.png]
it_behaves_like 'skipped type checked uploads', %w[audio_sample.wav]
end
end
end
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