Project maintainers can bypass group's scan result policy `block_branch_modification` setting
HackerOne report #2280292 by js_noob
on 2023-12-10, assigned to @cmaxim:
Report | Attachments | How To Reproduce
Report
Summary
Hello team, group owners can create scan result policies with the block_branch_modification
setting, which blocks all project owners/maintainers from removing a branch from the protected branches list. From the docs
When enabled, prevents a user from removing a branch from the protected branches list, deleting a protected branch, or changing the default branch if that branch is included in the security policy. This ensures users cannot remove protection status from a branch to merge vulnerable code.
However, this is bypassable.
Steps to reproduce
As a group owner:
- Create a new group, and apply the ultimate trial to it
- Navigate to https://gitlab.com/groups/GROUP/-/security/policies/new?type=scan_result_policy, and create a policy using the following YAML code
name: Test Policy
description: ''
enabled: true
actions:
- type: require_approval
approvals_required: 1
role_approvers:
- owner
rules:
- type: any_merge_request
branch_type: protected
commits: any
approval_settings:
block_branch_modification: true
prevent_pushing_and_force_pushing: true
prevent_approval_by_author: true
prevent_approval_by_commit_author: true
remove_approvals_with_new_commit: true
require_password_to_approve: false
- Create a project in that group
- Invite a maintainer to that project (not to the group)
As the project maintainer:
- Verify that you can't push code to the
main
branch - Send the following API request and save the ID of the returned rule for the
main
branch
curl --header "PRIVATE-TOKEN: PAT" "https://gitlab.com/api/v4/projects/PROJECT_ID/protected_branches"
- Send the following request
PATCH /GROUP/PROJECT/-/protected_branches/ID_FROM_STEP_6 HTTP/2
Host: gitlab.com
Cookie: REDACTED
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:120.0) Gecko/20100101 Firefox/120.0
Accept: application/json, text/plain, */*
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate, br
Referer: https://gitlab.com/my-new-nice-group-180/karina/-/settings/repository
X-Csrf-Token: Vo1A1ipTo4K9PMvS9_qAAclJP2UBOfw6n5hhL8-USqpoquf1JgvT6TgFisVPIpeaLnWBT9zT84pvSjAL5t4cFg
X-Requested-With: XMLHttpRequest
Content-Type: application/json
Sentry-Trace: 86e3cfc2a9a64cbd8f32dc9c5720eca1-a776e8af4b08a74e-1
Baggage: sentry-environment=gprd,sentry-release=e627abfc601,sentry-public_key=f5573e26de8f4293b285e556c35dfd6e,sentry-trace_id=86e3cfc2a9a64cbd8f32dc9c5720eca1
Content-Length: 69
Origin: https://gitlab.com
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: same-origin
Te: trailers
{"protected_branch":{"name":"messed_up","allow_force_push":true}}
- Navigate to https://gitlab.com/GROUP/PROJECT/-/settings/repository, and verify that the protection on the
main
is removed - Verify that you can push code to the
main
branch
POC
Screen_Recording_2023-12-10_at_10.38.30_PM.mov
What is the current bug behavior?
Request to update a protected branch allows branch name updates.
What is the expected correct behavior?
Request to update a protected branch shouldn't allow branch name updates.
Output of checks
This bug happens on GitLab.com
Impact
Users can remove protection status from a branch to merge vulnerable code.
Attachments
Warning: Attachments received through HackerOne, please exercise caution!
How To Reproduce
Please add reproducibility information to this section:
Potential Fixes
-
prevent name change of protected branch (both via the REST API and the GraphQL Mutation) (partially re implementing Let scan result policies prevent changes to pro... (!130653 - merged) for the name
field inProtectedBranches::UpdateService
)
Implementation Plan
diff --git a/ee/app/models/security/scan_result_policy_read.rb b/ee/app/models/security/scan_result_policy_read.rb
index fa1faba08e5e..7b01078feab3 100644
--- a/ee/app/models/security/scan_result_policy_read.rb
+++ b/ee/app/models/security/scan_result_policy_read.rb
@@ -31,6 +31,9 @@ class ScanResultPolicyRead < ApplicationRecord
scope :for_project, ->(project) { where(project: project) }
scope :targeting_commits, -> { where.not(commits: nil) }
scope :including_approval_merge_request_rules, -> { includes(:approval_merge_request_rules) }
+ scope :blocking_branch_modification, -> do
+ where("project_approval_settings->>'block_branch_modification' = 'true'")
+ end
def newly_detected?
license_states.include?(ApprovalProjectRule::NEWLY_DETECTED)
diff --git a/ee/app/services/ee/protected_branches/renaming_blocked_by_policy.rb b/ee/app/services/ee/protected_branches/renaming_blocked_by_policy.rb
new file mode 100644
index 000000000000..ad34fa7d1072
--- /dev/null
+++ b/ee/app/services/ee/protected_branches/renaming_blocked_by_policy.rb
@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+module EE
+ module ProtectedBranches
+ module RenamingBlockedByPolicy
+ def execute(protected_branch)
+ raise ::Gitlab::Access::AccessDeniedError if renaming? && blocked?(protected_branch)
+
+ super
+ end
+
+ private
+
+ def renaming?
+ params.key?(:name)
+ end
+
+ def blocked?(protected_branch)
+ return blocking_branch_modification?(protected_branch.project) if protected_branch.project_level?
+
+ blocking_group_branch_modification?(protected_branch.group)
+ end
+
+ def blocking_branch_modification?(project)
+ return false unless project&.licensed_feature_available?(:security_orchestration_policies)
+ return false unless ::Feature.enabled?(:scan_result_policies_block_unprotecting_branches, project)
+
+ project.scan_result_policy_reads.blocking_branch_modification.exists?
+ end
+
+ def blocking_group_branch_modification?(group)
+ return false unless group&.licensed_feature_available?(:security_orchestration_policies)
+ return false unless ::Feature.enabled?(:scan_result_policy_block_group_branch_modification, group)
+
+ ::Security::SecurityOrchestrationPolicies::GroupProtectedBranchesDeletionCheckService
+ .new(group: group)
+ .execute
+ end
+ end
+ end
+end
diff --git a/ee/app/services/ee/protected_branches/update_service.rb b/ee/app/services/ee/protected_branches/update_service.rb
index 86d5f8c5f638..c79418d1f223 100644
--- a/ee/app/services/ee/protected_branches/update_service.rb
+++ b/ee/app/services/ee/protected_branches/update_service.rb
@@ -3,6 +3,8 @@
module EE
module ProtectedBranches
module UpdateService
+ prepend RenamingBlockedByPolicy
+
def after_execute(protected_branch:, old_merge_access_levels:, old_push_access_levels:)
super