From bc299f54e841488b4ab37777761db1dfc7f3b60e Mon Sep 17 00:00:00 2001
From: GitLab Bot <gitlab-bot@gitlab.com>
Date: Tue, 27 Apr 2021 08:57:43 +0000
Subject: Add latest changes from gitlab-org/security/gitlab@13-11-stable-ee

---
 Gemfile.lock                                       |  4 +-
 .../concerns/sessionless_authentication.rb         |  6 ++-
 app/controllers/graphql_controller.rb              |  8 +++-
 app/graphql/mutations/base_mutation.rb             | 13 +++++-
 app/policies/global_policy.rb                      |  4 ++
 .../dependency_proxy_authentication_service.rb     |  5 ++-
 ...ent-mutation-execution-with-read-api-tokens.yml |  5 +++
 ...ity-10io-dependency-proxy-and-deploy-tokens.yml |  5 +++
 .../security-bump-carrierwave-to-1-3-2.yml         |  5 +++
 lib/gitlab/auth/scope_validator.rb                 | 24 +++++++++++
 .../graphql/authorize/object_authorization.rb      | 19 +++++++--
 .../graphql/mutations/notes/create/note_spec.rb    |  2 +
 spec/requests/jwt_controller_spec.rb               | 10 ++---
 ...dependency_proxy_authentication_service_spec.rb | 25 ++++++------
 spec/services/projects/download_service_spec.rb    |  5 ++-
 spec/support/helpers/graphql_helpers.rb            | 18 +++++----
 .../requests/graphql_shared_examples.rb            | 46 ++++++++++++++++++++++
 17 files changed, 168 insertions(+), 36 deletions(-)
 create mode 100644 changelogs/unreleased/327155-prevent-mutation-execution-with-read-api-tokens.yml
 create mode 100644 changelogs/unreleased/security-10io-dependency-proxy-and-deploy-tokens.yml
 create mode 100644 changelogs/unreleased/security-bump-carrierwave-to-1-3-2.yml
 create mode 100644 lib/gitlab/auth/scope_validator.rb

diff --git a/Gemfile.lock b/Gemfile.lock
index 19fad573b63..007ca87b465 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -171,10 +171,11 @@ GEM
     capybara-screenshot (1.0.22)
       capybara (>= 1.0, < 4)
       launchy
-    carrierwave (1.3.1)
+    carrierwave (1.3.2)
       activemodel (>= 4.0.0)
       activesupport (>= 4.0.0)
       mime-types (>= 1.16)
+      ssrf_filter (~> 1.0)
     cbor (0.5.9.6)
     character_set (1.4.0)
     charlock_holmes (0.7.7)
@@ -1205,6 +1206,7 @@ GEM
       sprockets (>= 3.0.0)
     sqlite3 (1.3.13)
     sshkey (2.0.0)
+    ssrf_filter (1.0.7)
     stackprof (0.2.15)
     state_machines (0.5.0)
     state_machines-activemodel (0.8.0)
diff --git a/app/controllers/concerns/sessionless_authentication.rb b/app/controllers/concerns/sessionless_authentication.rb
index 882fef7a342..3c8a683439a 100644
--- a/app/controllers/concerns/sessionless_authentication.rb
+++ b/app/controllers/concerns/sessionless_authentication.rb
@@ -7,11 +7,15 @@
 module SessionlessAuthentication
   # This filter handles personal access tokens, atom requests with rss tokens, and static object tokens
   def authenticate_sessionless_user!(request_format)
-    user = Gitlab::Auth::RequestAuthenticator.new(request).find_sessionless_user(request_format)
+    user = request_authenticator.find_sessionless_user(request_format)
 
     sessionless_sign_in(user) if user
   end
 
+  def request_authenticator
+    @request_authenticator ||= Gitlab::Auth::RequestAuthenticator.new(request)
+  end
+
   def sessionless_user?
     current_user && !session.key?('warden.user.user.key')
   end
diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb
index a13ec1daddb..38bfb5ef2f8 100644
--- a/app/controllers/graphql_controller.rb
+++ b/app/controllers/graphql_controller.rb
@@ -110,7 +110,13 @@ class GraphqlController < ApplicationController
   end
 
   def context
-    @context ||= { current_user: current_user, is_sessionless_user: !!sessionless_user?, request: request }
+    api_user = !!sessionless_user?
+    @context ||= {
+      current_user: current_user,
+      is_sessionless_user: api_user,
+      request: request,
+      scope_validator: ::Gitlab::Auth::ScopeValidator.new(api_user, request_authenticator)
+    }
   end
 
   def build_variables(variable_info)
diff --git a/app/graphql/mutations/base_mutation.rb b/app/graphql/mutations/base_mutation.rb
index 1f18a37fcb9..da658e1f108 100644
--- a/app/graphql/mutations/base_mutation.rb
+++ b/app/graphql/mutations/base_mutation.rb
@@ -44,9 +44,18 @@ module Mutations
       end
     end
 
+    def self.authorizes_object?
+      true
+    end
+
     def self.authorized?(object, context)
-      # we never provide an object to mutations, but we do need to have a user.
-      context[:current_user].present? && !context[:current_user].blocked?
+      auth = ::Gitlab::Graphql::Authorize::ObjectAuthorization.new(:execute_graphql_mutation, :api)
+
+      return true if auth.ok?(:global, context[:current_user],
+                              scope_validator: context[:scope_validator])
+
+      # in our mutations we raise, rather than returning a null value.
+      raise_resource_not_available_error!
     end
 
     # See: AuthorizeResource#authorized_resource?
diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb
index 5ee34ebbb2f..d16c4734b2c 100644
--- a/app/policies/global_policy.rb
+++ b/app/policies/global_policy.rb
@@ -23,6 +23,7 @@ class GlobalPolicy < BasePolicy
     prevent :receive_notifications
     prevent :use_quick_actions
     prevent :create_group
+    prevent :execute_graphql_mutation
   end
 
   rule { default }.policy do
@@ -32,6 +33,7 @@ class GlobalPolicy < BasePolicy
     enable :receive_notifications
     enable :use_quick_actions
     enable :use_slash_commands
+    enable :execute_graphql_mutation
   end
 
   rule { inactive }.policy do
@@ -48,6 +50,8 @@ class GlobalPolicy < BasePolicy
     prevent :use_slash_commands
   end
 
+  rule { ~can?(:access_api) }.prevent :execute_graphql_mutation
+
   rule { blocked | (internal & ~migration_bot & ~security_bot) }.policy do
     prevent :access_git
   end
diff --git a/app/services/auth/dependency_proxy_authentication_service.rb b/app/services/auth/dependency_proxy_authentication_service.rb
index 1b8c16b7c79..fab42e0ebb6 100644
--- a/app/services/auth/dependency_proxy_authentication_service.rb
+++ b/app/services/auth/dependency_proxy_authentication_service.rb
@@ -8,7 +8,10 @@ module Auth
 
     def execute(authentication_abilities:)
       return error('dependency proxy not enabled', 404) unless ::Gitlab.config.dependency_proxy.enabled
-      return error('access forbidden', 403) unless current_user
+
+      # Because app/controllers/concerns/dependency_proxy/auth.rb consumes this
+      # JWT only as `User.find`, we currently only allow User (not DeployToken, etc)
+      return error('access forbidden', 403) unless current_user.is_a?(User)
 
       { token: authorized_token.encoded }
     end
