Merge tag 'debian/13.9.6+ds1-1' into buster-fasttrack

gitlab Debian release 13.9.6+ds1-1

CHANGELOG.md

@@ -2,6 +2,14 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 13.9.6 (2021-04-13)
+
+### Security (2 changes)
+
+- Clean only legitimate JPG and TIFF files.
+- Update ruby-saml and rexml gems.
+
+
 ## 13.9.5 (2021-03-31)
 
 ### Security (6 changes)

GITALY_SERVER_VERSION

@@ -1 +1 @@
-13.9.5
+13.9.6

GITLAB_WORKHORSE_VERSION

@@ -1 +1 @@
-8.63.2
+8.63.3

Gemfile

@@ -29,6 +29,8 @@ gem 'devise', '~> 4.7.2'
 gem 'bcrypt', '3.1.12'
 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-oauth2', '~> 0.0.9'

Gemfile.lock

@@ -998,7 +998,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)
@@ -1072,8 +1072,9 @@ GEM
     ruby-magic-static (0.3.4)
     ruby-prof (1.3.1)
     ruby-progressbar (1.11.0)
-    ruby-saml (1.7.2)
+    ruby-saml (1.12.1)
-      nokogiri (>= 1.5.10)
+      nokogiri (>= 1.10.5)
+      rexml
     ruby-statistics (2.1.2)
     ruby2_keywords (0.0.2)
     ruby_parser (3.15.0)
@@ -1498,6 +1499,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
@@ -1509,6 +1511,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.0.1)

VERSION

@@ -1 +1 @@
-13.9.5
+13.9.6

debian/changelog

@@ -1,3 +1,12 @@
+gitlab (13.9.6+ds1-1) experimental; urgency=medium
+
+  * New upstream security release 13.9.6+ds1
+  * Update minimum version of ruby-rexml to 3.2.5
+  * Add ruby-saml as a dependency
+  * Refresh patches
+
+ -- Pirate Praveen <praveen@debian.org>  Thu, 15 Apr 2021 22:42:45 +0530
+
 gitlab (13.9.5+ds1-1~fto10+1) buster-fasttrack; urgency=medium
 
   * Rebuild for buster-fasttrack.

debian/control

@@ -56,6 +56,8 @@ Depends: ${shlibs:Depends}, ${misc:Depends},
  ruby-bcrypt (>= 3.1.14~),
  ruby-doorkeeper (>= 5.5~),
  ruby-doorkeeper-openid-connect (>= 1.7.5~),
+ ruby-rexml (>= 3.2.5~),
+ ruby-saml (>= 1.12.1~),
  ruby-omniauth (>= 1.8~),
  ruby-omniauth-auth0 (>= 2.0~),
  ruby-omniauth-azure-oauth2 (>= 0.0.10~),

debian/patches/0050-relax-stable-libs.patch

@@ -3,7 +3,7 @@ gitlab Gemfile
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -2,53 +2,53 @@
+@@ -2,55 +2,55 @@
  
  source 'https://rubygems.org'
  
@@ -42,9 +42,13 @@ gitlab Gemfile
 -gem 'bcrypt', '3.1.12'
 -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 'bcrypt', '~> 3.1', '>= 3.1.12'
 +gem 'doorkeeper', '~> 5.5'
 +gem 'doorkeeper-openid_connect', '~> 1.7', '>= 1.7.5'
++gem 'rexml', '~> 3.2', '>= 3.2.5'
++gem 'ruby-saml', '~> 1.12', '>= 1.12.1'
  gem 'omniauth', '~> 1.8'
 -gem 'omniauth-auth0', '~> 2.0.0'
 +gem 'omniauth-auth0', '~> 2.0'
@@ -77,7 +81,7 @@ gitlab Gemfile
  
  # Kerberos authentication. EE-only
  gem 'gssapi', group: :kerberos
-@@ -56,17 +56,17 @@
+@@ -58,17 +58,17 @@
  # Spam and anti-bot protection
  gem 'recaptcha', '~> 4.11', require: 'recaptcha/rails'
  gem 'akismet', '~> 3.0'
@@ -100,7 +104,7 @@ gitlab Gemfile
  # GitLab Pages letsencrypt support
  gem 'acme-client', '~> 2.0', '>= 2.0.6'
  
