Merge branch 'security-255886' into 'master'

Path traversal to RCE via LFS upload

See merge request gitlab-org/security/gitlab!980

app/uploaders/gitlab_uploader.rb

@@ -5,6 +5,10 @@ class GitlabUploader < CarrierWave::Uploader::Base
 
   class_attribute :options
 
+  PROTECTED_METHODS = %i(filename cache_dir work_dir store_dir).freeze
+
+  ObjectNotReadyError = Class.new(StandardError)
+
   class << self
     # DSL setter
     def storage_options(options)
@@ -33,6 +37,8 @@ class GitlabUploader < CarrierWave::Uploader::Base
 
   delegate :base_dir, :file_storage?, to: :class
 
+  before :cache, :protect_from_path_traversal!
+
   def initialize(model, mounted_as = nil, **uploader_context)
     super(model, mounted_as)
   end
@@ -121,6 +127,9 @@ class GitlabUploader < CarrierWave::Uploader::Base
   # For example, `FileUploader` builds the storage path based on the associated
   # project model's `path_with_namespace` value, which can change when the
   # project or its containing namespace is moved or renamed.
+  #
+  # When implementing this method, raise `ObjectNotReadyError` if the model
+  # does not yet exist, as it will be tested in `#protect_from_path_traversal!`
   def dynamic_segment
     raise(NotImplementedError)
   end
@@ -138,4 +147,21 @@ class GitlabUploader < CarrierWave::Uploader::Base
   def pathname
     @pathname ||= Pathname.new(path)
   end
+
+  # Protect against path traversal attacks
+  # This takes a list of methods to test for path traversal, e.g. ../../
+  # and checks each of them. This uses `.send` so that any potential errors
+  # don't block the entire set from being tested.
+  #
+  # @param [CarrierWave::SanitizedFile]
+  # @return [Nil]
+  # @raise [Gitlab::Utils::PathTraversalAttackError]
+  def protect_from_path_traversal!(file)
+    PROTECTED_METHODS.each do |method|
+      Gitlab::Utils.check_path_traversal!(self.send(method)) # rubocop: disable GitlabSecurity/PublicSend
+
+    rescue ObjectNotReadyError
+      # Do nothing. This test was attempted before the file was ready for that method
+    end
+  end
 end

app/uploaders/job_artifact_uploader.rb

@@ -4,7 +4,6 @@ class JobArtifactUploader < GitlabUploader
   extend Workhorse::UploadPath
   include ObjectStorage::Concern
 
-  ObjectNotReadyError = Class.new(StandardError)
   UnknownFileLocationError = Class.new(StandardError)
 
   storage_options Gitlab.config.artifacts
@@ -24,7 +23,9 @@ class JobArtifactUploader < GitlabUploader
   private
 
   def dynamic_segment
-    raise ObjectNotReadyError, 'JobArtifact is not ready' unless model.id
+    # This now tests model.created_at because it can for some reason be nil in the test suite,
+    # and it's not clear if this is intentional or not
+    raise ObjectNotReadyError, 'JobArtifact is not ready' unless model.id && model.created_at
 
     if model.hashed_path?
       hashed_path

app/uploaders/packages/package_file_uploader.rb

@@ -20,6 +20,8 @@ class Packages::PackageFileUploader < GitlabUploader
   private
 
   def dynamic_segment
+    raise ObjectNotReadyError, "Package model not ready" unless model.id
+
     Gitlab::HashedPath.new('packages', model.package.id, 'files', model.id, root_hash: model.package.project_id)
   end
 end

changelogs/unreleased/security-255886.yml

+---
+title: Path traversal to RCE via LFS upload
+merge_request:
+author:
+type: security

lib/gitlab/utils.rb

@@ -10,6 +10,8 @@ module Gitlab
     # Also see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/24223#note_284122580
     # It also checks for ALT_SEPARATOR aka '\' (forward slash)
     def check_path_traversal!(path)
+      return unless path.is_a?(String)
+
       path = decode_path(path)
       path_regex = /(\A(\.{1,2})\z|\A\.\.[\/\\]|[\/\\]\.\.\z|[\/\\]\.\.[\/\\]|\n)/
 

spec/lib/gitlab/utils_spec.rb

@@ -52,6 +52,10 @@ RSpec.describe Gitlab::Utils do
       expect(check_path_traversal!('dir/..foo.rb')).to eq('dir/..foo.rb')
       expect(check_path_traversal!('dir/.foo.rb')).to eq('dir/.foo.rb')
     end
+
+    it 'does nothing for a non-string' do
+      expect(check_path_traversal!(nil)).to be_nil
+    end
   end
 
   describe '.allowlisted?' do

spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb

@@ -14,6 +14,7 @@ end
 
 RSpec.shared_examples "builds correct paths" do |**patterns|
   let(:patterns) { patterns }
+  let(:fixture) { File.join('spec', 'fixtures', 'rails_sample.jpg') }
 
   before do
     allow(subject).to receive(:filename).and_return('<filename>')
@@ -55,4 +56,15 @@ RSpec.shared_examples "builds correct paths" do |**patterns|
       let(:target) { subject.class }
     end
   end
+
+  describe "path traversal exploits" do
+    before do
+      allow(subject).to receive(:filename).and_return("3bc58d54542d6a5efffa9a87554faac0254f73f675b337899ea869f6d38b7371/122../../../../../../../../.ssh/authorized_keys")
+    end
+
+    it "throws an exception" do
+      expect { subject.cache!(fixture_file_upload(fixture)) }.to raise_error(Gitlab::Utils::PathTraversalAttackError)
+      expect { subject.store!(fixture_file_upload(fixture)) }.to raise_error(Gitlab::Utils::PathTraversalAttackError)
+    end
+  end
 end

spec/uploaders/import_export_uploader_spec.rb

@@ -24,9 +24,14 @@ RSpec.describe ImportExportUploader do
 
     include_context 'with storage', described_class::Store::REMOTE
 
-    it_behaves_like 'builds correct paths',
-                    store_dir: %r[import_export_upload/import_file/],
-                    upload_path: %r[import_export_upload/import_file/]
+    patterns = {
+      store_dir: %r[import_export_upload/import_file/],
+      upload_path: %r[import_export_upload/import_file/]
+    }
+
+    it_behaves_like 'builds correct paths', patterns do
+      let(:fixture) { File.join('spec', 'fixtures', 'group_export.tar.gz') }
+    end
 
     describe '#move_to_store' do
       it 'returns false' do