diff --git a/changelogs/unreleased/327155-prevent-mutation-execution-with-read-api-tokens.yml b/changelogs/unreleased/327155-prevent-mutation-execution-with-read-api-tokens.yml
new file mode 100644
index 00000000000..1758390a5cb
--- /dev/null
+++ b/changelogs/unreleased/327155-prevent-mutation-execution-with-read-api-tokens.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent tokens with only read_api scope from executing mutations
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-10io-dependency-proxy-and-deploy-tokens.yml b/changelogs/unreleased/security-10io-dependency-proxy-and-deploy-tokens.yml
new file mode 100644
index 00000000000..26137a42420
--- /dev/null
+++ b/changelogs/unreleased/security-10io-dependency-proxy-and-deploy-tokens.yml
@@ -0,0 +1,5 @@
+---
+title: Do not allow deploy tokens in the dependency proxy authentication service
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-bump-carrierwave-to-1-3-2.yml b/changelogs/unreleased/security-bump-carrierwave-to-1-3-2.yml
new file mode 100644
index 00000000000..4e31f9d855d
--- /dev/null
+++ b/changelogs/unreleased/security-bump-carrierwave-to-1-3-2.yml
@@ -0,0 +1,5 @@
+---
+title: Bump Carrierwave gem to v1.3.2
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/auth/scope_validator.rb b/lib/gitlab/auth/scope_validator.rb
new file mode 100644
index 00000000000..de4c36ad594
--- /dev/null
+++ b/lib/gitlab/auth/scope_validator.rb
@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+# Wrapper around a RequestAuthenticator to
+# perform authorization of scopes. Access is limited to
+# only those methods needed to validate that an API user
+# has at least one permitted scope.
+module Gitlab
+  module Auth
+    class ScopeValidator
+      def initialize(api_user, request_authenticator)
+        @api_user = api_user
+        @request_authenticator = request_authenticator
+      end
+
+      def valid_for?(permitted)
+        return true unless @api_user
+        return true if permitted.none?
+
+        scopes = permitted.map { |s| API::Scope.new(s) }
+        @request_authenticator.valid_access_token?(scopes: scopes)
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/graphql/authorize/object_authorization.rb b/lib/gitlab/graphql/authorize/object_authorization.rb
index 0bc87108871..f13acc9ea27 100644
--- a/lib/gitlab/graphql/authorize/object_authorization.rb
+++ b/lib/gitlab/graphql/authorize/object_authorization.rb
@@ -4,10 +4,11 @@ module Gitlab
   module Graphql
     module Authorize
       class ObjectAuthorization
-        attr_reader :abilities
+        attr_reader :abilities, :permitted_scopes
 
-        def initialize(abilities)
+        def initialize(abilities, scopes = %i[api read_api])
           @abilities = Array.wrap(abilities).flatten
+          @permitted_scopes = Array.wrap(scopes)
         end
 
         def none?
@@ -18,7 +19,13 @@ module Gitlab
           abilities.present?
         end
 
-        def ok?(object, current_user)
+        def ok?(object, current_user, scope_validator: nil)
+          scopes_ok?(scope_validator) && abilities_ok?(object, current_user)
+        end
+
+        private
+
+        def abilities_ok?(object, current_user)
           return true if none?
 
           subject = object.try(:declarative_policy_subject) || object
@@ -26,6 +33,12 @@ module Gitlab
             Ability.allowed?(current_user, ability, subject)
           end
         end
+
+        def scopes_ok?(validator)
+          return true unless validator.present?
+
+          validator.valid_for?(permitted_scopes)
+        end
       end
     end
   end
diff --git a/spec/requests/api/graphql/mutations/notes/create/note_spec.rb b/spec/requests/api/graphql/mutations/notes/create/note_spec.rb
index 1eed1c8e2ae..8dd8ed361ba 100644
--- a/spec/requests/api/graphql/mutations/notes/create/note_spec.rb
+++ b/spec/requests/api/graphql/mutations/notes/create/note_spec.rb
@@ -31,6 +31,8 @@ RSpec.describe 'Adding a Note' do
       project.add_developer(current_user)
     end
 
+    it_behaves_like 'a working GraphQL mutation'
+
     it_behaves_like 'a Note mutation that creates a Note'
 
     it_behaves_like 'a Note mutation when there are active record validation errors'