-@@ -74,27 +74,27 @@
+@@ -76,27 +76,27 @@
  gem 'browser', '~> 4.2'
  
  # GPG
@@ -136,7 +140,7 @@ gitlab Gemfile
  gem 'graphlient', '~> 0.4.0' # Used by BulkImport feature (group::import)
  
  gem 'hashie'
-@@ -105,11 +105,11 @@
+@@ -107,11 +107,11 @@
  gem 'kaminari', '~> 1.0'
  
  # HAML
@@ -150,7 +154,7 @@ gitlab Gemfile
  
  # for backups
  gem 'fog-aws', '~> 3.8'
-@@ -130,37 +130,37 @@
+@@ -132,37 +132,37 @@
  gem 'unf', '~> 0.1.4'
  
  # Seed data
@@ -201,7 +205,7 @@ gitlab Gemfile
  gem 'escape_utils', '~> 1.1'
  
  # Calendar rendering
-@@ -171,7 +171,7 @@
+@@ -173,7 +173,7 @@
  gem 'diff_match_patch', '~> 0.1.0'
  
  # Application server
@@ -210,7 +214,7 @@ gitlab Gemfile
  # https://github.com/sharpstone/rack-timeout/blob/master/README.md#rails-apps-manually
  gem 'rack-timeout', '~> 0.5.1', require: 'rack/timeout/base'
  
-@@ -181,7 +181,7 @@
+@@ -183,7 +183,7 @@
  end
  
  group :puma do
@@ -219,7 +223,7 @@ gitlab Gemfile
    gem 'puma_worker_killer', '~> 0.3.1', require: false
  end
  
-@@ -192,13 +192,13 @@
+@@ -194,13 +194,13 @@
  gem 'acts-as-taggable-on', '~> 7.0'
  
  # Background jobs
@@ -236,7 +240,7 @@ gitlab Gemfile
  
  # HTTP requests
  gem 'httparty', '~> 0.16.4'
-@@ -210,14 +210,14 @@
+@@ -212,14 +212,14 @@
  gem 'ruby-progressbar', '~> 1.10'
  
  # GitLab settings
@@ -254,7 +258,7 @@ gitlab Gemfile
  
  # Export Ruby Regex to Javascript
  gem 'js_regex', '~> 3.4'
-@@ -230,20 +230,20 @@
+@@ -232,20 +232,20 @@
  gem 'connection_pool', '~> 2.0'
  
  # Redis session store
@@ -278,7 +282,7 @@ gitlab Gemfile
  
  # Hangouts Chat integration
  gem 'hangouts-chat', '~> 0.0.5'
-@@ -255,11 +255,11 @@
+@@ -257,11 +257,11 @@
  gem 'ruby-fogbugz', '~> 0.2.1'
  
  # Kubernetes integration
@@ -293,7 +297,7 @@ gitlab Gemfile
  
  # Sanitizes SVG input
  gem 'loofah', '~> 2.2'
-@@ -285,9 +285,9 @@
+@@ -287,9 +287,9 @@
  
  gem 'rack-proxy', '~> 0.6.0'
  
@@ -306,7 +310,7 @@ gitlab Gemfile
  
  gem 'addressable', '~> 2.7'
  gem 'gemojione', '~> 3.3'
-@@ -298,18 +298,18 @@
+@@ -300,18 +300,18 @@
  gem "gitlab-license", "~> 1.3"
  
  # Protect against bruteforcing
@@ -329,7 +333,7 @@ gitlab Gemfile
  # Thrift is a dependency of gitlab-labkit, we want a version higher than 0.14.0
  # because of https://gitlab.com/gitlab-org/gitlab/-/issues/321900
  gem 'thrift', '>= 0.14.0'
-@@ -317,11 +317,11 @@
+@@ -319,11 +319,11 @@
  # I18n
  gem 'ruby_parser', '~> 3.15', require: false
  gem 'rails-i18n', '~> 6.0'
@@ -343,7 +347,7 @@ gitlab Gemfile
  
  # Perf bar
  gem 'peek', '~> 1.1'
-@@ -354,39 +354,39 @@
+@@ -356,39 +356,39 @@
  end
  
  group :development, :test do
@@ -394,7 +398,7 @@ gitlab Gemfile
  
    gem 'timecop', '~> 0.9.1'
  
