"External status checks" can be accepted by users below developer access if the user is either author or assignee of the target merge request

HackerOne report #1375393 by joaxcar on 2021-10-20, assigned to @nmalcolm:

Video Demo: https://www.youtube.com/watch?v=aSUaEPTf-rY

Report

Summary

Any user who is either author or assignee of a merge request can approve that merge request's external status checks. This includes users with Guest access that creates MR's either through email or through a fork of the project. It also includes users with Guest or Reporter access getting assigned to an MR, which is not uncommon in public projects.

There exists a tiny overlap with my report 1375376 which is yet not triaged. I describe this overlap in the end of this summary. The reports look similar, but the vulnerabilities are not related. A fix in 1375376 would not fix this report, only the overlap.

The external status check documentation does not offer too much information about how the feature is supposed to function. But the developer discussions and the unit tests suggests that approving an external status check should be restricted for users with at least Developer access in the project. Here is the issue tracking the development #267519 (closed)

In this thread the possibility of users abusing the fact that a status check is not tied to any special token. Rather they use regular PAT's, these discussion mentions

find_merge_request_with_access will at least mean that only those with developer+ access to the project in question would be able to exploit the feature in this way.

The unit tests for this feature checks this assumption with these lines

describe 'permissions' do  
    before do  
      stub_licensed_features(external_status_checks: true)  
    end

    it { expect { subject }.to be_allowed_for(:maintainer).of(project) }  
    it { expect { subject }.to be_allowed_for(:developer).of(project) }  
    it { expect { subject }.to be_denied_for(:reporter).of(project) }  
  end

Validating if the user making the request is developer+.

So to enforce this they have put an authentication block checking if the user have permission to respond to external status checks using the function called find_merge_request_with_access in this way

merge_request = find_merge_request_with_access(params[:merge_request_iid], :approve_merge_request)

Checking the permission :approve_merge_request which is enabled for developers. But as it turns out, this permission is also enabled for users with the permission :update_merge_request. In https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/policies/merge_request_policy.rb there is this rule

 rule { can?(:update_merge_request) }.policy do  
    enable :approve_merge_request  
  end

That enables the permission for anyone that are allowed to update the MR. And in https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/policies/issuable_policy.rb there exists this rule

rule { can?(:guest_access) & assignee_or_author }.policy do  
    enable :read_issue  
    enable :update_issue  
    enable :reopen_issue  
    enable :read_merge_request  
    enable :update_merge_request  
    enable :reopen_merge_request  
  end

enabling :update_merge_request for anyone that have :guest_access and is either assignee or author.

This is probably the root of the problem. And as far as I could make out this is not the intended behavior. A user with Guest access can create an MR by forking and directly send approval for all external status checks to lure the developers that the MR have been checked. It leads to at least two problems:

A user with no membership can create a MR in a public project and then "approve" the `external status check's without any membership
A user who is demoted to Reporter in a private project can still "approve" `external status check's in MR's where the user is either author or assignee

and at the moment thanks to the vulnerability that I have reported in 1375376 at present it is also possible to:

A user who is demoted to Guest in a private project can still "approve" `external status check's in MR's where the user is either author or assignee while not being able to actually view the MR

Steps to reproduce

External status checks is an Ultimate feature, so make sure the project is created in such an environment

Create two users owner01 and guest01
Log in as owner01 and create a public project project01 by visiting https://gitlab.com/projects/new#blank_project and take a note of the project ID
Go to the project settings page and expand the tab merge requests and scroll down to external status checks, settings page https://gitlab.com/owner01/project01/edit
Create a status check with any name and endpoint, and leave the
Log out and log in as guest01
Go to the project page https://gitlab.com/owner01/project01 and create a fork with the fork button, call it fork01.
When the fork is created, create a new branch in the fork https://gitlab.com/guest01/fork01/-/branches/new called new_branch
When the fork is created directly click on the option "create a merge request", in the "New merge request" page click Change branches and select the target branch as any branch on the original project01
Click "Create" and a new MR should be created in project01 (this is a guest contribution and a normal open-source flow, but note that the guest01 user is NOT a member of project01)
Go to https://gitlab.com/-/profile/personal_access_tokens and create an access token for the API for guest01
Open a terminal and make this request to get the ID of the status check (user project01 ID and MR IID which is probably 1 and guest01 token), take a note of the returned ID of the status check

curl "https://gitlab.joaxcar.com/api/v4/projects/<PROJECT_ID>/merge_requests/<MR_IID>/status_checks" -H "Authorization: Bearer <TOKEN>"

Send this request to check for the SHA, the request will fail with a message telling you which SHA to use, in this request we use a dummy SHA=a (make sure to also replace CHECK_ID to the found ID from step 12)

curl --request POST \  
  --url 'https://gitlab.com/api/v4/projects/<PROJECT_ID>/merge_requests/<MR_IID>/status_check_responses?sha=a&external_status_check_id=<CHECK_ID>' \  
  --header 'Authorization: Bearer <TOKEN>'

Now use the returned SHA in this request to finally "approve" the status check for the MR

curl --request POST \  
  --url 'https://gitlab.domain.com/api/v4/projects/<PROJECT_ID>/merge_requests/<MR_IID>/status_check_responses?sha=<SHA>&external_status_check_id=<CHECK_ID>' \  
  --header 'Authorization: Bearer <TOKEN>'

Go to the MR page and verify that the status check is now green and checked, https://gitlab.com/owner01/project01/-/merge_requests/1

Impact

A Guest user can send acknowledge messages to "approve" external status checks on MR's where the user is either author or assignee. This makes it possible for a malicious user to "spoof" acceptance of MR's in projects where the user should not be able to do this. In public projects this mean that any guest contribution from non-members can have its external status checks checked by the author itself even if not a member of the project.

What is the current bug behavior?

Users with access level below Developer can accept external status checks if they are either author or assignee of the MR

What is the expected correct behavior?

Only Developer+ users that are members of the project should be able to user their PAT to "approve" the external status check

Output of checks

This bug happens on GitLab.com

Impact

A Guest (or Reporter) user can send acknowledge messages to "approve" external status checks on MR's where the user is either author or assignee. This makes it possible for a malicious user to "spoof" acceptance of MR's in projects where the user should not be able to do this. In public projects this mean that any guest contribution from non-members can have its external status checks checked by the author itself even if not a member of the project.

How To Reproduce

Please add reproducibility information to this section:

See above!

Edited Oct 22, 2021 by Nick Malcolm