diff --git a/spec/requests/jwt_controller_spec.rb b/spec/requests/jwt_controller_spec.rb
index 8be26784a3d..5b5658da97e 100644
--- a/spec/requests/jwt_controller_spec.rb
+++ b/spec/requests/jwt_controller_spec.rb
@@ -263,25 +263,21 @@ RSpec.describe JwtController do
       let(:credential_user) { group_deploy_token.username }
       let(:credential_password) { group_deploy_token.token }
 
-      it_behaves_like 'with valid credentials'
+      it_behaves_like 'returning response status', :forbidden
     end
 
     context 'with project deploy token' do
       let(:credential_user) { project_deploy_token.username }
       let(:credential_password) { project_deploy_token.token }
 
-      it_behaves_like 'with valid credentials'
+      it_behaves_like 'returning response status', :forbidden
     end
 
     context 'with invalid credentials' do
       let(:credential_user) { 'foo' }
       let(:credential_password) { 'bar' }
 
-      it 'returns unauthorized' do
-        subject
-
-        expect(response).to have_gitlab_http_status(:unauthorized)
-      end
+      it_behaves_like 'returning response status', :unauthorized
     end
   end
 
diff --git a/spec/services/auth/dependency_proxy_authentication_service_spec.rb b/spec/services/auth/dependency_proxy_authentication_service_spec.rb
index ba50149f53a..1fd1677c7da 100644
--- a/spec/services/auth/dependency_proxy_authentication_service_spec.rb
+++ b/spec/services/auth/dependency_proxy_authentication_service_spec.rb
@@ -13,28 +13,31 @@ RSpec.describe Auth::DependencyProxyAuthenticationService do
   describe '#execute' do
     subject { service.execute(authentication_abilities: nil) }
 
+    shared_examples 'returning' do |status:, message:|
+      it "returns #{message}", :aggregate_failures do
+        expect(subject[:http_status]).to eq(status)
+        expect(subject[:message]).to eq(message)
+      end
+    end
+
     context 'dependency proxy is not enabled' do
       before do
         stub_config(dependency_proxy: { enabled: false })
       end
 
-      it 'returns not found' do
-        result = subject
-
-        expect(result[:http_status]).to eq(404)
-        expect(result[:message]).to eq('dependency proxy not enabled')
-      end
+      it_behaves_like 'returning', status: 404, message: 'dependency proxy not enabled'
     end
 
     context 'without a user' do
       let(:user) { nil }
 
-      it 'returns forbidden' do
-        result = subject
+      it_behaves_like 'returning', status: 403, message: 'access forbidden'
+    end
+
+    context 'with a deploy token as user' do
+      let_it_be(:user) { create(:deploy_token) }
 
-        expect(result[:http_status]).to eq(403)
-        expect(result[:message]).to eq('access forbidden')
-      end
+      it_behaves_like 'returning', status: 403, message: 'access forbidden'
     end
 
     context 'with a user' do
diff --git a/spec/services/projects/download_service_spec.rb b/spec/services/projects/download_service_spec.rb
index 0f743eaa7f5..7d4fce814f5 100644
--- a/spec/services/projects/download_service_spec.rb
+++ b/spec/services/projects/download_service_spec.rb
@@ -20,8 +20,9 @@ RSpec.describe Projects::DownloadService do
 
     context 'for URLs that are on the whitelist' do
       before do