-@@ -408,18 +408,18 @@
+@@ -410,18 +410,18 @@
  end
  
  group :test do
@@ -419,7 +423,7 @@ gitlab Gemfile
    gem 'rails-controller-testing'
    gem 'concurrent-ruby', '~> 1.1'
    gem 'test-prof', '~> 0.12.0'
-@@ -438,7 +438,7 @@
+@@ -440,7 +440,7 @@
  gem 'email_reply_trimmer', '~> 0.1'
  gem 'html2text'
  
@@ -428,7 +432,7 @@ gitlab Gemfile
  gem 'stackprof', '~> 0.2.15', require: false
  gem 'rbtrace', '~> 0.4', require: false
  gem 'memory_profiler', '~> 0.9', require: false
-@@ -452,8 +452,8 @@
+@@ -454,8 +454,8 @@
  gem 'health_check', '~> 3.0'
  
  # System information
@@ -439,7 +443,7 @@ gitlab Gemfile
  
  # NTP client
  gem 'net-ntp'
-@@ -469,13 +469,13 @@
+@@ -471,13 +471,13 @@
  end
  
  # Gitaly GRPC protocol definitions
@@ -456,7 +460,7 @@ gitlab Gemfile
  
  # Feature toggles
  gem 'flipper', '~> 0.17.1'
-@@ -494,12 +494,12 @@
+@@ -496,12 +496,12 @@
  # Countries list
  gem 'countries', '~> 3.0'
  
@@ -471,7 +475,7 @@ gitlab Gemfile
  
  # Locked as long as quoted-printable encoding issues are not resolved
  # Monkey-patched in `config/initializers/mail_encoding_patch.rb`
-@@ -513,12 +513,12 @@
+@@ -515,12 +515,12 @@
  gem 'valid_email', '~> 0.1'
  
  # JSON

debian/patches/0100-remove-development-test.patch

@@ -2,7 +2,7 @@ Bundler will fail when it can't find these locally
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -94,7 +94,6 @@
+@@ -96,7 +96,6 @@
  # https://gitlab.com/gitlab-org/gitlab/issues/31747
  gem 'graphiql-rails', '~> 1.4', '>= 1.4.10'
  gem 'apollo_upload_server', '~> 2.0', '>= 2.0.2'
@@ -10,7 +10,7 @@ Bundler will fail when it can't find these locally
  gem 'graphlient', '~> 0.4.0' # Used by BulkImport feature (group::import)
  
  gem 'hashie'
-@@ -319,7 +318,6 @@
+@@ -321,7 +320,6 @@
  gem 'rails-i18n', '~> 6.0'
  gem 'gettext_i18n_rails', '~> 1.8'
  gem 'gettext_i18n_rails_js', '~> 1.3'
@@ -18,7 +18,7 @@ Bundler will fail when it can't find these locally
  
  gem 'batch-loader', '~> 1.4'
  
-@@ -339,20 +337,6 @@
+@@ -341,20 +339,6 @@
    gem 'raindrops', '~> 0.18'
  end
  
@@ -39,7 +39,7 @@ Bundler will fail when it can't find these locally
  group :development, :test do
    gem 'deprecation_toolkit', '~> 1.5', '>= 1.5.1', require: false
    gem 'bullet', '~> 6.1', '>= 6.1.3'
-@@ -375,12 +359,6 @@
+@@ -377,12 +361,6 @@
    gem 'spring', '~> 2.1'
    gem 'spring-commands-rspec', '~> 1.0', '>= 1.0.4'
  
@@ -52,7 +52,7 @@ Bundler will fail when it can't find these locally
    gem 'benchmark-ips', '~> 2.3', require: false
  
    gem 'knapsack', '~> 1.17'
-@@ -397,16 +375,6 @@
+@@ -399,16 +377,6 @@
    gem 'rblineprof', '~> 0.3.6', platform: :mri, require: false
  end
  

debian/patches/0110-make-test-dependencies-conditional.patch

@@ -2,7 +2,7 @@ Make test dependencies conditional so we can enable them when running autopkgtes
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -337,7 +337,7 @@
+@@ -339,7 +339,7 @@
    gem 'raindrops', '~> 0.18'
  end
  
