Welcome to mirror list, hosted at ThFree Co, Russian Federation.

gitlab.com/gitlab-org/gitlab-foss.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2020-10-30 18:14:31 +0300
committerGitLab Bot <gitlab-bot@gitlab.com>2020-10-30 18:14:31 +0300
commit5c0f71e2673e6ecd0c2e2d871c2d0aaba81b448a (patch)
tree5dda274b856ec45e6cb2e99024d44a17b5b094ed
parent38a98f4d88ec8d647391f0be752e4fe3c413d94d (diff)
Add latest changes from gitlab-org/security/gitlab@13-4-stable-ee
-rw-r--r--app/services/projects/transfer_service.rb12
-rw-r--r--app/uploaders/gitlab_uploader.rb26
-rw-r--r--app/uploaders/job_artifact_uploader.rb5
-rw-r--r--app/uploaders/packages/package_file_uploader.rb2
-rw-r--r--changelogs/unreleased/security-10io-multipart-check-brackets-balance-in-param-names.yml5
-rw-r--r--changelogs/unreleased/security-255886.yml5
-rw-r--r--changelogs/unreleased/security-container-regex-backtrack.yml5
-rw-r--r--changelogs/unreleased/security-kubernetes-agent-internal-api.yml5
-rw-r--r--lib/api/internal/kubernetes.rb2
-rw-r--r--lib/gitlab/middleware/multipart.rb16
-rw-r--r--lib/gitlab/regex.rb2
-rw-r--r--lib/gitlab/utils.rb30
-rw-r--r--spec/features/file_uploads/multipart_invalid_uploads_spec.rb49
-rw-r--r--spec/lib/gitlab/middleware/multipart_with_handler_for_jwt_params_spec.rb41
-rw-r--r--spec/lib/gitlab/middleware/multipart_with_handler_spec.rb52
-rw-r--r--spec/lib/gitlab/regex_spec.rb13
-rw-r--r--spec/lib/gitlab/utils_spec.rb31
-rw-r--r--spec/requests/api/internal/kubernetes_spec.rb12
-rw-r--r--spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb12
-rw-r--r--spec/uploaders/import_export_uploader_spec.rb11
20 files changed, 314 insertions, 22 deletions
diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb
index dba5177718d..ca02cc03bfd 100644
--- a/app/services/projects/transfer_service.rb
+++ b/app/services/projects/transfer_service.rb
@@ -79,11 +79,7 @@ module Projects
# Directories on disk
move_project_folders(project)
- # Move missing group labels to project
- Labels::TransferService.new(current_user, @old_group, project).execute
-
- # Move missing group milestones
- Milestones::TransferService.new(current_user, @old_group, project).execute
+ transfer_missing_group_resources(@old_group)
# Move uploads
move_project_uploads(project)
@@ -103,6 +99,12 @@ module Projects
refresh_permissions
end
+ def transfer_missing_group_resources(group)
+ Labels::TransferService.new(current_user, group, project).execute
+
+ Milestones::TransferService.new(current_user, group, project).execute
+ end
+
def allowed_transfer?(current_user, project)
@new_namespace &&
can?(current_user, :change_namespace, project) &&
diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb
index 654bb15378c..411d8b2614f 100644
--- a/app/uploaders/gitlab_uploader.rb
+++ b/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
diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb
index 47976c909e8..83dc1030606 100644
--- a/app/uploaders/job_artifact_uploader.rb
+++ b/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
diff --git a/app/uploaders/packages/package_file_uploader.rb b/app/uploaders/packages/package_file_uploader.rb
index 28545b9fcdf..61fbe2b4c49 100644
--- a/app/uploaders/packages/package_file_uploader.rb
+++ b/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
diff --git a/changelogs/unreleased/security-10io-multipart-check-brackets-balance-in-param-names.yml b/changelogs/unreleased/security-10io-multipart-check-brackets-balance-in-param-names.yml
new file mode 100644
index 00000000000..e0d1a4e535d
--- /dev/null
+++ b/changelogs/unreleased/security-10io-multipart-check-brackets-balance-in-param-names.yml
@@ -0,0 +1,5 @@
+---
+title: Validate each upload param key in multipart.rb
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-255886.yml b/changelogs/unreleased/security-255886.yml
new file mode 100644
index 00000000000..8fe8da59444
--- /dev/null
+++ b/changelogs/unreleased/security-255886.yml
@@ -0,0 +1,5 @@
+---
+title: Path traversal to RCE via LFS upload
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-container-regex-backtrack.yml b/changelogs/unreleased/security-container-regex-backtrack.yml
new file mode 100644
index 00000000000..c88fd526d47
--- /dev/null
+++ b/changelogs/unreleased/security-container-regex-backtrack.yml
@@ -0,0 +1,5 @@
+---
+title: Update container_repository_name_regex to prevent catastrophic backtracking
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-kubernetes-agent-internal-api.yml b/changelogs/unreleased/security-kubernetes-agent-internal-api.yml
new file mode 100644
index 00000000000..9ed192e90cc
--- /dev/null
+++ b/changelogs/unreleased/security-kubernetes-agent-internal-api.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent private repo from being accessed via internal Kubernetes API
+merge_request:
+author:
+type: security
diff --git a/lib/api/internal/kubernetes.rb b/lib/api/internal/kubernetes.rb
index 6d5dfd086e7..2e14a7783e5 100644
--- a/lib/api/internal/kubernetes.rb
+++ b/lib/api/internal/kubernetes.rb
@@ -85,7 +85,7 @@ module API
# TODO sort out authorization for real
# https://gitlab.com/gitlab-org/gitlab/-/issues/220912
- if !project || !project.public?
+ unless Ability.allowed?(nil, :download_code, project)
not_found!
end
diff --git a/lib/gitlab/middleware/multipart.rb b/lib/gitlab/middleware/multipart.rb
index 8e6ac7610f2..e637c0ce304 100644
--- a/lib/gitlab/middleware/multipart.rb
+++ b/lib/gitlab/middleware/multipart.rb
@@ -31,6 +31,7 @@ module Gitlab
RACK_ENV_KEY = 'HTTP_GITLAB_WORKHORSE_MULTIPART_FIELDS'
JWT_PARAM_SUFFIX = '.gitlab-workhorse-upload'
JWT_PARAM_FIXED_KEY = 'upload'
+ REWRITTEN_FIELD_NAME_MAX_LENGTH = 10000.freeze
class Handler
def initialize(env, message)
@@ -41,6 +42,8 @@ module Gitlab
def with_open_files
@rewritten_fields.each do |field, tmp_path|
+ raise "invalid field: #{field.inspect}" unless valid_field_name?(field)
+
parsed_field = Rack::Utils.parse_nested_query(field)
raise "unexpected field: #{field.inspect}" unless parsed_field.count == 1
@@ -108,6 +111,17 @@ module Gitlab
private
+ def valid_field_name?(name)
+ # length validation
+ return false if name.size >= REWRITTEN_FIELD_NAME_MAX_LENGTH
+
+ # brackets validation
+ return false if name.include?('[]') || name.start_with?('[', ']')
+ return false unless ::Gitlab::Utils.valid_brackets?(name, allow_nested: false)
+
+ true
+ end
+
def package_allowed_paths
packages_config = ::Gitlab.config.packages
return [] unless allow_packages_storage_path?(packages_config)
@@ -140,6 +154,8 @@ module Gitlab
class HandlerForJWTParams < Handler
def with_open_files
@rewritten_fields.keys.each do |field|
+ raise "invalid field: #{field.inspect}" unless valid_field_name?(field)
+
parsed_field = Rack::Utils.parse_nested_query(field)
raise "unexpected field: #{field.inspect}" unless parsed_field.count == 1
diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb
index 8e23ac6aca5..df5e3163719 100644
--- a/lib/gitlab/regex.rb
+++ b/lib/gitlab/regex.rb
@@ -139,7 +139,7 @@ module Gitlab
# See https://github.com/docker/distribution/blob/master/reference/regexp.go.
#
def container_repository_name_regex
- @container_repository_regex ||= %r{\A[a-z0-9]+((?:[._/]|__|[-]{0,10})[a-z0-9]+)*\Z}
+ @container_repository_regex ||= %r{\A[a-z0-9]+(([._/]|__|-*)[a-z0-9])*\z}
end
##
diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb
index e2d93e7cd29..3df54e74b4f 100644
--- a/lib/gitlab/utils.rb
+++ b/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)/
@@ -208,5 +210,33 @@ module Gitlab
def stable_sort_by(list)
list.sort_by.with_index { |x, idx| [yield(x), idx] }
end
+
+ # Check for valid brackets (`[` and `]`) in a string using this aspects:
+ # * open brackets count == closed brackets count
+ # * (optionally) reject nested brackets via `allow_nested: false`
+ # * open / close brackets coherence, eg. ][[] -> invalid
+ def valid_brackets?(string = '', allow_nested: true)
+ # remove everything except brackets
+ brackets = string.remove(/[^\[\]]/)
+
+ return true if brackets.empty?
+ # balanced counts check
+ return false if brackets.size.odd?
+
+ unless allow_nested
+ # nested brackets check
+ return false if brackets.include?('[[') || brackets.include?(']]')
+ end
+
+ # open / close brackets coherence check
+ untrimmed = brackets
+ loop do
+ trimmed = untrimmed.gsub('[]', '')
+ return true if trimmed.empty?
+ return false if trimmed == untrimmed
+
+ untrimmed = trimmed
+ end
+ end
end
end
diff --git a/spec/features/file_uploads/multipart_invalid_uploads_spec.rb b/spec/features/file_uploads/multipart_invalid_uploads_spec.rb
new file mode 100644
index 00000000000..c7490f88db6
--- /dev/null
+++ b/spec/features/file_uploads/multipart_invalid_uploads_spec.rb
@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Invalid uploads that must be rejected', :api, :js do
+ include_context 'file upload requests helpers'
+
+ let_it_be(:project) { create(:project) }
+ let_it_be(:user) { create(:user, :admin) }
+ let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
+
+ context 'invalid upload key', :capybara_ignore_server_errors do
+ let(:api_path) { "/projects/#{project.id}/packages/nuget/" }
+ let(:url) { capybara_url(api(api_path)) }
+ let(:file) { fixture_file_upload('spec/fixtures/dk.png') }
+
+ subject do
+ HTTParty.put(
+ url,
+ basic_auth: { user: user.username, password: personal_access_token.token },
+ body: body
+ )
+ end
+
+ RSpec.shared_examples 'by rejecting uploads with an invalid key' do
+ RSpec.shared_examples 'rejecting invalid keys' do |key_name:|
+ context "with invalid key #{key_name}" do
+ let(:body) { { key_name => file, 'package[test][name]' => 'test' } }
+
+ it { expect { subject }.not_to change { Packages::Package.nuget.count } }
+
+ it { expect(subject.code).to eq(500) }
+
+ it { expect(subject.body).to include("invalid field: \"#{key_name}\"") }
+ end
+ end
+
+ it_behaves_like 'rejecting invalid keys', key_name: 'package[test'
+ it_behaves_like 'rejecting invalid keys', key_name: 'package[]test'
+ it_behaves_like 'rejecting invalid keys', key_name: '[]'
+ it_behaves_like 'rejecting invalid keys', key_name: '[package]test'
+ it_behaves_like 'rejecting invalid keys', key_name: 'package][test]]'
+ it_behaves_like 'rejecting invalid keys', key_name: 'package[test[nested]]'
+ it_behaves_like 'rejecting invalid keys', key_name: 'x' * 11000
+ end
+
+ it_behaves_like 'handling file uploads', 'by rejecting uploads with an invalid key'
+ end
+end
diff --git a/spec/lib/gitlab/middleware/multipart_with_handler_for_jwt_params_spec.rb b/spec/lib/gitlab/middleware/multipart_with_handler_for_jwt_params_spec.rb
index 875e3820011..a1e9ac6e425 100644
--- a/spec/lib/gitlab/middleware/multipart_with_handler_for_jwt_params_spec.rb
+++ b/spec/lib/gitlab/middleware/multipart_with_handler_for_jwt_params_spec.rb
@@ -123,15 +123,46 @@ RSpec.describe Gitlab::Middleware::Multipart do
end
end
- context 'with invalid key in parameters' do
+ context 'with an invalid upload key' do
include_context 'with one temporary file for multipart'
- let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) }
- let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'wrong_key', filename: filename, remote_id: remote_id) }
+ RSpec.shared_examples 'rejecting the invalid key' do |key_in_header:, key_in_upload_params:, error_message:|
+ let(:rewritten_fields) { rewritten_fields_hash(key_in_header => uploaded_filepath) }
+ let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: key_in_upload_params, filename: filename, remote_id: remote_id) }
- it 'raises an error' do
- expect { subject }.to raise_error(RuntimeError, 'Empty JWT param: file.gitlab-workhorse-upload')
+ it 'raises an error' do
+ expect { subject }.to raise_error(RuntimeError, error_message)
+ end
end
+
+ it_behaves_like 'rejecting the invalid key',
+ key_in_header: 'file',
+ key_in_upload_params: 'wrong_key',
+ error_message: 'Empty JWT param: file.gitlab-workhorse-upload'
+ it_behaves_like 'rejecting the invalid key',
+ key_in_header: 'user[avatar',
+ key_in_upload_params: 'user[avatar]',
+ error_message: 'invalid field: "user[avatar"'
+ it_behaves_like 'rejecting the invalid key',
+ key_in_header: '[user]avatar',
+ key_in_upload_params: 'user[avatar]',
+ error_message: 'invalid field: "[user]avatar"'
+ it_behaves_like 'rejecting the invalid key',
+ key_in_header: 'user[]avatar',
+ key_in_upload_params: 'user[avatar]',
+ error_message: 'invalid field: "user[]avatar"'
+ it_behaves_like 'rejecting the invalid key',
+ key_in_header: 'user[avatar[image[url]]]',
+ key_in_upload_params: 'user[avatar]',
+ error_message: 'invalid field: "user[avatar[image[url]]]"'
+ it_behaves_like 'rejecting the invalid key',
+ key_in_header: '[]',
+ key_in_upload_params: 'user[avatar]',
+ error_message: 'invalid field: "[]"'
+ it_behaves_like 'rejecting the invalid key',
+ key_in_header: 'x' * 11000,
+ key_in_upload_params: 'user[avatar]',
+ error_message: "invalid field: \"#{'x' * 11000}\""
end
context 'with a modified JWT payload' do
diff --git a/spec/lib/gitlab/middleware/multipart_with_handler_spec.rb b/spec/lib/gitlab/middleware/multipart_with_handler_spec.rb
index 742a5639ace..8c2af775574 100644
--- a/spec/lib/gitlab/middleware/multipart_with_handler_spec.rb
+++ b/spec/lib/gitlab/middleware/multipart_with_handler_spec.rb
@@ -139,6 +139,58 @@ RSpec.describe Gitlab::Middleware::Multipart do
subject
end
end
+
+ context 'with invalid key in header' do
+ include_context 'with one temporary file for multipart'
+
+ RSpec.shared_examples 'rejecting the invalid key' do |key_in_header:, key_in_upload_params:, error_message:|
+ let(:rewritten_fields) { rewritten_fields_hash(key_in_header => uploaded_filepath) }
+ let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: key_in_upload_params, filename: filename, remote_id: remote_id) }
+
+ it 'raises an error' do
+ expect { subject }.to raise_error(RuntimeError, error_message)
+ end
+ end
+
+ it_behaves_like 'rejecting the invalid key',
+ key_in_header: 'user[avatar',
+ key_in_upload_params: 'user[avatar]',
+ error_message: 'invalid field: "user[avatar"'
+ it_behaves_like 'rejecting the invalid key',
+ key_in_header: '[user]avatar',
+ key_in_upload_params: 'user[avatar]',
+ error_message: 'invalid field: "[user]avatar"'
+ it_behaves_like 'rejecting the invalid key',
+ key_in_header: 'user[]avatar',
+ key_in_upload_params: 'user[avatar]',
+ error_message: 'invalid field: "user[]avatar"'
+ it_behaves_like 'rejecting the invalid key',
+ key_in_header: 'user[avatar[image[url]]]',
+ key_in_upload_params: 'user[avatar]',
+ error_message: 'invalid field: "user[avatar[image[url]]]"'
+ it_behaves_like 'rejecting the invalid key',
+ key_in_header: '[]',
+ key_in_upload_params: 'user[avatar]',
+ error_message: 'invalid field: "[]"'
+ it_behaves_like 'rejecting the invalid key',
+ key_in_header: 'x' * 11000,
+ key_in_upload_params: 'user[avatar]',
+ error_message: "invalid field: \"#{'x' * 11000}\""
+ end
+
+ context 'with key with unbalanced brackets in header' do
+ include_context 'with one temporary file for multipart'
+
+ let(:invalid_key) { 'user[avatar' }
+ let(:rewritten_fields) { rewritten_fields_hash( invalid_key => uploaded_filepath) }
+ let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'user[avatar]', filename: filename, remote_id: remote_id) }
+
+ it 'builds no UploadedFile' do
+ expect(app).not_to receive(:call)
+
+ expect { subject }.to raise_error(RuntimeError, "invalid field: \"#{invalid_key}\"")
+ end
+ end
end
end
end
diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb
index 88c3315150b..b4ed4f37c3b 100644
--- a/spec/lib/gitlab/regex_spec.rb
+++ b/spec/lib/gitlab/regex_spec.rb
@@ -107,11 +107,16 @@ RSpec.describe Gitlab::Regex do
it { is_expected.to match('my/awesome/image-1') }
it { is_expected.to match('my/awesome/image.test') }
it { is_expected.to match('my/awesome/image--test') }
- # docker distribution allows for infinite `-`
- # https://github.com/docker/distribution/blob/master/reference/regexp.go#L13
- # but we have a range of 0,10 to add a reasonable limit.
- it { is_expected.not_to match('my/image-----------test') }
+ it { is_expected.to match('my/image__test') }
+ # this example tests for catastrophic backtracking
+ it { is_expected.to match('user1/project/a_bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb------------x') }
+ it { is_expected.not_to match('user1/project/a_bbbbb-------------') }
it { is_expected.not_to match('my/image-.test') }
+ it { is_expected.not_to match('my/image___test') }
+ it { is_expected.not_to match('my/image_.test') }
+ it { is_expected.not_to match('my/image_-test') }
+ it { is_expected.not_to match('my/image..test') }
+ it { is_expected.not_to match('my/image\ntest') }
it { is_expected.not_to match('.my/image') }
it { is_expected.not_to match('my/image.') }
end
diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb
index 1eaceec1d8a..36257a0605b 100644
--- a/spec/lib/gitlab/utils_spec.rb
+++ b/spec/lib/gitlab/utils_spec.rb
@@ -3,6 +3,8 @@
require 'spec_helper'
RSpec.describe Gitlab::Utils do
+ using RSpec::Parameterized::TableSyntax
+
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!, :allowlisted?, :check_allowed_absolute_path!, :decode_path, :ms_to_round_sec, to: :described_class
@@ -50,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
@@ -448,4 +454,29 @@ RSpec.describe Gitlab::Utils do
end
end
end
+
+ describe '.valid_brackets?' do
+ where(:input, :allow_nested, :valid) do
+ 'no brackets' | true | true
+ 'no brackets' | false | true
+ 'user[avatar]' | true | true
+ 'user[avatar]' | false | true
+ 'user[avatar][friends]' | true | true
+ 'user[avatar][friends]' | false | true
+ 'user[avatar[image[url]]]' | true | true
+ 'user[avatar[image[url]]]' | false | false
+ 'user[avatar[]friends]' | true | true
+ 'user[avatar[]friends]' | false | false
+ 'user[avatar]]' | true | false
+ 'user[avatar]]' | false | false
+ 'user][avatar]]' | true | false
+ 'user][avatar]]' | false | false
+ 'user[avatar' | true | false
+ 'user[avatar' | false | false
+ end
+
+ with_them do
+ it { expect(described_class.valid_brackets?(input, allow_nested: allow_nested)).to eq(valid) }
+ end
+ end
end
diff --git a/spec/requests/api/internal/kubernetes_spec.rb b/spec/requests/api/internal/kubernetes_spec.rb
index f669483b5a4..a532b8e59f2 100644
--- a/spec/requests/api/internal/kubernetes_spec.rb
+++ b/spec/requests/api/internal/kubernetes_spec.rb
@@ -166,6 +166,16 @@ RSpec.describe API::Internal::Kubernetes do
)
)
end
+
+ context 'repository is for project members only' do
+ let(:project) { create(:project, :public, :repository_private) }
+
+ it 'returns 404' do
+ send_request(params: { id: project.id }, headers: { 'Authorization' => "Bearer #{agent_token.token}" })
+
+ expect(response).to have_gitlab_http_status(:not_found)
+ end
+ end
end
context 'project is private' do
@@ -190,7 +200,7 @@ RSpec.describe API::Internal::Kubernetes do
context 'project does not exist' do
it 'returns 404' do
- send_request(params: { id: 0 }, headers: { 'Authorization' => "Bearer #{agent_token.token}" })
+ send_request(params: { id: non_existing_record_id }, headers: { 'Authorization' => "Bearer #{agent_token.token}" })
expect(response).to have_gitlab_http_status(:not_found)
end
diff --git a/spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb b/spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb
index 12bcbb8b812..7126d3ace96 100644
--- a/spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb
+++ b/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
diff --git a/spec/uploaders/import_export_uploader_spec.rb b/spec/uploaders/import_export_uploader_spec.rb
index b1fdcf067c6..cb7a89193e6 100644
--- a/spec/uploaders/import_export_uploader_spec.rb
+++ b/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