summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--CHANGELOG.md13
-rw-r--r--GITALY_SERVER_VERSION2
-rw-r--r--GITLAB_PAGES_VERSION2
-rw-r--r--GITLAB_WORKHORSE_VERSION2
-rw-r--r--VERSION2
-rw-r--r--app/controllers/oauth/authorizations_controller.rb11
-rw-r--r--app/controllers/projects/raw_controller.rb2
-rw-r--r--app/controllers/projects/repositories_controller.rb2
-rw-r--r--db/migrate/20201222151823_update_trusted_apps_to_confidential.rb23
-rw-r--r--db/schema_migrations/202012221518231
-rw-r--r--db/structure.sql2
-rw-r--r--lib/api/concerns/packages/nuget_endpoints.rb2
-rw-r--r--lib/gitlab/regex.rb13
-rw-r--r--spec/controllers/oauth/authorizations_controller_spec.rb14
-rw-r--r--spec/controllers/projects/raw_controller_spec.rb12
-rw-r--r--spec/controllers/projects/repositories_controller_spec.rb12
-rw-r--r--spec/lib/gitlab/regex_spec.rb6
-rw-r--r--workhorse/CHANGELOG12
-rw-r--r--workhorse/VERSION2
-rw-r--r--workhorse/internal/rejectmethods/middleware.go38
-rw-r--r--workhorse/internal/rejectmethods/middleware_test.go43
-rw-r--r--workhorse/internal/upstream/upstream.go3
-rw-r--r--workhorse/main_test.go18
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
diff --git a/VERSION b/VERSION
index 5d6328a378c..4ed0bdb8717 100644
--- a/VERSION
+++ b/VERSION
@@ -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)
}