diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-04-27 08:57:06 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-04-27 08:57:06 +0000 |
commit | ecec480cbe10cc9740d4b83147aec3bbd533ef40 (patch) | |
tree | e66b6529b42b2013910fd6d60c152569b47bea12 | |
parent | 8244be7ee6dc69e639b516ceefe993bd323ed310 (diff) | |
download | gitlab-ce-ecec480cbe10cc9740d4b83147aec3bbd533ef40.tar.gz |
Add latest changes from gitlab-org/security/gitlab@13-10-stable-ee
-rw-r--r-- | Gemfile.lock | 4 | ||||
-rw-r--r-- | app/controllers/concerns/sessionless_authentication.rb | 6 | ||||
-rw-r--r-- | app/controllers/graphql_controller.rb | 8 | ||||
-rw-r--r-- | app/graphql/mutations/base_mutation.rb | 4 | ||||
-rw-r--r-- | app/policies/global_policy.rb | 4 | ||||
-rw-r--r-- | changelogs/unreleased/327155-prevent-mutation-execution-with-read-api-tokens.yml | 5 | ||||
-rw-r--r-- | changelogs/unreleased/security-bump-carrierwave-to-1-3-2.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/auth/scope_validator.rb | 24 | ||||
-rw-r--r-- | lib/gitlab/graphql/authorize/object_authorization.rb | 45 | ||||
-rw-r--r-- | spec/requests/api/graphql/mutations/notes/create/note_spec.rb | 2 | ||||
-rw-r--r-- | spec/services/projects/download_service_spec.rb | 5 | ||||
-rw-r--r-- | spec/support/helpers/graphql_helpers.rb | 18 | ||||
-rw-r--r-- | spec/support/shared_examples/requests/graphql_shared_examples.rb | 46 |
13 files changed, 164 insertions, 12 deletions
diff --git a/Gemfile.lock b/Gemfile.lock index e6cf7c5d935..b5bbd5a61d6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -170,10 +170,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) @@ -1203,6 +1204,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 a9ef33bf3b9..36ba3d686af 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 53064041ab8..a728d5cc18b 100644 --- a/app/controllers/graphql_controller.rb +++ b/app/controllers/graphql_controller.rb @@ -108,7 +108,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 ac5ddc5bd4c..a53cc72d904 100644 --- a/app/graphql/mutations/base_mutation.rb +++ b/app/graphql/mutations/base_mutation.rb @@ -28,8 +28,12 @@ module Mutations end def ready?(**args) + auth = ::Gitlab::Graphql::Authorize::ObjectAuthorization.new(:execute_graphql_mutation, :api) + if Gitlab::Database.read_only? raise Gitlab::Graphql::Errors::ResourceNotAvailable, ERROR_MESSAGE + elsif !auth.ok?(:global, current_user, scope_validator: context[:scope_validator]) + raise_resource_not_available_error! else true end 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/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-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 new file mode 100644 index 00000000000..f13acc9ea27 --- /dev/null +++ b/lib/gitlab/graphql/authorize/object_authorization.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Authorize + class ObjectAuthorization + attr_reader :abilities, :permitted_scopes + + def initialize(abilities, scopes = %i[api read_api]) + @abilities = Array.wrap(abilities).flatten + @permitted_scopes = Array.wrap(scopes) + end + + def none? + abilities.empty? + end + + def any? + abilities.present? + end + + 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 + abilities.all? do |ability| + 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 +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/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 75d9508f470..5ea3f65e660 100644 --- a/spec/support/helpers/graphql_helpers.rb +++ b/spec/support/helpers/graphql_helpers.rb @@ -390,17 +390,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] |