-        stub_request(:get, 'http://mycompany.fogbugz.com/rails_sample.jpg').to_return(body: File.read(Rails.root + 'spec/fixtures/rails_sample.jpg'))
-        stub_request(:get, 'http://mycompany.fogbugz.com/doc_sample.txt').to_return(body: File.read(Rails.root + 'spec/fixtures/doc_sample.txt'))
+        # `ssrf_filter` resolves the hostname. See https://github.com/carrierwaveuploader/carrierwave/commit/91714adda998bc9e8decf5b1f5d260d808761304
+        stub_request(:get, %r{http://[\d\.]+/rails_sample.jpg}).to_return(body: File.read(Rails.root + 'spec/fixtures/rails_sample.jpg'))
+        stub_request(:get, %r{http://[\d\.]+/doc_sample.txt}).to_return(body: File.read(Rails.root + 'spec/fixtures/doc_sample.txt'))
       end
 
       context 'an image file' do
diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb
index d714f04fbba..9d6c6ab93e4 100644
--- a/spec/support/helpers/graphql_helpers.rb
+++ b/spec/support/helpers/graphql_helpers.rb
@@ -396,17 +396,21 @@ module GraphqlHelpers
     post api('/', current_user, version: 'graphql'), params: { _json: queries }, headers: headers
   end
 
-  def post_graphql(query, current_user: nil, variables: nil, headers: {})
+  def post_graphql(query, current_user: nil, variables: nil, headers: {}, token: {})
     params = { query: query, variables: serialize_variables(variables) }
-    post api('/', current_user, version: 'graphql'), params: params, headers: headers
+    post api('/', current_user, version: 'graphql', **token), params: params, headers: headers
 
-    if graphql_errors # Errors are acceptable, but not this one:
-      expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error'))
-    end
+    return unless graphql_errors
+
+    # Errors are acceptable, but not this one:
+    expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error'))
   end
 
-  def post_graphql_mutation(mutation, current_user: nil)
-    post_graphql(mutation.query, current_user: current_user, variables: mutation.variables)
+  def post_graphql_mutation(mutation, current_user: nil, token: {})
+    post_graphql(mutation.query,
+                 current_user: current_user,
+                 variables: mutation.variables,
+                 token: token)
   end
 
   def post_graphql_mutation_with_uploads(mutation, current_user: nil)
diff --git a/spec/support/shared_examples/requests/graphql_shared_examples.rb b/spec/support/shared_examples/requests/graphql_shared_examples.rb
index a66bc7112fe..d133c5ea641 100644
--- a/spec/support/shared_examples/requests/graphql_shared_examples.rb
+++ b/spec/support/shared_examples/requests/graphql_shared_examples.rb
@@ -10,6 +10,52 @@ RSpec.shared_examples 'a working graphql query' do
   end
 end
 
+RSpec.shared_examples 'a working GraphQL mutation' do
+  include GraphqlHelpers
+
+  before do
+    post_graphql_mutation(mutation, current_user: current_user, token: token)
+  end
+
+  shared_examples 'allows access to the mutation' do
+    let(:scopes) { ['api'] }
+
+    it_behaves_like 'a working graphql query' do
+      it 'returns data' do
+        expect(graphql_data.compact).not_to be_empty
+      end
+    end
+  end
+
+  shared_examples 'prevents access to the mutation' do
+    let(:scopes) { ['read_api'] }
+
+    it 'does not resolve the mutation' do
+      expect(graphql_data.compact).to be_empty
+      expect(graphql_errors).to be_present
+    end
+  end
+
+  context 'with a personal access token' do
+    let(:token) do
+      pat = create(:personal_access_token, user: current_user, scopes: scopes)
+      { personal_access_token: pat }
+    end
+
+    it_behaves_like 'prevents access to the mutation'
+    it_behaves_like 'allows access to the mutation'
+  end
+
+  context 'with an OAuth token' do
+    let(:token) do
+      { oauth_access_token: create(:oauth_access_token, resource_owner: current_user, scopes: scopes.join(' ')) }
+    end
+
+    it_behaves_like 'prevents access to the mutation'
+    it_behaves_like 'allows access to the mutation'
+  end
+end
+
 RSpec.shared_examples 'a mutation on an unauthorized resource' do
   it_behaves_like 'a mutation that returns top-level errors',
                       errors: [::Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR]
-- 
cgit v1.2.3