"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
andguest01
- Log in as
owner01
and create a public projectproject01
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 toexternal 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 itfork01
. - 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 originalproject01
- 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 theguest01
user is NOT a member ofproject01
) - 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 andguest01
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!