Merge branch 'ap-33785-file-integrity' into 'master'

Check that uploaded file content matches the file extension

See merge request gitlab-org/gitlab!20697

app/uploaders/avatar_uploader.rb

@@ -5,6 +5,9 @@ class AvatarUploader < GitlabUploader
   include RecordsUploads::Concern
   include ObjectStorage::Concern
   prepend ObjectStorage::Extension::RecordsUploads
+  include UploadTypeCheck::Concern
+
+  check_upload_type extensions: AvatarUploader::SAFE_IMAGE_EXT
 
   def exists?
     model.avatar.file && model.avatar.file.present?

app/uploaders/favicon_uploader.rb

 # frozen_string_literal: true
 
 class FaviconUploader < AttachmentUploader
+  include UploadTypeCheck::Concern
+
   EXTENSION_WHITELIST = %w[png ico].freeze
 
+  check_upload_type extensions: EXTENSION_WHITELIST
+
   def extension_whitelist
     EXTENSION_WHITELIST
   end

app/uploaders/upload_type_check.rb

+# 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

changelogs/unreleased/ap-33785-file-integrity.yml

+---
+title: Ensure content matches extension on image uploads
+merge_request: 20697
+author:
+type: security

spec/support/shared_contexts/upload_type_check_shared_context.rb

+# 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

spec/support/shared_examples/uploaders/upload_type_shared_examples.rb

+# 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

spec/uploaders/avatar_uploader_spec.rb

@@ -46,4 +46,16 @@ describe AvatarUploader do
       expect(uploader.absolute_path).to eq(absolute_path)
     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

spec/uploaders/favicon_uploader_spec.rb

+# 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

spec/uploaders/upload_type_check_spec.rb

+# 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