Merge branch 'jwanjohi_improve_path_traversal_check' into 'master'

Improve checks for path traversal validation

See merge request gitlab-org/gitlab!33114

changelogs/unreleased/jwanjohi_improve_path_traversal_check.yml

+---
+title: Improve path traversal validation checks
+merge_request: 33114
+author:
+type: security

doc/development/api_styleguide.md

@@ -173,7 +173,8 @@ guide on how you can add a new custom validator.
   validates the parameter value for different cases. Mainly, it checks whether a
   path is relative and does it contain `../../` relative traversal using
   `File::Separator` or not, and whether the path is absolute, for example
-  `/etc/passwd/`.
+  `/etc/passwd/`. By default, absolute paths are not allowed. However, you can optionally pass in an allowlist for allowed absolute paths in the following way:  
+  `requires :file_path, type: String, file_path: { allowlist: ['/foo/bar/', '/home/foo/', '/app/home'] }`
 
 - `Git SHA`:
 

lib/api/validations/validators/file_path.rb

@@ -5,10 +5,12 @@ module API
     module Validators
       class FilePath < Grape::Validations::Base
         def validate_param!(attr_name, params)
+          options = @option.is_a?(Hash) ? @option : {}
+          path_allowlist = options.fetch(:allowlist, [])
           path = params[attr_name]
-
-          Gitlab::Utils.check_path_traversal!(path)
-        rescue ::Gitlab::Utils::PathTraversalAttackError
+          path = Gitlab::Utils.check_path_traversal!(path)
+          Gitlab::Utils.check_allowed_absolute_path!(path, path_allowlist)
+        rescue
           raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)],
                                                message: "should be a valid file path"
         end

lib/gitlab/utils.rb

@@ -7,23 +7,43 @@ module Gitlab
 
     # Ensure that the relative path will not traverse outside the base directory
     # We url decode the path to avoid passing invalid paths forward in url encoded format.
-    # We are ok to pass some double encoded paths to File.open since they won't resolve.
     # 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, allowed_absolute: false)
-      path = CGI.unescape(path)
-
-      if path.start_with?("..#{File::SEPARATOR}", "..#{File::ALT_SEPARATOR}") ||
-          path.include?("#{File::SEPARATOR}..#{File::SEPARATOR}") ||
-          path.end_with?("#{File::SEPARATOR}..") ||
-          (!allowed_absolute && Pathname.new(path).absolute?)
+    def check_path_traversal!(path)
+      path = decode_path(path)
+      path_regex = /(\A(\.{1,2})\z|\A\.\.[\/\\]|[\/\\]\.\.\z|[\/\\]\.\.[\/\\]|\n)/
 
+      if path.match?(path_regex)
         raise PathTraversalAttackError.new('Invalid path')
       end
 
       path
     end
 
+    def allowlisted?(absolute_path, allowlist)
+      path = absolute_path.downcase
+
+      allowlist.map(&:downcase).any? do |allowed_path|
+        path.start_with?(allowed_path)
+      end
+    end
+
+    def check_allowed_absolute_path!(path, allowlist)
+      return unless Pathname.new(path).absolute?
+      return if allowlisted?(path, allowlist)
+
+      raise StandardError, "path #{path} is not allowed"
+    end
+
+    def decode_path(encoded_path)
+      decoded = CGI.unescape(encoded_path)
+      if decoded != CGI.unescape(decoded)
+        raise StandardError, "path #{encoded_path} is not allowed"
+      end
+
+      decoded
+    end
+
     def force_utf8(str)
       str.dup.force_encoding(Encoding::UTF_8)
     end

spec/lib/api/validations/validators/file_path_spec.rb

@@ -6,31 +6,64 @@ RSpec.describe API::Validations::Validators::FilePath do
   include ApiValidatorsHelpers
 
   subject do
-    described_class.new(['test'], {}, false, scope.new)
+    described_class.new(['test'], params, false, scope.new)
   end
 
