Add latest changes from gitlab-org/security/gitlab@13-10-stable-ee

Gemfile

@@ -28,6 +28,8 @@ gem 'devise', '~> 4.7.2'
 gem 'bcrypt', '~> 3.1', '>= 3.1.14'
 gem 'doorkeeper', '~> 5.5.0.rc2'
 gem 'doorkeeper-openid_connect', '~> 1.7.5'
+gem 'rexml', '~> 3.2.5'
+gem 'ruby-saml', '~> 1.12.1'
 gem 'omniauth', '~> 1.8'
 gem 'omniauth-auth0', '~> 2.0.0'
 gem 'omniauth-azure-activedirectory-v2', '~> 0.1'

Gemfile.lock

@@ -1042,7 +1042,7 @@ GEM
     retriable (3.1.2)
     reverse_markdown (1.4.0)
       nokogiri
-    rexml (3.2.4)
+    rexml (3.2.5)
     rinku (2.0.0)
     rotp (2.1.2)
     rouge (3.26.0)
@@ -1116,8 +1116,9 @@ GEM
     ruby-magic-static (0.3.4)
     ruby-prof (1.3.1)
     ruby-progressbar (1.11.0)
-    ruby-saml (1.7.2)
-      nokogiri (>= 1.5.10)
+    ruby-saml (1.12.1)
+      nokogiri (>= 1.10.5)
+      rexml
     ruby-statistics (2.1.2)
     ruby2_keywords (0.0.2)
     ruby_parser (3.15.0)
@@ -1552,6 +1553,7 @@ DEPENDENCIES
   request_store (~> 1.5)
   responders (~> 3.0)
   retriable (~> 3.1.2)
+  rexml (~> 3.2.5)
   rouge (~> 3.26.0)
   rqrcode-rails3 (~> 0.1.7)
   rspec-parameterized
@@ -1563,6 +1565,7 @@ DEPENDENCIES
   ruby-magic-static (~> 0.3.4)
   ruby-prof (~> 1.3.0)
   ruby-progressbar (~> 1.10)
+  ruby-saml (~> 1.12.1)
   ruby_parser (~> 3.15)
   rubyzip (~> 2.0.0)
   rugged (~> 1.1)

changelogs/unreleased/security-327154-only-jpeg-tiff.yml

+---
+title: Clean only legitimate JPG and TIFF files
+merge_request:
+author:
+type: security

changelogs/unreleased/security-ruby-saml-auth-bypass-fix.yml

+---
+title: Update ruby-saml and rexml gems
+merge_request:
+author:
+type: security

lib/gitlab/sanitizers/exif.rb

@@ -45,6 +45,7 @@ module Gitlab
 
       ALLOWED_TAGS = WHITELISTED_TAGS + IGNORED_TAGS
       EXCLUDE_PARAMS = WHITELISTED_TAGS.map { |tag| "-#{tag}" }
+      ALLOWED_MIME_TYPES = %w(image/jpeg image/tiff).freeze
 
       attr_reader :logger
 
@@ -96,12 +97,12 @@ module Gitlab
         end
       end
 
+      private
+
       def extra_tags(path)
         exif_tags(path).keys - ALLOWED_TAGS
       end
 
-      private
-
       def remove_and_store(tmpdir, src_path, uploader)
         exec_remove_exif!(src_path)
         logger.info "#{upload_ref(uploader.upload)}: exif removed, storing"
@@ -133,15 +134,26 @@ module Gitlab
         # upload is stored into the file with the original name - this filename
         # is used by carrierwave when storing the file back to the storage
         filename = File.join(dir, uploader.filename)
+        contents = uploader.read
+
+        check_for_allowed_types(contents)
 
         File.open(filename, 'w') do |file|
           file.binmode
-          file.write uploader.read
+          file.write contents
         end
 
         filename
       end
 
+      def check_for_allowed_types(contents)
+        mime_type = Gitlab::Utils::MimeType.from_string(contents)
+
+        unless ALLOWED_MIME_TYPES.include?(mime_type)
+          raise "File type #{mime_type} not supported. Only supports #{ALLOWED_MIME_TYPES.join(", ")}."
+        end
+      end
+
       def upload_ref(upload)
         "#{upload.id}:#{upload.path}"
       end

