Project maintainers can bypass group's merge request approval policy `block_branch_modification` setting
HackerOne report #2520947 by js_noob
on 2024-05-26, 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 pushing code to a branch directly, unless approved. 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.
NB: This report is very similar to #2280292 (same impact but different scenario), however, I realized that the corresponding GitLab issue is tagged with severity 2 while I was paid as a medium, so I believe this should be high as well.
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=approval_policy, and create a policy while having the same options as the following, but of course replace the user with the owner
3. Create a project in that group
4. 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 - Create a new branch with some code, create an MR to the
main
branch, and verify that it needs approval - Navigate to https://gitlab.com/-/graphql-explorer, run the following query, and grab the approval rule ID (has the form of
id://gitlab/ApprovalProjectRule/ID
query Project {
project(fullPath: "test-group-12891821/1") {
id
namespace {
id
}
branchRules {
nodes {
id
isDefault
isProtected
name
approvalRules {
nodes {
id
name
approvalsRequired
}
}
}
}
}
}
- Run the following mutation, using the ID grabbed above
mutation ApprovalRuleDelete {
approvalProjectRuleDelete(
input: {id: "ID_FROM_STEP_7"}
) {
approvalRule {
id
}
}
}
- Verify that the MR created doesn't need any approval
POC
Screen_Recording_2024-05-26_at_7.31.37_PM.mov
What is the current bug behavior?
Users can delete approval rules associated with security policies.
What is the expected correct behavior?
Users shouldn't be able to delete approval rules associated with security policies.
Output of checks
This bug happens on GitLab.com
Impact
Users can push unapproved code to protected branches, ultimately allowing project maintainers to disclose group's CI/CD variables.
Attachments
Warning: Attachments received through HackerOne, please exercise caution!
How To Reproduce
Please add reproducibility information to this section:
Implementation Plan
diff --git a/ee/app/graphql/mutations/approval_project_rules/delete.rb b/ee/app/graphql/mutations/approval_project_rules/delete.rb
index 3c5fb4e44db4..03cd7220d158 100644
--- a/ee/app/graphql/mutations/approval_project_rules/delete.rb
+++ b/ee/app/graphql/mutations/approval_project_rules/delete.rb
@@ -24,6 +24,8 @@ def resolve(id:)
approval_rule: (approval_rule if result.success?),
errors: approval_rule.errors.full_messages
}
+ rescue Gitlab::Access::AccessDeniedError
+ raise_resource_not_available_error!
end
end
end
diff --git a/ee/app/services/approval_rules/project_rule_destroy_service.rb b/ee/app/services/approval_rules/project_rule_destroy_service.rb
index 854f843c9f71..74dd64ed8e8b 100644
--- a/ee/app/services/approval_rules/project_rule_destroy_service.rb
+++ b/ee/app/services/approval_rules/project_rule_destroy_service.rb
@@ -11,6 +11,8 @@ def initialize(approval_rule, current_user)
end
def execute
+ raise Gitlab::Access::AccessDeniedError if originates_from_security_policy?
+
ApplicationRecord.transaction do
# Removes only MR rules associated with project rule
remove_associated_approval_rules_from_unmerged_merge_requests
@@ -23,6 +25,10 @@ def execute
private
+ def originates_from_security_policy?
+ rule.security_orchestration_policy_configuration_id?
+ end
+
def success
audit_deletion