diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-10-30 18:16:56 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-10-30 18:16:56 +0300 |
commit | fa2fec1d18330e4cd9803ff164db19e7367e3838 (patch) | |
tree | 91a9bf1c74eeff29690f33e3faf2b8ca87051af3 /spec | |
parent | 8ee0746f54c19fcb8fe81058594aa8d373c5b7d7 (diff) |
Add latest changes from gitlab-org/security/gitlab@13-5-stable-ee
Diffstat (limited to 'spec')
-rw-r--r-- | spec/features/file_uploads/multipart_invalid_uploads_spec.rb | 52 | ||||
-rw-r--r-- | spec/lib/gitlab/middleware/multipart_with_handler_for_jwt_params_spec.rb | 41 | ||||
-rw-r--r-- | spec/lib/gitlab/middleware/multipart_with_handler_spec.rb | 52 | ||||
-rw-r--r-- | spec/lib/gitlab/regex_spec.rb | 13 | ||||
-rw-r--r-- | spec/lib/gitlab/utils_spec.rb | 31 | ||||
-rw-r--r-- | spec/requests/api/internal/kubernetes_spec.rb | 12 | ||||
-rw-r--r-- | spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb | 12 | ||||
-rw-r--r-- | spec/uploaders/import_export_uploader_spec.rb | 11 |
8 files changed, 211 insertions, 13 deletions
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..e9e24c12af1 --- /dev/null +++ b/spec/features/file_uploads/multipart_invalid_uploads_spec.rb @@ -0,0 +1,52 @@ +# 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 'rejecting invalid keys' do |key_name:, message: nil| + 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(message.presence || "invalid field: \"#{key_name}\"") } + end + end + + RSpec.shared_examples 'by rejecting uploads with an invalid key' do + 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]]' + end + + # These keys are rejected directly by rack itself. + # The request will not be received by multipart.rb (can't use the 'handling file uploads' shared example) + it_behaves_like 'rejecting invalid keys', key_name: 'x' * 11000, message: 'Puma caught this error: exceeded available parameter key space (RangeError)' + it_behaves_like 'rejecting invalid keys', key_name: 'package[]test', message: 'Puma caught this error: expected Hash (got Array)' + + 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 1c56e489a94..66ed80a7d61 100644 --- a/spec/lib/gitlab/regex_spec.rb +++ b/spec/lib/gitlab/regex_spec.rb @@ -137,11 +137,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 |