Skip to content

Password bypass on approvals using policy projects

⚠ 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 #2486223 by vexin on 2024-05-02, assigned to @kmorrison1:

Report | Attachments | How To Reproduce

Report

NOTE! Thanks for submitting a report! Please note that initial triage is handled by HackerOne staff. They are identified with a HackerOne triage badge and will escalate to the GitLab team any. Please replace all the (parenthesized) sections below with the pertinent details. Remember, the more detail you provide, the easier it is for us to triage and respond quickly, so be sure to take your time filling out the report!

Summary

When using GitLab policy projects to define approval rules, defining the require_password_to_approve setting as true will cause approvals to appear as though they require a password to approve, but will allow any password or a commented quick action to approve.

Steps to reproduce
  1. A GitLab group or subgroup with two projects and two users
  2. Designate one project as the policy project, where you will store the GitLab policy configuration
  3. In the policy project, add a merge request approval policy following the schema defined in https://docs.gitlab.com/ee/user/application_security/policies/scan-result-policies.html - include a rule of type any_merge_request that targets a desired branch (e.g. main), also include an action of type require_approval that requires some users, groups, or roles that may approve. Include an approval_settings block for the policy that sets require_password_to_approve to true. This policy should be committed to a file like .gitlab/security-policies/policy.yml

Full example policy included:

---  
approval_policy:  
  - name: Example GR Short  
    description: Example Group Reviews Short description  
    enabled: true  
    rules:  
      - type: any_merge_request  
        branches:  
          - main  
        commits: any  
    actions:  
      - type: require_approval  
        approvals_required: 1  
        role_approvers:  
          - owner  
          - maintainer  
          - developer  
    approval_settings:  
      block_branch_modification: true  
      prevent_pushing_and_force_pushing: true  
      prevent_approval_by_author: true  
      prevent_approval_by_commit_author: false  
      remove_approvals_with_new_commit: true  
      require_password_to_approve: true
  1. In the other project, create a branch with some example commit. Push that branch and create a Merge Request. Using another account that fits into some category of the approvers from the policy above, review the Merge Request. GitLab should say X approvals required, and when you click approve, it will pop up with a modal to enter your password to confirm the approval. Enter some value that is not your password, and click approve on the modal. The approval should go through.
  2. Additionally, if you revoke your approval of the merge request using this secondary account, you can also see that approving the merge request via the quick action /approve in a comment will also approve the merge request without requiring a password.
  3. Overriding the policy by manually enabling the Require user re-authentication (password or SAML) to approve option in the project's Merge Request Settings will cause the approval modal to validate the password appropriately, and blocking the /approve quick action.
Impact

When a project or group is using Merge Request approval policies, a compromised session may allow an attacker to approve Merge Requests without re-authentication as defined by the policy. This can result in merges and CI/CD execution without the required re-authentication security.

Examples

Example project with merge request https://gitlab.com/tbtesting/Test
Example policy project: https://gitlab.com/tbtesting/policy

What is the current bug behavior?

When using the require_password_to_approve approval setting, the password re-authentication modal appears as though it were re-authenticating for approval, but the modal accepts any string, empty or otherwise as a password and successfully approves the merge request.
image.png
image.png

What is the expected correct behavior?

The approval should be rejected when an invalid password is entered.

Relevant logs and/or screenshots

Error is fully encapsulated within the test projects here, and screenshots above https://gitlab.com/tbtesting

Output of checks

This bug happens on GitLab.com

Results of GitLab environment info

Not applicable

Impact

An attacker can approve merge requests without re-authentication, so in the case of a compromised session, they could trigger merges and downstream CI/CD runs.

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/services/ee/merge_requests/approval_service.rb b/ee/app/services/ee/merge_requests/approval_service.rb
index d7df01f7d8fe..9dc6e761ea3f 100644
--- a/ee/app/services/ee/merge_requests/approval_service.rb
+++ b/ee/app/services/ee/merge_requests/approval_service.rb
@@ -91,6 +91,7 @@ def saml_approval_in_time?
       end
 
       def mr_approval_setting_password_required?(merge_request)
+        return true if merge_request.require_password_to_approve?
         return false unless root_group.is_a? Group
 
         ComplianceManagement::MergeRequestApprovalSettings::Resolver
Edited by Dominic Bauer