Project maintainers can bypass group's merge request approval policy `block_branch_modification` setting

⚠ Please read the process on how to fix security issues before starting to work on the issue. Vulnerabilities must be fixed in a security mirror.

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:

1. Create a new group, and apply the ultimate trial to it
2. 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:

5. Verify that you can't push code to the main branch
6. Create a new branch with some code, create an MR to the main branch, and verify that it needs approval
7. 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  
          }  
        }  
      }  
    }  
  }  
}

8. Run the following mutation, using the ID grabbed above

mutation ApprovalRuleDelete {  
  approvalProjectRuleDelete(  
    input: {id: "ID_FROM_STEP_7"}  
  ) {  
    approvalRule {  
      id  
    }  
  }  
}

9. 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!

• Screen_Recording_2024-05-26_at_7.31.37_PM.mov
• Screenshot_2024-05-26_at_7.39.50_PM.png

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