diff options
17 files changed, 168 insertions, 36 deletions
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] |