diff options
23 files changed, 228 insertions, 9 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index fe4775033e1..e45d9933ab4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,19 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 13.7.2 (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. +- Fix stealing API token from GitLab Pages and DoS Prometheus through GitLab Pages. +- Update trusted OAuth applications to set them as confidential. +- Upgrade Workhorse to 8.58.2. + + ## 13.7.1 (2020-12-23) ### Fixed (1 change) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 5d6328a378c..4ed0bdb8717 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -13.7.1
\ No newline at end of file +13.7.2
\ No newline at end of file diff --git a/GITLAB_PAGES_VERSION b/GITLAB_PAGES_VERSION index 359c41089a4..2b17ffd5042 100644 --- a/GITLAB_PAGES_VERSION +++ b/GITLAB_PAGES_VERSION @@ -1 +1 @@ -1.32.0 +1.34.0 diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index b315ff1896d..8aa8b5f68bd 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -8.58.0 +8.58.2 @@ -1 +1 @@ -13.7.1
\ No newline at end of file +13.7.2
\ No newline at end of file diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index 6e8686ee90b..ade698baa7f 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 8be7af3e2c5..3fff93abe5c 100644 --- a/app/controllers/projects/raw_controller.rb +++ b/app/controllers/projects/raw_controller.rb @@ -21,7 +21,7 @@ class Projects::RawController < Projects::ApplicationController def show @blob = @repository.blob_at(@ref, @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 fb6a09cff65..da018b24836 100644 --- a/app/controllers/projects/repositories_controller.rb +++ b/app/controllers/projects/repositories_controller.rb @@ -53,7 +53,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 00000000000..bcb94c65125 --- /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 00000000000..914e96473a0 --- /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 105b7701409..6380f64c64c 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -23004,6 +23004,8 @@ CREATE INDEX tmp_build_stage_position_index ON ci_builds USING btree (stage_id, 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 INDEX tmp_index_on_vulnerabilities_non_dismissed ON vulnerabilities USING btree (id) WHERE (state <> 2); CREATE UNIQUE INDEX unique_merge_request_metrics_by_merge_request_id ON merge_request_metrics USING btree (merge_request_id); diff --git a/lib/api/concerns/packages/nuget_endpoints.rb b/lib/api/concerns/packages/nuget_endpoints.rb index 5177c4d23c0..1a03a6a6dad 100644 --- a/lib/api/concerns/packages/nuget_endpoints.rb +++ b/lib/api/concerns/packages/nuget_endpoints.rb @@ -15,7 +15,7 @@ module API extend ActiveSupport::Concern 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 included do helpers do diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 4ae6297f6f5..96f2b7570b3 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -27,7 +27,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/spec/controllers/oauth/authorizations_controller_spec.rb b/spec/controllers/oauth/authorizations_controller_spec.rb index 23d472f6853..f811f13def8 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 dfe7ba34e6d..b1c3c1c0276 100644 --- a/spec/controllers/projects/raw_controller_spec.rb +++ b/spec/controllers/projects/raw_controller_spec.rb @@ -250,6 +250,18 @@ RSpec.describe Projects::RawController do expect(response.cache_control[:no_store]).to be_nil 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.header['Cache-Control']).to include('max-age=60, private') + end + end + context 'when If-None-Match header is set' do it 'returns a 304 status' do request_file diff --git a/spec/controllers/projects/repositories_controller_spec.rb b/spec/controllers/projects/repositories_controller_spec.rb index e7f4a8a1422..e6327a72a68 100644 --- a/spec/controllers/projects/repositories_controller_spec.rb +++ b/spec/controllers/projects/repositories_controller_spec.rb @@ -137,6 +137,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 ebb37f45b95..776ca81a338 100644 --- a/spec/lib/gitlab/regex_spec.rb +++ b/spec/lib/gitlab/regex_spec.rb @@ -292,6 +292,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/workhorse/CHANGELOG b/workhorse/CHANGELOG index 489ac4531fc..271928845c1 100644 --- a/workhorse/CHANGELOG +++ b/workhorse/CHANGELOG @@ -1,5 +1,17 @@ # Changelog for gitlab-workhorse +## v8.58.2 + +### Security +- Allow DELETE HTTP method + https://gitlab.com/gitlab-org/gitlab-workhorse/-/merge_requests/ + +## v8.58.1 + +### Security +- Reject unknown http methods + https://gitlab.com/gitlab-org/gitlab-workhorse/-/merge_requests/ + ## v8.58.0 ### Added diff --git a/workhorse/VERSION b/workhorse/VERSION index b315ff1896d..8aa8b5f68bd 100644 --- a/workhorse/VERSION +++ b/workhorse/VERSION @@ -1 +1 @@ -8.58.0 +8.58.2 diff --git a/workhorse/internal/rejectmethods/middleware.go b/workhorse/internal/rejectmethods/middleware.go new file mode 100644 index 00000000000..171463979d5 --- /dev/null +++ b/workhorse/internal/rejectmethods/middleware.go @@ -0,0 +1,38 @@ +package rejectmethods + +import ( + "net/http" + + "github.com/prometheus/client_golang/prometheus" +) + +var acceptedMethods = map[string]bool{ + http.MethodGet: true, + http.MethodHead: true, + http.MethodPost: true, + http.MethodPut: true, + http.MethodPatch: true, + http.MethodDelete: true, + http.MethodConnect: true, + http.MethodOptions: true, + http.MethodTrace: true, +} + +var rejectedRequestsCount = prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "gitlab_workhorse_unknown_method_rejected_requests", + Help: "The number of requests with unknown HTTP method which were rejected", + }, +) + +// NewMiddleware returns middleware which rejects all unknown http methods +func NewMiddleware(handler http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if acceptedMethods[r.Method] { + handler.ServeHTTP(w, r) + } else { + rejectedRequestsCount.Inc() + http.Error(w, http.StatusText(http.StatusMethodNotAllowed), http.StatusMethodNotAllowed) + } + }) +} diff --git a/workhorse/internal/rejectmethods/middleware_test.go b/workhorse/internal/rejectmethods/middleware_test.go new file mode 100644 index 00000000000..2921975aeef --- /dev/null +++ b/workhorse/internal/rejectmethods/middleware_test.go @@ -0,0 +1,43 @@ +package rejectmethods + +import ( + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestNewMiddleware(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + io.WriteString(w, "OK\n") + }) + + middleware := NewMiddleware(handler) + + acceptedMethods := []string{"GET", "HEAD", "POST", "PUT", "PATCH", "CONNECT", "OPTIONS", "TRACE"} + for _, method := range acceptedMethods { + t.Run(method, func(t *testing.T) { + tmpRequest, _ := http.NewRequest(method, "/", nil) + recorder := httptest.NewRecorder() + + middleware.ServeHTTP(recorder, tmpRequest) + + result := recorder.Result() + + require.Equal(t, http.StatusOK, result.StatusCode) + }) + } + + t.Run("UNKNOWN", func(t *testing.T) { + tmpRequest, _ := http.NewRequest("UNKNOWN", "/", nil) + recorder := httptest.NewRecorder() + + middleware.ServeHTTP(recorder, tmpRequest) + + result := recorder.Result() + + require.Equal(t, http.StatusMethodNotAllowed, result.StatusCode) + }) +} diff --git a/workhorse/internal/upstream/upstream.go b/workhorse/internal/upstream/upstream.go index fd3f6191a5a..c81a21c0ecd 100644 --- a/workhorse/internal/upstream/upstream.go +++ b/workhorse/internal/upstream/upstream.go @@ -17,6 +17,7 @@ import ( "gitlab.com/gitlab-org/gitlab-workhorse/internal/config" "gitlab.com/gitlab-org/gitlab-workhorse/internal/helper" + "gitlab.com/gitlab-org/gitlab-workhorse/internal/rejectmethods" "gitlab.com/gitlab-org/gitlab-workhorse/internal/upload" "gitlab.com/gitlab-org/gitlab-workhorse/internal/upstream/roundtripper" "gitlab.com/gitlab-org/gitlab-workhorse/internal/urlprefix" @@ -63,6 +64,8 @@ func NewUpstream(cfg config.Config, accessLogger *logrus.Logger) http.Handler { } handler := correlation.InjectCorrelationID(&up, correlationOpts...) + // TODO: move to LabKit https://gitlab.com/gitlab-org/gitlab-workhorse/-/issues/339 + handler = rejectmethods.NewMiddleware(handler) return handler } diff --git a/workhorse/main_test.go b/workhorse/main_test.go index 16fa8ff10b7..d15af1d3e4c 100644 --- a/workhorse/main_test.go +++ b/workhorse/main_test.go @@ -642,6 +642,24 @@ func TestPropagateCorrelationIdHeader(t *testing.T) { } } +func TestRejectUnknownMethod(t *testing.T) { + ts := testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + }) + defer ts.Close() + ws := startWorkhorseServer(ts.URL) + defer ws.Close() + + req, err := http.NewRequest("UNKNOWN", ws.URL+"/api/v3/projects/123/repository/not/special", nil) + require.NoError(t, err) + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + + require.Equal(t, http.StatusMethodNotAllowed, resp.StatusCode) +} + func setupStaticFile(fpath, content string) error { return setupStaticFileHelper(fpath, content, testDocumentRoot) } |