@@ -11,7 +11,7 @@ Make test dependencies conditional so we can enable them when running autopkgtes
    gem 'deprecation_toolkit', '~> 1.5', '>= 1.5.1', require: false
    gem 'bullet', '~> 6.1', '>= 6.1.3'
    gem 'gitlab-pry-byebug', platform: :mri, require: ['pry-byebug', 'pry-byebug/pry_remote_ext']
-@@ -373,9 +373,7 @@
+@@ -375,9 +375,7 @@
    gem 'parallel', '~> 1.19', require: false
  
    gem 'rblineprof', '~> 0.3.6', platform: :mri, require: false

debian/patches/0340-relax-httparty.patch

@@ -2,7 +2,7 @@ Allow newer versions of httparty to satisfy dependency
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -200,7 +200,7 @@
+@@ -202,7 +202,7 @@
  gem 'fugit', '~> 1.2', '>= 1.2.1'
  
  # HTTP requests

debian/patches/0350-relax-rdoc.patch

@@ -2,7 +2,7 @@ Allow rdoc from ruby 2.5 to match requirement
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -148,7 +148,7 @@
+@@ -150,7 +150,7 @@
  gem 'commonmarker', '~> 0.21'
  gem 'kramdown', '~> 2.3'
  gem 'RedCloth', '~> 4.3', '>= 4.3.2'

debian/patches/0430-remove-gitlab-markup.patch

@@ -4,7 +4,7 @@ maintaining two almost same packages.
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -143,7 +143,6 @@
+@@ -145,7 +145,6 @@
  # Markdown and HTML processing
  gem 'html-pipeline', '~> 2.13', '>= 2.13.2'
  gem 'deckar01-task_list', '~> 2.3', '>= 2.3.1'

debian/patches/0440-remove-unicorn.patch

@@ -3,7 +3,7 @@ gitlab-puma changes is included in puma package.
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -173,11 +173,6 @@
+@@ -175,11 +175,6 @@
  # https://github.com/sharpstone/rack-timeout/blob/master/README.md#rails-apps-manually
  gem 'rack-timeout', '~> 0.5.1', require: 'rack/timeout/base'
  

debian/patches/0480-embed-elasticsearch-model.patch

@@ -2,7 +2,7 @@ Embed this gem until gitlab moved to 7.x version
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -132,7 +132,7 @@
+@@ -134,7 +134,7 @@
  gem 'seed-fu', '~> 2.3', '>= 2.3.7'
  
  # Search

debian/patches/0480-embed-elasticsearch-rails.patch

@@ -2,7 +2,7 @@ Embed this gem until gitlab moved to 7.x version
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -133,7 +133,7 @@
+@@ -135,7 +135,7 @@
  
  # Search
  gem 'elasticsearch-model', '~> 6.1', path: 'vendor/gems/elasticsearch-model'

debian/patches/0482-remove-ee-only-gems.patch

@@ -2,7 +2,7 @@ This gem is used only in gitlab Enterprise Edition
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -50,9 +50,6 @@
+@@ -52,9 +52,6 @@
  gem 'rack-oauth2', '~> 1.16'
  gem 'jwt', '~> 2.1'
  
@@ -12,7 +12,7 @@ This gem is used only in gitlab Enterprise Edition
  # Spam and anti-bot protection
  gem 'recaptcha', '~> 4.11', require: 'recaptcha/rails'
  gem 'akismet', '~> 3.0'
-@@ -288,8 +285,6 @@
+@@ -290,8 +287,6 @@
  gem 'request_store', '~> 1.5'
  gem 'base32', '~> 0.3.0'
  

debian/patches/0484-relax-grape-entity.patch

@@ -2,7 +2,7 @@ Debian already has 0.8
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -81,7 +81,7 @@
+@@ -83,7 +83,7 @@
  
  # API
  gem 'grape', '~> 1.5', '>= 1.5.2'

debian/patches/0485-relax-gitlab-sidekiq-fetcher.patch

@@ -2,7 +2,7 @@ Allow newer version in the archive to satisfy the requirement
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -185,7 +185,7 @@
+@@ -187,7 +187,7 @@
  gem 'sidekiq', '~> 5.2', '>= 5.2.7'
  gem 'sidekiq-cron', '~> 1.0'
  gem 'redis-namespace', '~> 1.7'

debian/patches/0486-relax-sidekiq.patch

