From 5386883416ed8ba9b6962ad09e810b981e8a7433 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Tue, 13 Apr 2021 18:57:51 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@13-8-stable-ee --- Gemfile | 2 ++ Gemfile.lock | 9 ++++-- .../unreleased/security-327154-only-jpeg-tiff.yml | 5 ++++ .../security-ruby-saml-auth-bypass-fix.yml | 5 ++++ lib/gitlab/sanitizers/exif.rb | 18 +++++++++-- spec/lib/gitlab/sanitizers/exif_spec.rb | 35 +++++++++++++++++----- 6 files changed, 61 insertions(+), 13 deletions(-) create mode 100644 changelogs/unreleased/security-327154-only-jpeg-tiff.yml create mode 100644 changelogs/unreleased/security-ruby-saml-auth-bypass-fix.yml diff --git a/Gemfile b/Gemfile index eab1984d05d..6e535daa6a2 100644 --- a/Gemfile +++ b/Gemfile @@ -27,6 +27,8 @@ gem 'devise', '~> 4.7.2' gem 'bcrypt', '3.1.12' gem 'doorkeeper', '~> 5.3.0' gem 'doorkeeper-openid_connect', '~> 1.7.4' +gem 'rexml', '~> 3.2.5' +gem 'ruby-saml', '~> 1.12.1' gem 'omniauth', '~> 1.8' gem 'omniauth-auth0', '~> 2.0.0' gem 'omniauth-azure-oauth2', '~> 0.0.9' diff --git a/Gemfile.lock b/Gemfile.lock index b545803c2d2..a1553aba7fd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -988,7 +988,7 @@ GEM mime-types (>= 1.16, < 4.0) netrc (~> 0.8) retriable (3.1.2) - rexml (3.2.4) + rexml (3.2.5) rinku (2.0.0) rotp (2.1.2) rouge (3.26.0) @@ -1062,8 +1062,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) @@ -1485,6 +1486,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 @@ -1496,6 +1498,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 (~> 0.28) diff --git a/changelogs/unreleased/security-327154-only-jpeg-tiff.yml b/changelogs/unreleased/security-327154-only-jpeg-tiff.yml new file mode 100644 index 00000000000..1751bd735bf --- /dev/null +++ b/changelogs/unreleased/security-327154-only-jpeg-tiff.yml @@ -0,0 +1,5 @@ +--- +title: Clean only legitimate JPG and TIFF files +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-ruby-saml-auth-bypass-fix.yml b/changelogs/unreleased/security-ruby-saml-auth-bypass-fix.yml new file mode 100644 index 00000000000..6f7d3c809c7 --- /dev/null +++ b/changelogs/unreleased/security-ruby-saml-auth-bypass-fix.yml @@ -0,0 +1,5 @@ +--- +title: Update ruby-saml and rexml gems +merge_request: +author: +type: security diff --git a/lib/gitlab/sanitizers/exif.rb b/lib/gitlab/sanitizers/exif.rb index ed3e32f3e79..eec50deb61e 100644 --- a/lib/gitlab/sanitizers/exif.rb +++ b/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 diff --git a/spec/lib/gitlab/sanitizers/exif_spec.rb b/spec/lib/gitlab/sanitizers/exif_spec.rb index 88ef3ce6aa5..63b2f3fc693 100644 --- a/spec/lib/gitlab/sanitizers/exif_spec.rb +++ b/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 -- cgit v1.2.1