Project maintainers can bypass group's scan result 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 #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

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!

Screen_Recording_2023-12-10_at_10.38.30_PM.mov

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 in ProtectedBranches::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

Edited Jan 18, 2024 by Dominic Bauer

Project maintainers can bypass group's scan result policy block_branch_modification setting