@@ -2,7 +2,7 @@ ruby-sidekiq 6 is in unstable
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -182,7 +182,7 @@
+@@ -184,7 +184,7 @@
  gem 'acts-as-taggable-on', '~> 7.0'
  
  # Background jobs

debian/patches/0499-10-relax-capybara.patch

@@ -1,6 +1,6 @@
 --- a/Gemfile
 +++ b/Gemfile
-@@ -368,7 +368,7 @@
+@@ -370,7 +370,7 @@
    gem 'rspec_profiling', '~> 0.0.6'
    gem 'rspec-parameterized', require: false
  

debian/patches/0499-20-remove-capybara-screenshot.patch

@@ -1,6 +1,6 @@
 --- a/Gemfile
 +++ b/Gemfile
-@@ -369,7 +369,6 @@
+@@ -371,7 +371,6 @@
    gem 'rspec-parameterized', require: false
  
    gem 'capybara', '~> 3.12'

debian/patches/0499-30-remove-guard-rspec.patch

@@ -1,6 +1,6 @@
 --- a/Gemfile
 +++ b/Gemfile
-@@ -378,7 +378,6 @@
+@@ -380,7 +380,6 @@
    gem 'concurrent-ruby', '~> 1.1'
    gem 'test-prof', '~> 0.12.0'
    gem 'rspec_junit_formatter'

debian/patches/0499-40-relax-rouge.patch

@@ -3,7 +3,7 @@ rouge update is blocked by
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -152,7 +152,7 @@
+@@ -154,7 +154,7 @@
  gem 'asciidoctor-include-ext', '~> 0.3.1', require: false
  gem 'asciidoctor-plantuml', '~> 0.0.12'
  gem 'asciidoctor-kroki', '~> 0.4.0', require: false

debian/patches/0499-70-relax-graphlient.patch

@@ -2,7 +2,7 @@ newer version is in the archive
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -91,7 +91,7 @@
+@@ -93,7 +93,7 @@
  # https://gitlab.com/gitlab-org/gitlab/issues/31747
  gem 'graphiql-rails', '~> 1.4', '>= 1.4.10'
  gem 'apollo_upload_server', '~> 2.0', '>= 2.0.2'

debian/patches/0499-90-relax-webrick.patch

@@ -1,6 +1,6 @@
 --- a/Gemfile
 +++ b/Gemfile
-@@ -319,7 +319,7 @@
+@@ -321,7 +321,7 @@
  # Metrics
  group :metrics do
    gem 'method_source', '~> 1.0', require: false

debian/patches/0499-91-relax-gitlab-labkit.patch

@@ -2,7 +2,7 @@ gitaly needs gitlab-labkit ~> 0.15.0
 
 --- a/Gemfile
 +++ b/Gemfile
-@@ -297,7 +297,7 @@
+@@ -299,7 +299,7 @@
  gem 'premailer-rails', '~> 1.10', '>= 1.10.3'
  
  # LabKit: Tracing and Correlation

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

workhorse/CHANGELOG

@@ -1,5 +1,11 @@
 # Changelog for gitlab-workhorse
 
+## v8.63.3
+
+### Security
+- Check image content type before running exiftool in workhorse
+  https://gitlab.com/gitlab-org/gitlab-workhorse/-/merge_requests/
+
 ## v8.63.2
 
 ### Security

workhorse/VERSION

@@ -1 +1 @@
-8.63.2
+8.63.3

workhorse/go.mod