-  context 'valid file path' do
-    it 'does not raise a validation error' do
-      expect_no_validation_error('test' => './foo')
-      expect_no_validation_error('test' => './bar.rb')
-      expect_no_validation_error('test' => 'foo%2Fbar%2Fnew%2Ffile.rb')
-      expect_no_validation_error('test' => 'foo%2Fbar%2Fnew')
-      expect_no_validation_error('test' => 'foo%252Fbar%252Fnew%252Ffile.rb')
+  context 'when allowlist is not set' do
+    shared_examples 'file validation' do
+      context 'valid file path' do
+        it 'does not raise a validation error' do
+          expect_no_validation_error('test' => './foo')
+          expect_no_validation_error('test' => './bar.rb')
+          expect_no_validation_error('test' => 'foo%2Fbar%2Fnew%2Ffile.rb')
+          expect_no_validation_error('test' => 'foo%2Fbar%2Fnew')
+          expect_no_validation_error('test' => 'foo/bar')
+        end
+      end
+
+      context 'invalid file path' do
+        it 'raise a validation error' do
+          expect_validation_error('test' => '../foo')
+          expect_validation_error('test' => '../')
+          expect_validation_error('test' => 'foo/../../bar')
+          expect_validation_error('test' => 'foo/../')
+          expect_validation_error('test' => 'foo/..')
+          expect_validation_error('test' => '../')
+          expect_validation_error('test' => '..\\')
+          expect_validation_error('test' => '..\/')
+          expect_validation_error('test' => '%2e%2e%2f')
+          expect_validation_error('test' => '/etc/passwd')
+          expect_validation_error('test' => 'test%0a/etc/passwd')
+          expect_validation_error('test' => '%2Ffoo%2Fbar%2Fnew%2Ffile.rb')
+          expect_validation_error('test' => '%252Ffoo%252Fbar%252Fnew%252Ffile.rb')
+          expect_validation_error('test' => 'foo%252Fbar%252Fnew%252Ffile.rb')
+          expect_validation_error('test' => 'foo%25252Fbar%25252Fnew%25252Ffile.rb')
+        end
+      end
+    end
+
+    it_behaves_like 'file validation' do
+      let(:params) { {} }
+    end
+
+    it_behaves_like 'file validation' do
+      let(:params) { true }
     end
   end
 
-  context 'invalid file path' do
-    it 'raise a validation error' do
-      expect_validation_error('test' => '../foo')
-      expect_validation_error('test' => '../')
-      expect_validation_error('test' => 'foo/../../bar')
-      expect_validation_error('test' => 'foo/../')
-      expect_validation_error('test' => 'foo/..')
-      expect_validation_error('test' => '../')
-      expect_validation_error('test' => '..\\')
-      expect_validation_error('test' => '..\/')
-      expect_validation_error('test' => '%2e%2e%2f')
-      expect_validation_error('test' => '/etc/passwd')
+  context 'when allowlist is set' do
+    let(:params) { { allowlist: ['/home/bar'] } }
+
+    context 'when file path is included in the allowlist' do
+      it 'does not raise a validation error' do
+        expect_no_validation_error('test' => '/home/bar')
+      end
+    end
+
+    context 'when file path is not included in the allowlist' do
+      it 'raises a validation error' do
+        expect_validation_error('test' => '/foo/xyz')
+      end
     end
   end
 end

spec/lib/gitlab/utils_spec.rb

@@ -5,39 +5,93 @@ require 'spec_helper'
 RSpec.describe Gitlab::Utils do
   delegate :to_boolean, :boolean_to_yes_no, :slugify, :random_string, :which,
            :ensure_array_from_string, :to_exclusive_sentence, :bytes_to_megabytes,
-           :append_path, :check_path_traversal!, :ms_to_round_sec, to: :described_class
+           :append_path, :check_path_traversal!, :allowlisted?, :check_allowed_absolute_path!, :decode_path, :ms_to_round_sec, to: :described_class
 
   describe '.check_path_traversal!' do
+    it 'detects path traversal in string without any separators' do
+      expect { check_path_traversal!('.') }.to raise_error(/Invalid path/)
+      expect { check_path_traversal!('..') }.to raise_error(/Invalid path/)
+    end
+
     it 'detects path traversal at the start of the string' do
       expect { check_path_traversal!('../foo') }.to raise_error(/Invalid path/)
