diff --git a/CHANGELOG.md b/CHANGELOG.md index 395ff400fa..aeb17e5bb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,10 +2,18 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. -## 13.2.5 (2020-08-17) +## 13.2.6 (2020-08-18) - No changes. +## 13.2.5 (2020-08-17) + +### Security (2 changes) + +- Stop deploy token being mis-used as user in ProjectPolicy and GroupPolicy. +- Project access is checked during deploy token authentication. + + ## 13.2.4 (2020-08-11) ### Security (1 change) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 8c339431c5..9a30e11574 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -13.2.5 +13.2.6 diff --git a/VERSION b/VERSION index 8c339431c5..9a30e11574 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -13.2.5 +13.2.6 diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index 62f6609387..92cba5f8f7 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -166,6 +166,7 @@ class GroupPolicy < BasePolicy def access_level return GroupMember::NO_ACCESS if @user.nil? + return GroupMember::NO_ACCESS unless user_is_user? @access_level ||= lookup_access_level! end @@ -173,6 +174,12 @@ class GroupPolicy < BasePolicy def lookup_access_level! @subject.max_member_access_for_user(@user) end + + private + + def user_is_user? + user.is_a?(User) + end end GroupPolicy.prepend_if_ee('EE::GroupPolicy') diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 39b39bd2fc..3a245119cb 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -603,8 +603,13 @@ class ProjectPolicy < BasePolicy private + def user_is_user? + user.is_a?(User) + end + def team_member? return false if @user.nil? + return false unless user_is_user? greedy_load_subject = false @@ -632,6 +637,7 @@ class ProjectPolicy < BasePolicy # rubocop: disable CodeReuse/ActiveRecord def project_group_member? return false if @user.nil? + return false unless user_is_user? project.group && ( @@ -643,6 +649,7 @@ class ProjectPolicy < BasePolicy def team_access_level return -1 if @user.nil? + return -1 unless user_is_user? lookup_access_level! end diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 44e8c9c04b..1a23814959 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -218,6 +218,9 @@ module Gitlab return unless token && login return if login != token.username + # Registry access (with jwt) does not have access to project + return if project && !token.has_access_to?(project) + scopes = abilities_for_scopes(token.scopes) if valid_scoped_token?(token, all_available_scopes) diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index 0b391c8cba..b62f9b55b6 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -541,7 +541,7 @@ RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching do it 'fails if token is not related to project' do another_deploy_token = create(:deploy_token) - expect(gl_auth.find_for_git_client(login, another_deploy_token.token, project: project, ip: 'ip')) + expect(gl_auth.find_for_git_client(another_deploy_token.username, another_deploy_token.token, project: project, ip: 'ip')) .to eq(auth_failure) end @@ -566,6 +566,13 @@ RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching do expect(subject).to eq(auth_success) end + + it 'fails if token is not related to group' do + another_deploy_token = create(:deploy_token, :group, read_repository: true) + + expect(gl_auth.find_for_git_client(another_deploy_token.username, another_deploy_token.token, project: project_with_group, ip: 'ip')) + .to eq(auth_failure) + end end context 'when the deploy token has read_registry as a scope' do diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 733cc9bd9c..9bd692b45c 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -63,6 +63,24 @@ RSpec.describe GroupPolicy do end end + shared_examples 'deploy token does not get confused with user' do + before do + deploy_token.update!(id: user_id) + end + + let(:deploy_token) { create(:deploy_token) } + let(:current_user) { deploy_token } + + it do + expect_disallowed(*read_group_permissions) + expect_disallowed(*guest_permissions) + expect_disallowed(*reporter_permissions) + expect_disallowed(*developer_permissions) + expect_disallowed(*maintainer_permissions) + expect_disallowed(*owner_permissions) + end + end + context 'guests' do let(:current_user) { guest } @@ -74,6 +92,10 @@ RSpec.describe GroupPolicy do expect_disallowed(*maintainer_permissions) expect_disallowed(*owner_permissions) end + + it_behaves_like 'deploy token does not get confused with user' do + let(:user_id) { guest.id } + end end context 'reporter' do @@ -87,6 +109,10 @@ RSpec.describe GroupPolicy do expect_disallowed(*maintainer_permissions) expect_disallowed(*owner_permissions) end + + it_behaves_like 'deploy token does not get confused with user' do + let(:user_id) { reporter.id } + end end context 'developer' do @@ -100,6 +126,10 @@ RSpec.describe GroupPolicy do expect_disallowed(*maintainer_permissions) expect_disallowed(*owner_permissions) end + + it_behaves_like 'deploy token does not get confused with user' do + let(:user_id) { developer.id } + end end context 'maintainer' do @@ -136,6 +166,10 @@ RSpec.describe GroupPolicy do expect_disallowed(*owner_permissions) end end + + it_behaves_like 'deploy token does not get confused with user' do + let(:user_id) { maintainer.id } + end end context 'owner' do @@ -149,6 +183,10 @@ RSpec.describe GroupPolicy do expect_allowed(*maintainer_permissions) expect_allowed(*owner_permissions) end + + it_behaves_like 'deploy token does not get confused with user' do + let(:user_id) { owner.id } + end end context 'admin' do @@ -166,6 +204,14 @@ RSpec.describe GroupPolicy do context 'with admin mode', :enable_admin_mode do specify { expect_allowed(*admin_permissions) } end + + it_behaves_like 'deploy token does not get confused with user' do + let(:user_id) { admin.id } + + context 'with admin mode', :enable_admin_mode do + it { expect_disallowed(*admin_permissions) } + end + end end describe 'private nested group use the highest access level from the group and inherited permissions' do diff --git a/spec/requests/api/maven_packages_spec.rb b/spec/requests/api/maven_packages_spec.rb index 189d6a4c1a..b935130854 100644 --- a/spec/requests/api/maven_packages_spec.rb +++ b/spec/requests/api/maven_packages_spec.rb @@ -193,6 +193,24 @@ RSpec.describe API::MavenPackages do it_behaves_like 'downloads with a job token' it_behaves_like 'downloads with a deploy token' + + it 'does not allow download by a unauthorized deploy token with same id as a user with access' do + unauthorized_deploy_token = create(:deploy_token, read_package_registry: true, write_package_registry: true) + + another_user = create(:user) + project.add_developer(another_user) + + # We force the id of the deploy token and the user to be the same + unauthorized_deploy_token.update!(id: another_user.id) + + download_file( + package_file.file_name, + {}, + Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => unauthorized_deploy_token.token + ) + + expect(response).to have_gitlab_http_status(:forbidden) + end end context 'project name is different from a package name' do @@ -451,6 +469,20 @@ RSpec.describe API::MavenPackages do expect(response).to have_gitlab_http_status(:ok) end + it 'rejects requests by a unauthorized deploy token with same id as a user with access' do + unauthorized_deploy_token = create(:deploy_token, read_package_registry: true, write_package_registry: true) + + another_user = create(:user) + project.add_developer(another_user) + + # We force the id of the deploy token and the user to be the same + unauthorized_deploy_token.update!(id: another_user.id) + + authorize_upload({}, headers.merge(Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => unauthorized_deploy_token.token)) + + expect(response).to have_gitlab_http_status(:forbidden) + end + def authorize_upload(params = {}, request_headers = headers) put api("/projects/#{project.id}/packages/maven/com/example/my-app/#{version}/maven-metadata.xml/authorize"), params: params, headers: request_headers end @@ -538,6 +570,20 @@ RSpec.describe API::MavenPackages do expect(response).to have_gitlab_http_status(:ok) end + it 'rejects uploads by a unauthorized deploy token with same id as a user with access' do + unauthorized_deploy_token = create(:deploy_token, read_package_registry: true, write_package_registry: true) + + another_user = create(:user) + project.add_developer(another_user) + + # We force the id of the deploy token and the user to be the same + unauthorized_deploy_token.update!(id: another_user.id) + + upload_file(params, headers.merge(Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => unauthorized_deploy_token.token)) + + expect(response).to have_gitlab_http_status(:forbidden) + end + context 'version is not correct' do let(:version) { '$%123' } diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index f7771c7b0f..082857ab73 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -547,12 +547,6 @@ RSpec.describe 'Git LFS API and storage' do project.lfs_objects << lfs_object end - context 'when Deploy Token is valid' do - let(:deploy_token) { create(:deploy_token, projects: [project]) } - - it_behaves_like 'an authorized request', renew_authorization: false - end - context 'when Deploy Token is not valid' do let(:deploy_token) { create(:deploy_token, projects: [project], read_repository: false) } @@ -562,7 +556,14 @@ RSpec.describe 'Git LFS API and storage' do context 'when Deploy Token is not related to the project' do let(:deploy_token) { create(:deploy_token, projects: [other_project]) } - it_behaves_like 'LFS http 404 response' + it_behaves_like 'LFS http 401 response' + end + + # TODO: We should fix this test case that causes flakyness by alternating the result of the above test cases. + context 'when Deploy Token is valid' do + let(:deploy_token) { create(:deploy_token, projects: [project]) } + + it_behaves_like 'an authorized request', renew_authorization: false end end diff --git a/spec/support/shared_examples/policies/project_policy_shared_examples.rb b/spec/support/shared_examples/policies/project_policy_shared_examples.rb index f8526ec68d..df8e4bc96d 100644 --- a/spec/support/shared_examples/policies/project_policy_shared_examples.rb +++ b/spec/support/shared_examples/policies/project_policy_shared_examples.rb @@ -97,6 +97,28 @@ RSpec.shared_examples 'project policies as anonymous' do end end +RSpec.shared_examples 'deploy token does not get confused with user' do + before do + deploy_token.update!(id: user_id) + + # Project with public builds are available to all + project.update!(public_builds: false) + end + + let(:deploy_token) { create(:deploy_token) } + + subject { described_class.new(deploy_token, project) } + + it do + expect_disallowed(*guest_permissions) + expect_disallowed(*reporter_permissions) + expect_disallowed(*team_member_reporter_permissions) + expect_disallowed(*developer_permissions) + expect_disallowed(*maintainer_permissions) + expect_disallowed(*owner_permissions) + end +end + RSpec.shared_examples 'project policies as guest' do subject { described_class.new(guest, project) } @@ -115,6 +137,10 @@ RSpec.shared_examples 'project policies as guest' do expect_disallowed(*owner_permissions) end + it_behaves_like 'deploy token does not get confused with user' do + let(:user_id) { guest.id } + end + it_behaves_like 'archived project policies' do let(:regular_abilities) { guest_permissions } end @@ -128,7 +154,7 @@ RSpec.shared_examples 'project policies as guest' do context 'when public builds disabled' do before do - project.update(public_builds: false) + project.update!(public_builds: false) end it do @@ -139,7 +165,7 @@ RSpec.shared_examples 'project policies as guest' do context 'when builds are disabled' do before do - project.project_feature.update(builds_access_level: ProjectFeature::DISABLED) + project.project_feature.update!(builds_access_level: ProjectFeature::DISABLED) end it do @@ -165,6 +191,10 @@ RSpec.shared_examples 'project policies as reporter' do expect_disallowed(*owner_permissions) end + it_behaves_like 'deploy token does not get confused with user' do + let(:user_id) { reporter.id } + end + it_behaves_like 'archived project policies' do let(:regular_abilities) { reporter_permissions } end @@ -186,6 +216,10 @@ RSpec.shared_examples 'project policies as developer' do expect_disallowed(*owner_permissions) end + it_behaves_like 'deploy token does not get confused with user' do + let(:user_id) { developer.id } + end + it_behaves_like 'archived project policies' do let(:regular_abilities) { developer_permissions } end @@ -207,6 +241,10 @@ RSpec.shared_examples 'project policies as maintainer' do expect_disallowed(*owner_permissions) end + it_behaves_like 'deploy token does not get confused with user' do + let(:user_id) { maintainer.id } + end + it_behaves_like 'archived project policies' do let(:regular_abilities) { maintainer_permissions } end @@ -228,6 +266,10 @@ RSpec.shared_examples 'project policies as owner' do expect_allowed(*owner_permissions) end + it_behaves_like 'deploy token does not get confused with user' do + let(:user_id) { owner.id } + end + it_behaves_like 'archived project policies' do let(:regular_abilities) { owner_permissions } end @@ -249,6 +291,28 @@ RSpec.shared_examples 'project policies as admin with admin mode' do expect_allowed(*owner_permissions) end + context 'deploy token does not get confused with user' do + before do + allow(deploy_token).to receive(:id).and_return(admin.id) + + # Project with public builds are available to all + project.update!(public_builds: false) + end + + let(:deploy_token) { create(:deploy_token) } + + subject { described_class.new(deploy_token, project) } + + it do + expect_disallowed(*guest_permissions) + expect_disallowed(*reporter_permissions) + expect_disallowed(*team_member_reporter_permissions) + expect_disallowed(*developer_permissions) + expect_disallowed(*maintainer_permissions) + expect_disallowed(*owner_permissions) + end + end + it_behaves_like 'archived project policies' do let(:regular_abilities) { owner_permissions } end @@ -268,5 +332,20 @@ RSpec.shared_examples 'project policies as admin without admin mode' do subject { described_class.new(admin, project) } it { is_expected.to be_banned } + + context 'deploy token does not get confused with user' do + before do + allow(deploy_token).to receive(:id).and_return(admin.id) + + # Project with public builds are available to all + project.update!(public_builds: false) + end + + let(:deploy_token) { create(:deploy_token) } + + subject { described_class.new(deploy_token, project) } + + it { is_expected.to be_banned } + end end end