@@ -29,6 +29,7 @@ require (
 	gitlab.com/gitlab-org/gitaly v1.74.0
 	gitlab.com/gitlab-org/labkit v1.0.0
 	gocloud.dev v0.21.1-0.20201223184910-5094f54ed8bb
+	golang.org/x/image v0.0.0-20191009234506-e7c1f5e7dbb8
 	golang.org/x/lint v0.0.0-20200302205851-738671d3881b
 	golang.org/x/net v0.0.0-20201224014010-6772e930b67b
 	golang.org/x/sys v0.0.0-20210110051926-789bb1bd4061 // indirect

workhorse/internal/upload/exif/exif.go

@@ -22,6 +22,14 @@ type cleaner struct {
 	eof    bool
 }
 
+type FileType int
+
+const (
+	TypeUnknown FileType = iota
+	TypeJPEG
+	TypeTIFF
+)
+
 func NewCleaner(ctx context.Context, stdin io.Reader) (io.ReadCloser, error) {
 	c := &cleaner{ctx: ctx}
 
@@ -100,8 +108,16 @@ func (c *cleaner) startProcessing(stdin io.Reader) error {
 	return nil
 }
 
-func IsExifFile(filename string) bool {
+func FileTypeFromSuffix(filename string) FileType {
-	filenameMatch := regexp.MustCompile(`(?i)\.(jpg|jpeg|tiff)$`)
+	jpegMatch := regexp.MustCompile(`(?i)^[^\n]*\.(jpg|jpeg)$`)
-
+	if jpegMatch.MatchString(filename) {
-	return filenameMatch.MatchString(filename)
+		return TypeJPEG
+	}
+
+	tiffMatch := regexp.MustCompile(`(?i)^[^\n]*\.tiff$`)
+	if tiffMatch.MatchString(filename) {
+		return TypeTIFF
+	}
+
+	return TypeUnknown
 }

workhorse/internal/upload/exif/exif_test.go

@@ -11,39 +11,57 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestIsExifFile(t *testing.T) {
+func TestFileTypeFromSuffix(t *testing.T) {
 	tests := []struct {
 		name     string
-		expected bool
+		expected FileType
 	}{
 		{
 			name:     "/full/path.jpg",
-			expected: true,
+			expected: TypeJPEG,
 		},
 		{
 			name:     "path.jpeg",
-			expected: true,
+			expected: TypeJPEG,
 		},
 		{
 			name:     "path.tiff",
-			expected: true,
+			expected: TypeTIFF,
 		},
 		{
 			name:     "path.JPG",
-			expected: true,
+			expected: TypeJPEG,
 		},
 		{
 			name:     "path.tar",
-			expected: false,
+			expected: TypeUnknown,
 		},
 		{
 			name:     "path",
-			expected: false,
+			expected: TypeUnknown,
+		},
+		{
+			name:     "something.jpg.py",
+			expected: TypeUnknown,
+		},
+		{
+			name:     "something.py.jpg",
+			expected: TypeJPEG,
+		},
+		{
+			name: `something.jpg
+			.py`,
+			expected: TypeUnknown,
+		},
+		{
+			name: `something.something
+			.jpg`,
+			expected: TypeUnknown,
 		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			require.Equal(t, test.expected, IsExifFile(test.name))
+			require.Equal(t, test.expected, FileTypeFromSuffix(test.name))
 		})
 	}
 }

workhorse/internal/upload/exif/testdata/sample_exif_invalid.jpg

@@ -0,0 +1 @@
+invalid data

workhorse/internal/upload/rewrite.go

@@ -8,12 +8,15 @@ import (
 	"io/ioutil"
 	"mime/multipart"
 	"net/http"
+	"os"
 	"strings"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	"gitlab.com/gitlab-org/labkit/log"
 
+	"golang.org/x/image/tiff"
+
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/lsif_transformer/parser"
@@ -122,9 +125,11 @@ func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipa
 
 	var inputReader io.ReadCloser
 	var err error
+
+	imageType := exif.FileTypeFromSuffix(filename)
 	switch {
-	case exif.IsExifFile(filename):
+	case imageType != exif.TypeUnknown:
-		inputReader, err = handleExifUpload(ctx, p, filename)
+		inputReader, err = handleExifUpload(ctx, p, filename, imageType)
 		if err != nil {
 			return err
 		}
@@ -164,12 +169,48 @@ func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipa
 	return rew.filter.ProcessFile(ctx, name, fh, rew.writer)
 }
 
-func handleExifUpload(ctx context.Context, r io.Reader, filename string) (io.ReadCloser, error) {
+func handleExifUpload(ctx context.Context, r io.Reader, filename string, imageType exif.FileType) (io.ReadCloser, error) {
+	tmpfile, err := ioutil.TempFile("", "exifremove")
+	if err != nil {
+		return nil, err
+	}
+	go func() {
+		<-ctx.Done()
+		tmpfile.Close()
+	}()
+	if err := os.Remove(tmpfile.Name()); err != nil {
+		return nil, err
+	}
+
+	_, err = io.Copy(tmpfile, r)
+	if err != nil {
+		return nil, err
+	}
+
+	tmpfile.Seek(0, io.SeekStart)
+	isValidType := false
+	switch imageType {
+	case exif.TypeJPEG:
+		isValidType = isJPEG(tmpfile)
+	case exif.TypeTIFF:
+		isValidType = isTIFF(tmpfile)
+	}
+
+	tmpfile.Seek(0, io.SeekStart)
+	if !isValidType {
+		log.WithContextFields(ctx, log.Fields{
+			"filename":  filename,
+			"imageType": imageType,
+		}).Print("invalid content type, not running exiftool")
+
+		return tmpfile, nil
+	}
+
 	log.WithContextFields(ctx, log.Fields{
 		"filename": filename,
 	}).Print("running exiftool to remove any metadata")
 
-	cleaner, err := exif.NewCleaner(ctx, r)
+	cleaner, err := exif.NewCleaner(ctx, tmpfile)
 	if err != nil {
 		return nil, err
 	}
@@ -177,6 +218,29 @@ func handleExifUpload(ctx context.Context, r io.Reader, filename string) (io.Rea
 	return cleaner, nil
 }
 
+func isTIFF(r io.Reader) bool {
+	_, err := tiff.Decode(r)
+	if err == nil {
+		return true
+	}
+
+	if _, unsupported := err.(tiff.UnsupportedError); unsupported {
+		return true
+	}
+
+	return false
+}
+
+func isJPEG(r io.Reader) bool {
+	// Only the first 512 bytes are used to sniff the content type.
+	buf, err := ioutil.ReadAll(io.LimitReader(r, 512))
+	if err != nil {
+		return false
+	}
+
+	return http.DetectContentType(buf) == "image/jpeg"
+}
+
 func handleLsifUpload(ctx context.Context, reader io.Reader, tempPath, filename string, preauth *api.Response) (io.ReadCloser, error) {
 	parserConfig := parser.Config{
 		TempPath: tempPath,

workhorse/internal/upload/rewrite_test.go

@@ -0,0 +1,43 @@
+package upload
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestImageTypeRecongition(t *testing.T) {
+	tests := []struct {
+		filename string
+		isJPEG   bool
+		isTIFF   bool
+	}{
+		{
+			filename: "exif/testdata/sample_exif.jpg",
+			isJPEG:   true,
+			isTIFF:   false,
+		}, {
+			filename: "exif/testdata/sample_exif.tiff",
+			isJPEG:   false,
+			isTIFF:   true,
+		}, {
+			filename: "exif/testdata/sample_exif_corrupted.jpg",
+			isJPEG:   true,
+			isTIFF:   false,
+		}, {
+			filename: "exif/testdata/sample_exif_invalid.jpg",
+			isJPEG:   false,
+			isTIFF:   false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.filename, func(t *testing.T) {
+			input, err := os.Open(test.filename)
+			require.NoError(t, err)
+			require.Equal(t, test.isJPEG, isJPEG(input))
+			require.Equal(t, test.isTIFF, isTIFF(input))
+		})
+	}
+}

workhorse/internal/upload/uploads_test.go

@@ -358,26 +358,10 @@ func TestInvalidFileNames(t *testing.T) {
 }
 
 func TestUploadHandlerRemovingExif(t *testing.T) {
-	tempPath, err := ioutil.TempDir("", "uploads")
-	require.NoError(t, err)
-	defer os.RemoveAll(tempPath)
-
-	var buffer bytes.Buffer
-
 	content, err := ioutil.ReadFile("exif/testdata/sample_exif.jpg")
 	require.NoError(t, err)
 
-	writer := multipart.NewWriter(&buffer)
+	runUploadTest(t, content, "sample_exif.jpg", 200, func(w http.ResponseWriter, r *http.Request) {
-	file, err := writer.CreateFormFile("file", "test.jpg")
-	require.NoError(t, err)
-
-	_, err = file.Write(content)
-	require.NoError(t, err)
-
-	err = writer.Close()
-	require.NoError(t, err)
-
-	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`/url/path\z`), func(w http.ResponseWriter, r *http.Request) {
 		err := r.ParseMultipartForm(100000)
 		require.NoError(t, err)
 
@@ -389,30 +373,54 @@ func TestUploadHandlerRemovingExif(t *testing.T) {
 		w.WriteHeader(200)
 		fmt.Fprint(w, "RESPONSE")
 	})
-	defer ts.Close()
-
-	httpRequest, err := http.NewRequest("POST", ts.URL+"/url/path", &buffer)
-	require.NoError(t, err)
-
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
-	httpRequest = httpRequest.WithContext(ctx)
-	httpRequest.ContentLength = int64(buffer.Len())
-	httpRequest.Header.Set("Content-Type", writer.FormDataContentType())
-	response := httptest.NewRecorder()
-
-	handler := newProxy(ts.URL)
-	apiResponse := &api.Response{TempPath: tempPath}
-	preparer := &DefaultPreparer{}
-	opts, _, err := preparer.Prepare(apiResponse)
-	require.NoError(t, err)
-
-	HandleFileUploads(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts)
-	require.Equal(t, 200, response.Code)
 }
 
-func TestUploadHandlerRemovingInvalidExif(t *testing.T) {
+func TestUploadHandlerRemovingExifTiff(t *testing.T) {
+	content, err := ioutil.ReadFile("exif/testdata/sample_exif.tiff")
+	require.NoError(t, err)
+
+	runUploadTest(t, content, "sample_exif.tiff", 200, func(w http.ResponseWriter, r *http.Request) {
+		err := r.ParseMultipartForm(100000)
+		require.NoError(t, err)
+
+		size, err := strconv.Atoi(r.FormValue("file.size"))
+		require.NoError(t, err)
+		require.True(t, size < len(content), "Expected the file to be smaller after removal of exif")
+		require.True(t, size > 0, "Expected to receive not empty file")
+
+		w.WriteHeader(200)
+		fmt.Fprint(w, "RESPONSE")
+	})
+}
+
+func TestUploadHandlerRemovingExifInvalidContentType(t *testing.T) {
+	content, err := ioutil.ReadFile("exif/testdata/sample_exif_invalid.jpg")
+	require.NoError(t, err)
+
+	runUploadTest(t, content, "sample_exif_invalid.jpg", 200, func(w http.ResponseWriter, r *http.Request) {
+		err := r.ParseMultipartForm(100000)
+		require.NoError(t, err)
+
+		output, err := ioutil.ReadFile(r.FormValue("file.path"))
+		require.NoError(t, err)
+		require.Equal(t, content, output, "Expected the file to be same as before")
+
+		w.WriteHeader(200)
+		fmt.Fprint(w, "RESPONSE")
+	})
+}
+
+func TestUploadHandlerRemovingExifCorruptedFile(t *testing.T) {
+	content, err := ioutil.ReadFile("exif/testdata/sample_exif_corrupted.jpg")
+	require.NoError(t, err)
+
+	runUploadTest(t, content, "sample_exif_corrupted.jpg", 422, func(w http.ResponseWriter, r *http.Request) {
+		err := r.ParseMultipartForm(100000)
+		require.Error(t, err)
+	})
+}
+
+func runUploadTest(t *testing.T, image []byte, filename string, httpCode int, tsHandler func(http.ResponseWriter, *http.Request)) {
 	tempPath, err := ioutil.TempDir("", "uploads")
 	require.NoError(t, err)
 	defer os.RemoveAll(tempPath)
@@ -420,17 +428,16 @@ func TestUploadHandlerRemovingInvalidExif(t *testing.T) {
 	var buffer bytes.Buffer
 
 	writer := multipart.NewWriter(&buffer)
-	file, err := writer.CreateFormFile("file", "test.jpg")
+	file, err := writer.CreateFormFile("file", filename)
+	require.NoError(t, err)
+
+	_, err = file.Write(image)
 	require.NoError(t, err)
 
-	fmt.Fprint(file, "this is not valid image data")
 	err = writer.Close()
 	require.NoError(t, err)
 
-	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`/url/path\z`), func(w http.ResponseWriter, r *http.Request) {
+	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`/url/path\z`), tsHandler)
-		err := r.ParseMultipartForm(100000)
-		require.Error(t, err)
-	})
 	defer ts.Close()
 
 	httpRequest, err := http.NewRequest("POST", ts.URL+"/url/path", &buffer)
@@ -451,7 +458,7 @@ func TestUploadHandlerRemovingInvalidExif(t *testing.T) {
 	require.NoError(t, err)
 
 	HandleFileUploads(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts)
-	require.Equal(t, 422, response.Code)
+	require.Equal(t, httpCode, response.Code)
 }
 
 func newProxy(url string) *proxy.Proxy {