+      expect { check_path_traversal!('..\\foo') }.to raise_error(/Invalid path/)
     end
 
     it 'detects path traversal at the start of the string, even to just the subdirectory' do
       expect { check_path_traversal!('../') }.to raise_error(/Invalid path/)
+      expect { check_path_traversal!('..\\') }.to raise_error(/Invalid path/)
+      expect { check_path_traversal!('/../') }.to raise_error(/Invalid path/)
+      expect { check_path_traversal!('\\..\\') }.to raise_error(/Invalid path/)
     end
 
     it 'detects path traversal in the middle of the string' do
       expect { check_path_traversal!('foo/../../bar') }.to raise_error(/Invalid path/)
+      expect { check_path_traversal!('foo\\..\\..\\bar') }.to raise_error(/Invalid path/)
+      expect { check_path_traversal!('foo/..\\bar') }.to raise_error(/Invalid path/)
+      expect { check_path_traversal!('foo\\../bar') }.to raise_error(/Invalid path/)
+      expect { check_path_traversal!('foo/..\\..\\..\\..\\../bar') }.to raise_error(/Invalid path/)
     end
 
     it 'detects path traversal at the end of the string when slash-terminates' do
       expect { check_path_traversal!('foo/../') }.to raise_error(/Invalid path/)
+      expect { check_path_traversal!('foo\\..\\') }.to raise_error(/Invalid path/)
     end
 
     it 'detects path traversal at the end of the string' do
       expect { check_path_traversal!('foo/..') }.to raise_error(/Invalid path/)
+      expect { check_path_traversal!('foo\\..') }.to raise_error(/Invalid path/)
     end
 
     it 'does nothing for a safe string' do
       expect(check_path_traversal!('./foo')).to eq('./foo')
+      expect(check_path_traversal!('.test/foo')).to eq('.test/foo')
+      expect(check_path_traversal!('..test/foo')).to eq('..test/foo')
+      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
+  end
+
+  describe '.allowlisted?' do
+    let(:allowed_paths) { ['/home/foo', '/foo/bar', '/etc/passwd']}
+
+    it 'returns true if path is allowed' do
+      expect(allowlisted?('/foo/bar', allowed_paths)).to be(true)
+    end
+
+    it 'returns false if path is not allowed' do
+      expect(allowlisted?('/test/test', allowed_paths)).to be(false)
+    end
+  end
+
+  describe '.check_allowed_absolute_path!' do
+    let(:allowed_paths) { ['/home/foo'] }
+
+    it 'raises an exception if an absolute path is not allowed' do
+      expect { check_allowed_absolute_path!('/etc/passwd', allowed_paths) }.to raise_error(StandardError)
     end
 
-    it 'does nothing if an absolute path is allowed' do
-      expect(check_path_traversal!('/etc/folder/path', allowed_absolute: true)). to eq('/etc/folder/path')
+    it 'does nothing for an allowed absolute path' do
+      expect(check_allowed_absolute_path!('/home/foo', allowed_paths)).to be_nil
     end
+  end
 
