diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2023-05-22 21:44:12 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2023-05-22 21:44:12 +0300 |
commit | 34d6370bacdf1849de3618f23faaa9de76612e31 (patch) | |
tree | 1420e018fea4068504d059141212502e6f9ac040 | |
parent | 727b75d3e8ecb88c70edbb017f29a8da302656c0 (diff) |
Add latest changes from gitlab-org/security/gitlab@16-0-stable-eev16.0.1
-rw-r--r-- | CHANGELOG.md | 6 | ||||
-rw-r--r-- | GITALY_SERVER_VERSION | 2 | ||||
-rw-r--r-- | GITLAB_PAGES_VERSION | 2 | ||||
-rw-r--r-- | app/controllers/concerns/uploads_actions.rb | 6 | ||||
-rw-r--r-- | app/uploaders/object_storage.rb | 2 | ||||
-rw-r--r-- | spec/requests/groups/uploads_controller_spec.rb | 14 | ||||
-rw-r--r-- | spec/requests/projects/uploads_controller_spec.rb | 14 | ||||
-rw-r--r-- | spec/requests/uploads_controller_spec.rb | 17 | ||||
-rw-r--r-- | spec/support/shared_examples/requests/uploads_actions_shared_examples.rb | 25 |
9 files changed, 86 insertions, 2 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 70a33628b03..785f22815ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 16.0.1 (2023-05-22) + +### Security (1 change) + +- [Fix arbitary file read via filename param](gitlab-org/security/gitlab@2ddbf5464954addce7b8c82102377f0f137b604f) ([merge request](gitlab-org/security/gitlab!3265)) + ## 16.0.0 (2023-05-18) ### Added (168 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 0e94e314c86..ab54c33c763 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -16.0.0
\ No newline at end of file +16.0.1
\ No newline at end of file diff --git a/GITLAB_PAGES_VERSION b/GITLAB_PAGES_VERSION index 0e94e314c86..ab54c33c763 100644 --- a/GITLAB_PAGES_VERSION +++ b/GITLAB_PAGES_VERSION @@ -1 +1 @@ -16.0.0
\ No newline at end of file +16.0.1
\ No newline at end of file diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index db756ae336f..0d64a685065 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -10,6 +10,10 @@ module UploadsActions included do prepend_before_action :set_request_format_from_path_extension rescue_from FileUploader::InvalidSecret, with: :render_404 + + rescue_from ::Gitlab::Utils::PathTraversalAttackError do + head :bad_request + end end def create @@ -33,6 +37,8 @@ module UploadsActions # - or redirect to its URL # def show + Gitlab::Utils.check_path_traversal!(params[:filename]) + return render_404 unless uploader&.exists? ttl, directives = *cache_settings diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 4188e0caa8e..0a30f0e99f7 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -410,6 +410,8 @@ module ObjectStorage end def retrieve_from_store!(identifier) + Gitlab::Utils.check_path_traversal!(identifier) + # We need to force assign the value of @filename so that we will still # get the original_filename in cases wherein the file points to a random generated # path format. This happens for direct uploaded files to final location. diff --git a/spec/requests/groups/uploads_controller_spec.rb b/spec/requests/groups/uploads_controller_spec.rb new file mode 100644 index 00000000000..8a9e3edbe20 --- /dev/null +++ b/spec/requests/groups/uploads_controller_spec.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Groups::UploadsController, feature_category: :shared do + include WorkhorseHelpers + + it_behaves_like 'uploads actions' do + let_it_be(:model) { create(:group, :public) } + let_it_be(:upload) { create(:upload, :namespace_upload, :with_file, model: model) } + + let(:show_path) { show_group_uploads_path(model, upload.secret, File.basename(upload.path)) } + end +end diff --git a/spec/requests/projects/uploads_controller_spec.rb b/spec/requests/projects/uploads_controller_spec.rb new file mode 100644 index 00000000000..03b53143c84 --- /dev/null +++ b/spec/requests/projects/uploads_controller_spec.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::UploadsController, feature_category: :shared do + include WorkhorseHelpers + + it_behaves_like 'uploads actions' do + let_it_be(:model) { create(:project, :public) } + let_it_be(:upload) { create(:upload, :issuable_upload, :with_file, model: model) } + + let(:show_path) { show_project_uploads_path(model, upload.secret, File.basename(upload.path)) } + end +end diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb new file mode 100644 index 00000000000..1e6999982a8 --- /dev/null +++ b/spec/requests/uploads_controller_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe UploadsController, feature_category: :shared do + include WorkhorseHelpers + + it_behaves_like 'uploads actions' do + let_it_be(:model) { create(:personal_snippet, :public) } + let_it_be(:upload) { create(:upload, :personal_snippet_upload, :with_file, model: model) } + + # See config/routes/uploads.rb + let(:show_path) do + "/uploads/-/system/#{model.model_name.singular}/#{model.to_param}/#{upload.secret}/#{File.basename(upload.path)}" + end + end +end diff --git a/spec/support/shared_examples/requests/uploads_actions_shared_examples.rb b/spec/support/shared_examples/requests/uploads_actions_shared_examples.rb new file mode 100644 index 00000000000..41613fa9871 --- /dev/null +++ b/spec/support/shared_examples/requests/uploads_actions_shared_examples.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'uploads actions' do + describe "GET #show" do + context 'with file traversal in filename parameter' do + # Uploads in tests are stored in directories like: + # tmp/tests/public/uploads/@hashed/AB/CD/ABCD/SECRET + let(:filename) { "../../../../../../../../../Gemfile.lock" } + let(:escaped_filename) { CGI.escape filename } + + it 'responds with status 400' do + # Check files do indeed exists + upload_absolute_path = Pathname(upload.absolute_path) + expect(upload_absolute_path).to be_exist + attacked_file_path = upload_absolute_path.dirname.join(filename) + expect(attacked_file_path).to be_exist + + # Need to escape, otherwise we get `ActionController::UrlGenerationError Exception: No route matches` + get show_path.sub(File.basename(upload.path), escaped_filename) + + expect(response).to have_gitlab_http_status(:bad_request) + end + end + end +end |