Exif metadata not stripped when uploading image attachments via Emails

Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/239343#note_827572693
Changelog: security

lib/gitlab/email/attachment_uploader.rb

@@ -15,7 +15,9 @@ module Gitlab
         filter_signature_attachments(message).each do |attachment|
           tmp = Tempfile.new("gitlab-email-attachment")
           begin
-            File.open(tmp.path, "w+b") { |f| f.write attachment.body.decoded }
+            content = attachment.body.decoded
+            File.open(tmp.path, "w+b") { |f| f.write content }
+            sanitize_exif_if_needed(content, tmp.path)
 
             file = {
               tempfile:     tmp,
@@ -55,6 +57,12 @@ module Gitlab
       def normalize_mime(content_type)
         MIME::Type.simplified(content_type, remove_x_prefix: true)
       end
+
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/239343
+      def sanitize_exif_if_needed(content, path)
+        exif_sanitizer = Gitlab::Sanitizers::Exif.new
+        exif_sanitizer.clean_existing_path(path, content: content, skip_unallowed_types: true)
+      end
     end
   end
 end

lib/gitlab/sanitizers/exif.rb

@@ -97,6 +97,28 @@ module Gitlab
         end
       end
 
+      def clean_existing_path(src_path, dry_run: false, content: nil, skip_unallowed_types: false)
+        content ||= File.read(src_path)
+
+        if skip_unallowed_types
+          return unless check_for_allowed_types(content, raise_error: false)
+        else
+          check_for_allowed_types(content)
+        end
+
+        to_remove = extra_tags(src_path)
+
+        if to_remove.empty?
+          logger.info "#{src_path}: only whitelisted tags present, skipping"
+          return
+        end
+
+        logger.info "#{src_path}: found exif tags to remove: #{to_remove}"
+        return if dry_run
+
+        exec_remove_exif!(src_path)
+      end
+
       private
 
       def extra_tags(path)
@@ -146,12 +168,15 @@ module Gitlab
         filename
       end
 
-      def check_for_allowed_types(contents)
+      def check_for_allowed_types(contents, raise_error: true)
         mime_type = Gitlab::Utils::MimeType.from_string(contents)
 
-        unless ALLOWED_MIME_TYPES.include?(mime_type)
+        allowed = ALLOWED_MIME_TYPES.include?(mime_type)
+        if !allowed && raise_error
           raise "File type #{mime_type} not supported. Only supports #{ALLOWED_MIME_TYPES.join(", ")}."
         end
+
+        allowed
       end
 
       def upload_ref(upload)

spec/lib/gitlab/email/attachment_uploader_spec.rb

@@ -8,7 +8,27 @@ RSpec.describe Gitlab::Email::AttachmentUploader do
     let(:message_raw) { fixture_file("emails/attachment.eml") }
     let(:message) { Mail::Message.new(message_raw) }
 
+    before do
+      allow_next_instance_of(Gitlab::Sanitizers::Exif) do |instance|
+        allow(instance).to receive(:clean_existing_path).and_call_original
+      end
+    end
+
+    def expect_exif_sanitizer_called
+      expect_next_instance_of(Gitlab::Sanitizers::Exif) do |sanitizer|
+        expect(sanitizer).to receive(:clean_existing_path) do |path, **options|
+          expect(File.exist?(path)).to be true
+
+          file = File.open(path, "rb")
+          expect(options).to eql(content: file.read, skip_unallowed_types: true)
+          file.close
+        end
+      end
+    end
+
     it "uploads all attachments and returns their links" do
+      expect_exif_sanitizer_called
+
       links = described_class.new(message).execute(upload_parent: project, uploader_class: FileUploader)
       link = links.first
 
@@ -21,6 +41,8 @@ RSpec.describe Gitlab::Email::AttachmentUploader do
       let(:message_raw) { fixture_file("emails/valid_reply_signed_smime.eml") }
 
       it 'uploads all attachments except the signature' do
+        expect_exif_sanitizer_called
+
         links = described_class.new(message).execute(upload_parent: project, uploader_class: FileUploader)
 
         expect(links).not_to include(a_hash_including(alt: 'smime.p7s'))
@@ -36,6 +58,8 @@ RSpec.describe Gitlab::Email::AttachmentUploader do
       let(:message_raw) { fixture_file("emails/valid_reply_signed_smime_mixed_protocol_prefix.eml") }
 
       it 'uploads all attachments except the signature' do
+        expect_exif_sanitizer_called
+
         links = described_class.new(message).execute(upload_parent: project, uploader_class: FileUploader)
 
         expect(links).not_to include(a_hash_including(alt: 'smime.p7s'))

spec/lib/gitlab/sanitizers/exif_spec.rb

@@ -131,6 +131,124 @@ RSpec.describe Gitlab::Sanitizers::Exif do
     end
   end
 
+  describe '#clean_existing_path' do
+    let(:dry_run) { false }
+
+    let(:tmp_file) { Tempfile.new("rails_sample.jpg") }
+
+    subject { sanitizer.clean_existing_path(tmp_file.path, dry_run: dry_run) }
+
+    context "no dry run" do
+      let(:file_content) { fixture_file_upload('spec/fixtures/rails_sample.jpg') }
+
+      before do
+        File.open(tmp_file.path, "w+b") { |f| f.write file_content }
+      end
+
+      it "removes exif from the image" do
+        expected_args = ["exiftool", "-all=", "-tagsFromFile", "@", *Gitlab::Sanitizers::Exif::EXCLUDE_PARAMS, "--IPTC:all", "--XMP-iptcExt:all", kind_of(String)]
+
+        expect(sanitizer).to receive(:extra_tags).and_return(["", 0])
+        expect(sanitizer).to receive(:exec_remove_exif!).once.and_call_original
+        expect(Gitlab::Popen).to receive(:popen).with(expected_args) do |args|
+          File.write("#{args.last}_original", "foo") if args.last.start_with?(Dir.tmpdir)
+
+          [expected_args, 0]
+        end
+
+        subject
+      end
+
+      it "ignores image without exif" do
+        expected_args = ["exiftool", "-all", "-j", "-sort", "--IPTC:all", "--XMP-iptcExt:all", kind_of(String)]
+
+        expect(Gitlab::Popen).to receive(:popen).with(expected_args).and_return(["[{}]", 0])
+        expect(sanitizer).not_to receive(:exec_remove_exif!)
+
+        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 { subject }.to raise_exception(RuntimeError, "failed to get exif tags: error")
+      end
+
+      context 'for files that do not have the correct MIME type from file' 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, %r{File type text/plain not supported})
+        end
+      end
+
+      context 'skip_unallowed_types is false' do
+        context 'for files that do not have the correct MIME type from input content' do
+          let(:mime_type) { 'text/plain' }
+
+          it 'raises an error if not jpg/tiff images with the correct mime types' do
+            expect(sanitizer).not_to receive(:extra_tags)
+
+            expect do
+              sanitizer.clean_existing_path(tmp_file.path, content: file_content)
+            end.to raise_error(RuntimeError, %r{File type text/plain not supported})
+          end
+        end
+
+        context 'for files that do not have the correct MIME type from input content' do
+          let(:mime_type) { 'text/plain' }
+
+          it 'raises an error if not jpg/tiff images with the correct mime types' do
+            expect(sanitizer).not_to receive(:extra_tags)
+
+            expect do
+              sanitizer.clean_existing_path(tmp_file.path, content: file_content)
+            end.to raise_error(RuntimeError, %r{File type text/plain not supported})
+          end
+        end
+      end
+
+      context 'skip_unallowed_types is true' do
+        context 'for files that do not have the correct MIME type from input content' 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 do
+              sanitizer.clean_existing_path(tmp_file.path, content: file_content, skip_unallowed_types: true)
+            end.not_to raise_error
+          end
+        end
+
+        context 'for files that do not have the correct MIME type from input content' 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 do
+              sanitizer.clean_existing_path(tmp_file.path, content: file_content, skip_unallowed_types: true)
+            end.not_to raise_error
+          end
+        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!)
+
+        subject
+      end
+    end
+  end
+
   describe "#extra_tags" do
     it "returns a list of keys for exif file" do
       tags = '[{