-    it 'raises exception if an absolute path is not allowed' do
-      expect { check_path_traversal!('/etc/folder/path') }.to raise_error(/Invalid path/)
+  describe '.decode_path' do
+    it 'returns path unencoded for singled-encoded paths' do
+      expect(decode_path('%2Fhome%2Fbar%3Fasd%3Dqwe')).to eq('/home/bar?asd=qwe')
+    end
+
+    it 'returns path when it is unencoded' do
+      expect(decode_path('/home/bar?asd=qwe')).to eq('/home/bar?asd=qwe')
+    end
+
+    [
+      '..%252F..%252F..%252Fetc%252Fpasswd',
+      '%25252Fresult%25252Fchosennickname%25253D%252522jj%252522'
+    ].each do |multiple_encoded_path|
+      it 'raises an exception when the path is multiple-encoded' do
+        expect { decode_path(multiple_encoded_path) }.to raise_error(/path #{multiple_encoded_path} is not allowed/)
+      end
     end
   end
 

spec/requests/api/files_spec.rb

@@ -10,6 +10,7 @@ RSpec.describe API::Files do
   let(:guest) { create(:user) { |u| project.add_guest(u) } }
   let(:file_path) { "files%2Fruby%2Fpopen%2Erb" }
   let(:rouge_file_path) { "%2e%2e%2f" }
+  let(:absolute_path) { "%2Fetc%2Fpasswd.rb" }
   let(:invalid_file_message) { 'file_path should be a valid file path' }
   let(:params) do
     {
@@ -57,6 +58,18 @@ RSpec.describe API::Files do
     end
   end
 
+  shared_examples 'when path is absolute' do
+    it 'returns 400 when file path is absolute' do
+      subject
+
+      expect(response).to have_gitlab_http_status(:bad_request)
+
+      if response.body.present?
+        expect(json_response['error']).to eq(invalid_file_message)
+      end
+    end
+  end
+
   describe "HEAD /projects/:id/repository/files/:file_path" do
     shared_examples_for 'repository files' do
       it 'returns 400 when file path is invalid' do
@@ -65,6 +78,10 @@ RSpec.describe API::Files do
         expect(response).to have_gitlab_http_status(:bad_request)
       end
 
+      it_behaves_like 'when path is absolute' do
+        subject { head api(route(absolute_path), current_user), params: params }
+      end
+
       it 'returns file attributes in headers' do
         head api(route(file_path), current_user), params: params
 
@@ -165,6 +182,10 @@ RSpec.describe API::Files do
         expect(json_response['error']).to eq(invalid_file_message)
       end
 
+      it_behaves_like 'when path is absolute' do
+        subject { get api(route(absolute_path), api_user), params: params }
+      end
+
       it 'returns file attributes as json' do
         get api(route(file_path), api_user), params: params
 
@@ -350,6 +371,10 @@ RSpec.describe API::Files do
         expect(json_response['error']).to eq(invalid_file_message)
       end
 
+      it_behaves_like 'when path is absolute' do
+        subject { get api(route(absolute_path) + '/blame', current_user), params: params }
+      end
+
       it 'returns blame file attributes as json' do
         get api(route(file_path) + '/blame', current_user), params: params
 
@@ -473,6 +498,10 @@ RSpec.describe API::Files do
         expect(json_response['error']).to eq(invalid_file_message)
       end
 
+      it_behaves_like 'when path is absolute' do
+        subject { get api(route(absolute_path) + '/raw', current_user), params: params }
+      end
+
       it 'returns raw file info' do
         url = route(file_path) + "/raw"
         expect(Gitlab::Workhorse).to receive(:send_git_blob)
@@ -597,6 +626,10 @@ RSpec.describe API::Files do
       expect(json_response['error']).to eq(invalid_file_message)
     end
 
+    it_behaves_like 'when path is absolute' do
+      subject { post api(route(absolute_path), user), params: params }
+    end
+
     it "creates a new file in project repo" do
       post api(route(file_path), user), params: params
 
@@ -735,6 +768,16 @@ RSpec.describe API::Files do
       expect(json_response['error']).to eq(invalid_file_message)
     end
 
+    it_behaves_like 'when path is absolute' do
+      let(:last_commit) do
+        Gitlab::Git::Commit
+        .last_for_path(project.repository, 'master', URI.unescape(file_path))
+      end
+      let(:params_with_correct_id) { params.merge(last_commit_id: last_commit.id) }
+
+      subject { put api(route(absolute_path), user), params: params_with_correct_id }
+    end
+
     it "returns a 400 bad request if no params given" do
       put api(route(file_path), user)
 
@@ -770,6 +813,10 @@ RSpec.describe API::Files do
       expect(json_response['error']).to eq(invalid_file_message)
     end
 
+    it_behaves_like 'when path is absolute' do
+      subject { delete api(route(absolute_path), user), params: params }
+    end
+
     it "deletes existing file in project repo" do
       delete api(route(file_path), user), params: params