From 041695fe74fd348151a79d9e46ca189b6f912a98 Mon Sep 17 00:00:00 2001 From: Pirate Praveen Date: Fri, 8 Jan 2021 16:13:35 +0530 Subject: [PATCH] New upstream version 13.5.6 --- CHANGELOG.md | 13 ++++++ GITALY_SERVER_VERSION | 2 +- GITLAB_PAGES_VERSION | 2 +- GITLAB_WORKHORSE_VERSION | 2 +- VERSION | 2 +- .../oauth/authorizations_controller.rb | 11 +++++ app/controllers/projects/raw_controller.rb | 2 +- .../projects/repositories_controller.rb | 2 +- ...823_update_trusted_apps_to_confidential.rb | 23 ++++++++++ db/schema_migrations/20201222151823 | 1 + db/structure.sql | 2 + lib/api/nuget_packages.rb | 2 +- lib/gitlab/regex.rb | 13 +++++- .../usage_data_counters/hll_redis_counter.rb | 10 ++--- .../oauth/authorizations_controller_spec.rb | 14 ++++++ .../projects/raw_controller_spec.rb | 19 ++++++++ .../projects/repositories_controller_spec.rb | 12 +++++ spec/lib/gitlab/regex_spec.rb | 6 +++ .../hll_redis_counter_spec.rb | 44 +++++++++++++++++++ spec/lib/gitlab/usage_data_spec.rb | 26 +++++++++++ ...ent_management_activity_shared_examples.rb | 4 +- 21 files changed, 196 insertions(+), 16 deletions(-) create mode 100644 db/migrate/20201222151823_update_trusted_apps_to_confidential.rb create mode 100644 db/schema_migrations/20201222151823 diff --git a/CHANGELOG.md b/CHANGELOG.md index 528bc17aa9..cbf4b28bd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,19 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 13.5.6 (2021-01-07) + +### Security (7 changes) + +- Forbid public cache for private repos. +- Deny implicit flow for confidential apps. +- Update NuGet regular expression to protect against ReDoS. +- Fix regular expression backtracking issue in package name validation. +- Upgrade GitLab Pages to 1.28.2. +- Update trusted OAuth applications to set them as confidential. +- Upgrade Workhorse to 8.51.2. + + ## 13.5.5 (2020-12-07) ### Security (10 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 4afeb76808..c50c58133e 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -13.5.5 \ No newline at end of file +13.5.6 \ No newline at end of file diff --git a/GITLAB_PAGES_VERSION b/GITLAB_PAGES_VERSION index cfc730712d..bf4df28efc 100644 --- a/GITLAB_PAGES_VERSION +++ b/GITLAB_PAGES_VERSION @@ -1 +1 @@ -1.28.0 +1.28.2 diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index f66efe3878..3c049ca13d 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -8.51.0 +8.51.2 diff --git a/VERSION b/VERSION index 4afeb76808..c50c58133e 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -13.5.5 \ No newline at end of file +13.5.6 \ No newline at end of file diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index 6e8686ee90..ade698baa7 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -24,6 +24,17 @@ class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController end end + def create + # Confidential apps require the client_secret to be sent with the request. + # Doorkeeper allows implicit grant flow requests (response_type=token) to + # work without client_secret regardless of the confidential setting. + if pre_auth.authorizable? && pre_auth.response_type == 'token' && pre_auth.client.application.confidential + render "doorkeeper/authorizations/error" + else + super + end + end + private def verify_confirmed_email! diff --git a/app/controllers/projects/raw_controller.rb b/app/controllers/projects/raw_controller.rb index a9490c106d..ffb8aca2a7 100644 --- a/app/controllers/projects/raw_controller.rb +++ b/app/controllers/projects/raw_controller.rb @@ -20,7 +20,7 @@ class Projects::RawController < Projects::ApplicationController def show @blob = @repository.blob_at(@commit.id, @path) - send_blob(@repository, @blob, inline: (params[:inline] != 'false'), allow_caching: @project.public?) + send_blob(@repository, @blob, inline: (params[:inline] != 'false'), allow_caching: Guest.can?(:download_code, @project)) end private diff --git a/app/controllers/projects/repositories_controller.rb b/app/controllers/projects/repositories_controller.rb index ba3ab52e3a..092ab4fcfd 100644 --- a/app/controllers/projects/repositories_controller.rb +++ b/app/controllers/projects/repositories_controller.rb @@ -51,7 +51,7 @@ class Projects::RepositoriesController < Projects::ApplicationController end def set_cache_headers - expires_in cache_max_age(archive_metadata['CommitId']), public: project.public? + expires_in cache_max_age(archive_metadata['CommitId']), public: Guest.can?(:download_code, project) fresh_when(etag: archive_metadata['ArchivePath']) end diff --git a/db/migrate/20201222151823_update_trusted_apps_to_confidential.rb b/db/migrate/20201222151823_update_trusted_apps_to_confidential.rb new file mode 100644 index 0000000000..bcb94c6512 --- /dev/null +++ b/db/migrate/20201222151823_update_trusted_apps_to_confidential.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +class UpdateTrustedAppsToConfidential < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + INDEX_NAME = 'tmp_index_oauth_applications_on_id_where_trusted' + + disable_ddl_transaction! + + def up + add_concurrent_index :oauth_applications, :id, where: 'trusted = true', name: INDEX_NAME + + execute('UPDATE oauth_applications SET confidential = true WHERE trusted = true') + end + + def down + # We won't be able to tell which trusted applications weren't confidential before the migration + # and setting all trusted applications are not confidential would introduce security issues + + remove_concurrent_index_by_name :oauth_applications, INDEX_NAME + end +end diff --git a/db/schema_migrations/20201222151823 b/db/schema_migrations/20201222151823 new file mode 100644 index 0000000000..914e96473a --- /dev/null +++ b/db/schema_migrations/20201222151823 @@ -0,0 +1 @@ +d3af120a74b4c55345ac7fb524395251cd3c1b3cd9685f711196a134f427845c \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 4e6fc7e926..c5cab19b8d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -21962,6 +21962,8 @@ CREATE INDEX tmp_idx_index_issues_with_outdate_blocking_count ON issues USING bt CREATE INDEX tmp_index_for_email_unconfirmation_migration ON emails USING btree (id) WHERE (confirmed_at IS NOT NULL); +CREATE INDEX tmp_index_oauth_applications_on_id_where_trusted ON oauth_applications USING btree (id) WHERE (trusted = true); + CREATE UNIQUE INDEX unique_merge_request_metrics_by_merge_request_id ON merge_request_metrics USING btree (merge_request_id); CREATE UNIQUE INDEX users_security_dashboard_projects_unique_index ON users_security_dashboard_projects USING btree (project_id, user_id); diff --git a/lib/api/nuget_packages.rb b/lib/api/nuget_packages.rb index 0f2c956a9d..f894ab1074 100644 --- a/lib/api/nuget_packages.rb +++ b/lib/api/nuget_packages.rb @@ -11,7 +11,7 @@ module API helpers ::API::Helpers::Packages::BasicAuthHelpers POSITIVE_INTEGER_REGEX = %r{\A[1-9]\d*\z}.freeze - NON_NEGATIVE_INTEGER_REGEX = %r{\A0|[1-9]\d*\z}.freeze + NON_NEGATIVE_INTEGER_REGEX = %r{\A(0|[1-9]\d*)\z}.freeze PACKAGE_FILENAME = 'package.nupkg' diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 1b169b6186..55f614f01d 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -23,7 +23,18 @@ module Gitlab end def package_name_regex - @package_name_regex ||= %r{\A\@?(([\w\-\.\+]*)\/)*([\w\-\.]+)@?(([\w\-\.\+]*)\/)*([\w\-\.]*)\z}.freeze + @package_name_regex ||= + %r{ + \A\@? + (?> # atomic group to prevent backtracking + (([\w\-\.\+]*)\/)*([\w\-\.]+) + ) + @? + (?> # atomic group to prevent backtracking + (([\w\-\.\+]*)\/)*([\w\-\.]*) + ) + \z + }x.freeze end def maven_file_name_regex diff --git a/lib/gitlab/usage_data_counters/hll_redis_counter.rb b/lib/gitlab/usage_data_counters/hll_redis_counter.rb index eb132ef096..db387d0c09 100644 --- a/lib/gitlab/usage_data_counters/hll_redis_counter.rb +++ b/lib/gitlab/usage_data_counters/hll_redis_counter.rb @@ -180,12 +180,10 @@ module Gitlab end def weekly_redis_keys(events:, start_date:, end_date:) - weeks = end_date.to_date.cweek - start_date.to_date.cweek - weeks = 1 if weeks == 0 - - (0..(weeks - 1)).map do |week_increment| - events.map { |event| redis_key(event, start_date + week_increment * 7.days) } - end.flatten + end_date = end_date.end_of_week - 1.week + (start_date.to_date..end_date.to_date).map do |date| + events.map { |event| redis_key(event, date) } + end.flatten.uniq end end end diff --git a/spec/controllers/oauth/authorizations_controller_spec.rb b/spec/controllers/oauth/authorizations_controller_spec.rb index 23d472f685..f811f13def 100644 --- a/spec/controllers/oauth/authorizations_controller_spec.rb +++ b/spec/controllers/oauth/authorizations_controller_spec.rb @@ -95,6 +95,20 @@ RSpec.describe Oauth::AuthorizationsController do subject { post :create, params: params } include_examples 'OAuth Authorizations require confirmed user' + + context 'when application is confidential' do + before do + application.update(confidential: true) + params[:response_type] = 'token' + end + + it 'does not allow the implicit flow' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template('doorkeeper/authorizations/error') + end + end end describe 'DELETE #destroy' do diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb index b3921164c8..302c00086e 100644 --- a/spec/controllers/projects/raw_controller_spec.rb +++ b/spec/controllers/projects/raw_controller_spec.rb @@ -225,6 +225,25 @@ RSpec.describe Projects::RawController do end end end + + describe 'caching' do + def request_file + get(:show, params: { namespace_id: project.namespace, project_id: project, id: 'master/README.md' }) + end + + context 'when a public project has private repo' do + let(:project) { create(:project, :public, :repository, :repository_private) } + let(:user) { create(:user, maintainer_projects: [project]) } + + it 'does not set public caching header' do + sign_in user + request_file + + expect(response.cache_control[:public]).to eq(false) + expect(response.cache_control[:max_age]).to eq(60) + end + end + end end def execute_raw_requests(requests:, project:, file_path:, **params) diff --git a/spec/controllers/projects/repositories_controller_spec.rb b/spec/controllers/projects/repositories_controller_spec.rb index 97eea7c7e9..d4247a59f1 100644 --- a/spec/controllers/projects/repositories_controller_spec.rb +++ b/spec/controllers/projects/repositories_controller_spec.rb @@ -135,6 +135,18 @@ RSpec.describe Projects::RepositoriesController do expect(response.header['ETag']).to be_present expect(response.header['Cache-Control']).to include('max-age=60, public') end + + context 'and repo is private' do + let(:project) { create(:project, :repository, :public, :repository_private) } + + it 'sets appropriate caching headers' do + get_archive + + expect(response).to have_gitlab_http_status(:ok) + expect(response.header['ETag']).to be_present + expect(response.header['Cache-Control']).to include('max-age=60, private') + end + end end context 'when ref is a commit SHA' do diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb index 451526021c..750f22b2ef 100644 --- a/spec/lib/gitlab/regex_spec.rb +++ b/spec/lib/gitlab/regex_spec.rb @@ -283,6 +283,12 @@ RSpec.describe Gitlab::Regex do it { is_expected.not_to match('my package name') } it { is_expected.not_to match('!!()()') } it { is_expected.not_to match("..\n..\foo") } + + it 'has no backtracking issue' do + Timeout.timeout(1) do + expect(subject).not_to match("-" * 50000 + ";") + end + end end describe '.maven_file_name_regex' do diff --git a/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb b/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb index e84c3c1727..ec335f4f48 100644 --- a/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb +++ b/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb @@ -210,6 +210,50 @@ RSpec.describe Gitlab::UsageDataCounters::HLLRedisCounter, :clean_gitlab_redis_s context 'when no slot is set' do it { expect(described_class.unique_events(event_names: no_slot, start_date: 7.days.ago, end_date: Date.current)).to eq(1) } end + + context 'when data crosses into new year' do + it 'does not raise error' do + expect { described_class.unique_events(event_names: [weekly_event], start_date: DateTime.parse('2020-12-26'), end_date: DateTime.parse('2021-02-01')) } + .not_to raise_error + end + end + end + end + + describe '.weekly_redis_keys' do + using RSpec::Parameterized::TableSyntax + + let(:weekly_event) { 'g_compliance_dashboard' } + let(:redis_event) { described_class.send(:event_for, weekly_event) } + + subject(:weekly_redis_keys) { described_class.send(:weekly_redis_keys, events: [redis_event], start_date: DateTime.parse(start_date), end_date: DateTime.parse(end_date)) } + + where(:start_date, :end_date, :keys) do + '2020-12-21' | '2020-12-21' | [] + '2020-12-21' | '2020-12-20' | [] + '2020-12-21' | '2020-11-21' | [] + '2021-01-01' | '2020-12-28' | [] + '2020-12-21' | '2020-12-28' | ['g_{compliance}_dashboard-2020-52'] + '2020-12-21' | '2021-01-01' | ['g_{compliance}_dashboard-2020-52'] + '2020-12-27' | '2021-01-01' | ['g_{compliance}_dashboard-2020-52'] + '2020-12-26' | '2021-01-04' | ['g_{compliance}_dashboard-2020-52', 'g_{compliance}_dashboard-2020-53'] + '2020-12-26' | '2021-01-11' | ['g_{compliance}_dashboard-2020-52', 'g_{compliance}_dashboard-2020-53', 'g_{compliance}_dashboard-2021-01'] + '2020-12-26' | '2021-01-17' | ['g_{compliance}_dashboard-2020-52', 'g_{compliance}_dashboard-2020-53', 'g_{compliance}_dashboard-2021-01'] + '2020-12-26' | '2021-01-18' | ['g_{compliance}_dashboard-2020-52', 'g_{compliance}_dashboard-2020-53', 'g_{compliance}_dashboard-2021-01', 'g_{compliance}_dashboard-2021-02'] + end + + with_them do + it "returns the correct keys" do + expect(subject).to match(keys) + end + end + + it 'returns 1 key for last for week' do + expect(described_class.send(:weekly_redis_keys, events: [redis_event], start_date: 7.days.ago.to_date, end_date: Date.current).size).to eq 1 + end + + it 'returns 4 key for last for weeks' do + expect(described_class.send(:weekly_redis_keys, events: [redis_event], start_date: 4.weeks.ago.to_date, end_date: Date.current).size).to eq 4 end end diff --git a/spec/lib/gitlab/usage_data_spec.rb b/spec/lib/gitlab/usage_data_spec.rb index f64fa2b868..3a9496d55d 100644 --- a/spec/lib/gitlab/usage_data_spec.rb +++ b/spec/lib/gitlab/usage_data_spec.rb @@ -50,6 +50,32 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do expect { described_class.uncached_data }.to raise_error('Stopped calculating recorded_at') end + + context 'when generating usage ping in critical weeks' do + it 'does not raise error when generated in last week of the year' do + travel_to(DateTime.parse('2020-12-29')) do + expect { subject }.not_to raise_error + end + end + + it 'does not raise error when generated in first week of the year' do + travel_to(DateTime.parse('2021-01-01')) do + expect { subject }.not_to raise_error + end + end + + it 'does not raise error when generated in second week of the year' do + travel_to(DateTime.parse('2021-01-07')) do + expect { subject }.not_to raise_error + end + end + + it 'does not raise error when generated in 3rd week of the year' do + travel_to(DateTime.parse('2021-01-14')) do + expect { subject }.not_to raise_error + end + end + end end describe 'usage_activity_by_stage_package' do diff --git a/spec/support/shared_examples/lib/gitlab/usage_data_counters/incident_management_activity_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/usage_data_counters/incident_management_activity_shared_examples.rb index 4e35e388b2..788c35dd5b 100644 --- a/spec/support/shared_examples/lib/gitlab/usage_data_counters/incident_management_activity_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/usage_data_counters/incident_management_activity_shared_examples.rb @@ -3,8 +3,8 @@ RSpec.shared_examples 'an incident management tracked event' do |event| describe ".track_event", :clean_gitlab_redis_shared_state do let(:counter) { Gitlab::UsageDataCounters::HLLRedisCounter } - let(:start_time) { 1.minute.ago } - let(:end_time) { 1.minute.from_now } + let(:start_time) { 1.week.ago } + let(:end_time) { 1.week.from_now } it "tracks the event using redis" do # Allow other subsequent calls