Merge remote-tracking branch 'dev/13-7-stable' into 13-7-stable

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)

GITALY_SERVER_VERSION

-13.7.1
\ No newline at end of file
+13.7.2
\ No newline at end of file

GITLAB_PAGES_VERSION

-1.32.0
+1.34.0

GITLAB_WORKHORSE_VERSION

-8.58.0
+8.58.2

VERSION

-13.7.1
\ No newline at end of file
+13.7.2
\ No newline at end of file

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!

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

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
 

db/migrate/20201222151823_update_trusted_apps_to_confidential.rb

+# 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

db/schema_migrations/20201222151823

+d3af120a74b4c55345ac7fb524395251cd3c1b3cd9685f711196a134f427845c
\ No newline at end of file

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);

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

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

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

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

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

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

workhorse/CHANGELOG

 # 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

workhorse/VERSION

-8.58.0
+8.58.2

workhorse/internal/rejectmethods/middleware.go

+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)
+		}
+	})
+}

workhorse/internal/rejectmethods/middleware_test.go

+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)
+	})
+}

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
 }
 

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)
 }