spec/lib/gitlab/sanitizers/exif_spec.rb

@@ -4,6 +4,11 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Sanitizers::Exif do
   let(:sanitizer) { described_class.new }
+  let(:mime_type) { 'image/jpeg' }
+
+  before do
+    allow(Gitlab::Utils::MimeType).to receive(:from_string).and_return(mime_type)
+  end
 
   describe '#batch_clean' do
     context 'with image uploads' do
@@ -43,7 +48,7 @@ RSpec.describe Gitlab::Sanitizers::Exif do
       end
     end
 
-    it 'filters only jpg/tiff images' do
+    it 'filters only jpg/tiff images by filename' do
       create(:upload, path: 'filename.jpg')
       create(:upload, path: 'filename.jpeg')
       create(:upload, path: 'filename.JPG')
@@ -53,12 +58,16 @@ RSpec.describe Gitlab::Sanitizers::Exif do
       create(:upload, path: 'filename.txt')
 
       expect(sanitizer).to receive(:clean).exactly(5).times
+
       sanitizer.batch_clean
     end
   end
 
   describe '#clean' do
     let(:uploader) { create(:upload, :with_file, :issuable_upload).retrieve_uploader }
+    let(:dry_run) { false }
+
+    subject { sanitizer.clean(uploader, dry_run: dry_run) }
 
     context "no dry run" do
       it "removes exif from the image" do
@@ -76,7 +85,7 @@ RSpec.describe Gitlab::Sanitizers::Exif do
           [expected_args, 0]
         end
 
-        sanitizer.clean(uploader, dry_run: false)
+        subject
 
         expect(uploader.upload.id).not_to eq(original_upload.id)
         expect(uploader.upload.path).to eq(original_upload.path)
@@ -89,23 +98,35 @@ RSpec.describe Gitlab::Sanitizers::Exif do
         expect(sanitizer).not_to receive(:exec_remove_exif!)
         expect(uploader).not_to receive(:store!)
 
-        sanitizer.clean(uploader, dry_run: false)
+        subject
       end
 
       it "raises an error if the exiftool fails with an error" do
         expect(Gitlab::Popen).to receive(:popen).and_return(["error", 1])
 
-        expect { sanitizer.clean(uploader, dry_run: false) }.to raise_exception(RuntimeError, "failed to get exif tags: error")
+        expect { subject }.to raise_exception(RuntimeError, "failed to get exif tags: error")
+      end
+
+      context 'for files that do not have the correct MIME type' do
+        let(:mime_type) { 'text/plain' }
+
+        it 'cleans only jpg/tiff images with the correct mime types' do
+          expect(sanitizer).not_to receive(:extra_tags)
+
+          expect { subject }.to raise_error(RuntimeError, /File type text\/plain not supported/)
+        end
       end
     end
 
     context "dry run" do
+      let(:dry_run) { true }
+
       it "doesn't change the image" do
         expect(sanitizer).to receive(:extra_tags).and_return({ 'foo' => 'bar' })
         expect(sanitizer).not_to receive(:exec_remove_exif!)
         expect(uploader).not_to receive(:store!)
 
-        sanitizer.clean(uploader, dry_run: true)
+        subject
       end
     end
   end
@@ -119,7 +140,7 @@ RSpec.describe Gitlab::Sanitizers::Exif do
 
       expect(Gitlab::Popen).to receive(:popen).and_return([tags, 0])
 
-      expect(sanitizer.extra_tags('filename')).not_to be_empty
+      expect(sanitizer.send(:extra_tags, 'filename')).not_to be_empty
     end
 
     it "returns an empty list for file with only whitelisted and ignored tags" do
@@ -130,7 +151,7 @@ RSpec.describe Gitlab::Sanitizers::Exif do
 
       expect(Gitlab::Popen).to receive(:popen).and_return([tags, 0])
 
-      expect(sanitizer.extra_tags('some file')).to be_empty
+      expect(sanitizer.send(:extra_tags, 'some file')).to be_empty
     end
   end
 end