ESCALATED: 2FA not enforced on /profile/applications

HackerOne report #691477 by brdoors3 on 2019-09-10, assigned to hackerjuan:

Hi team,

I found one access control related to your feature 'Enforcing 2FA for all users' on https://gitlab.com/

POC

1 access one group and enforce the 2FA for all users

In this test I used one grace period of 1 hour

2 after 1 hour access again https://gitlab.com

The user will be redirected to https://gitlab.com/profile/two_factor_auth

3 try to access some other areas

All pages redirect to https://gitlab.com/profile/two_factor_auth (expected behavior)

4 access https://gitlab.com/profile/applications

The 2FA is not enforced on this page. I can create and delete applications without any 2FA active

POC video attached

Impact

In this scenario after grace period has elapsed one user can access and make any change on https://gitlab.com/profile/applications

Attachments

Warning: Attachments received through HackerOne, please exercise caution!

• gitlab.PNG
• gitlab_poc.mp4

added HackerOne priority2 security severity2 labels

Confirmed on GitLab 12.2.5.

This is issue is present because the /profile/applications route points to Oauth::ApplicationsController, which inherits from Doorkeeper::ApplicationsController instead of inheriting from Profiles::ApplicationController or including EnforcesTwoFactorAuthentication. Because of this this controller may be vulnerable to other authorisation issues.

added devopsmanage label

added groupauthentication and authorization [DEPRECATED] label

/cc @jeremy @lmcandrew

changed due date to October 23, 2019

changed title from {-Access control - "Enforcing 2FA for all users" in one group on https://gitlab.com/-} to 2FA not enforced on /profile/applications

@lmcandrew, do you agree that this is an ~S2?

Sorry @jeremy, I mislabeled this issue, it isn't a ~S2 but a ~S3

added security-set-milestone label

changed milestone to %12.5

removed security-set-milestone label

added [deprecated] Accepting merge requests label

added priority3 severity3 labels and removed priority2 severity2 labels

changed due date to November 23, 2019

added security-issue-escalated label

changed title from 2FA not enforced on /profile/applications to ESCALATED: 2FA not enforced on /profile/applications

@jeremy This security issue is overdue.

Please make sure that a new milestone is assigned within 7 business day(s) and provide some context, if possible. Thanks!

More information: Escalation Engine Workflow

@ebrinkman @jeremy This security issue is overdue.

Please make sure that a new milestone is assigned within 7 business day(s) and provide some context, if possible. Thanks!

More information: Escalation Engine Workflow

Setting ~"Category:Authentication and Authorization" based on ~"group::access".

Setting ~"Category:Authentication and Authorization" based on ~"group::access".

Setting ~"Category:Authentication and Authorization" based on ~"group::access".

added 1 deleted label

@ebrinkman @jeremy This security issue is overdue.

Please make sure that a new milestone is assigned within 7 business day(s) and provide some context, if possible. Thanks!

More information: Escalation Engine Workflow

@ebrinkman @jeremy This security issue is overdue.

Please make sure that a new milestone is assigned within 7 business day(s) and provide some context, if possible. Thanks!

More information: Escalation Engine Workflow

@ebrinkman @jeremy This security issue is overdue.

Please make sure that a new milestone is assigned within 7 business day(s) and provide some context, if possible. Thanks!

More information: Escalation Engine Workflow

@ebrinkman @jeremy This security issue is overdue.

Please make sure that a new milestone is assigned within 7 business day(s) and provide some context, if possible. Thanks!

More information: Escalation Engine Workflow

@ebrinkman @jeremy This security issue is overdue.

Please make sure that a new milestone is assigned within 7 business day(s) and provide some context, if possible. Thanks!

More information: Escalation Engine Workflow

assigned to @mksionek

removed [deprecated] Accepting merge requests label

@gitlab-com/gl-security/appsec I'm wondering if we also need to enforce 2FA for the other Doorkeeper controllers:

• app/controllers/oauth/authorizations_controller.rb
• app/controllers/oauth/authorized_applications_controller.rb
• app/controllers/oauth/token_info_controller.rb

I think the answer is yes but I'm not really sure, especially the TokenInfoController which doesn't seem to require an active user session.

@toupeira I just realized that this could be what is described at #222879 (closed) or that #222879 (closed) is a duplicate of this issue

it is!

@jeremymatos @mksionek hmm does that mean we should fix them both in the same MR (or at least, the same security release) to avoid disclosing details?

I saw we also have routes for the Doorkeeper::TokensController which we don't override at all in our app. AFAIK the create action there can also be used to generate access tokens with some of the other OAuth flows.

BTW there might be a way to handle this centrally in either https://gitlab.com/gitlab-org/gitlab/blob/f9ef6bc7154711758464c1996f058539c1228807/config/initializers/doorkeeper.rb#L7 or by overriding https://github.com/doorkeeper-gem/doorkeeper/blob/f387b101e8144f407969424393b975879dbd841f/lib/doorkeeper/helpers/controller.rb#L14. But for now I think just adding the EnforcesTwoFactorAuthentication concern and adding our own subclass for Doorkeeper::TokensController should be sufficient too.

For reference, here are all the routes defined by the doorkeeper gem:

   native_oauth_authorization GET    /oauth/authorize/native(.:format)            oauth/authorizations#show
          oauth_authorization GET    /oauth/authorize(.:format)                   oauth/authorizations#new
                              DELETE /oauth/authorize(.:format)                   oauth/authorizations#destroy
                              POST   /oauth/authorize(.:format)                   oauth/authorizations#create
                  oauth_token POST   /oauth/token(.:format)                       doorkeeper/tokens#create
                 oauth_revoke POST   /oauth/revoke(.:format)                      doorkeeper/tokens#revoke
             oauth_introspect POST   /oauth/introspect(.:format)                  doorkeeper/tokens#introspect
           oauth_applications GET    /oauth/applications(.:format)                oauth/applications#index
                              POST   /oauth/applications(.:format)                oauth/applications#create
        new_oauth_application GET    /oauth/applications/new(.:format)            oauth/applications#new
       edit_oauth_application GET    /oauth/applications/:id/edit(.:format)       oauth/applications#edit
            oauth_application GET    /oauth/applications/:id(.:format)            oauth/applications#show
                              PATCH  /oauth/applications/:id(.:format)            oauth/applications#update
                              PUT    /oauth/applications/:id(.:format)            oauth/applications#update
                              DELETE /oauth/applications/:id(.:format)            oauth/applications#destroy
oauth_authorized_applications GET    /oauth/authorized_applications(.:format)     oauth/authorized_applications#index
 oauth_authorized_application DELETE /oauth/authorized_applications/:id(.:format) oauth/authorized_applications#destroy
             oauth_token_info GET    /oauth/token/info(.:format)                  oauth/token_info#show

Most of these actions sound like they need a 2FA check, but I'm unsure about these:

• oauth/token_info#show
• doorkeeper/tokens#introspect

The doorkeeper-openid_connect gem also adds these routes:

      oauth_userinfo GET  /oauth/userinfo(.:format)       doorkeeper/openid_connect/userinfo#show
                     POST /oauth/userinfo(.:format)       doorkeeper/openid_connect/userinfo#show
oauth_discovery_keys GET  /oauth/discovery/keys(.:format) doorkeeper/openid_connect/discovery#keys

The discovery endpoint is public and should be fine, and the userinfo endpoints only work with a valid access token so it sounds similar to the token_info and introspect endpoints from Doorkeeper.

@toupeira Yes it is recommended to fix it in the same MR. 2 different MRs in the same security release would work also, but it could cause merge conflicts.

I don't know enough the usage of each route to know if 2FA should be checked on all of those, but yes the safe choice is by default to protect all of them with 2FA. And only deactivate it for the routes that should not have 2FA. I'm looping in @dblessing because he recently had to look for OAuth routes when fixing another security issue.

Ok, what I did was:

I've found out that we implement base controller for Doorkeeper: https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/base_doorkeeper_controller.rb (how it works: https://github.com/doorkeeper-gem/doorkeeper/pull/845/files). Token-related controllers do not inherit from it, as their base class is Doorkeeper::ApplicationMetalController as here or here. What do you think? @toupeira @jeremymatos

@mksionek ah great find!

In that case I think we can just add EnforcesTwoFactorAuthentication to that base controller.

And I see now they later also added a base_metal_controller setting in https://github.com/doorkeeper-gem/doorkeeper/pull/1283, so maybe we can just specify the same Gitlab::BaseDoorkeeperController there? Not really familiar with the metal controller stuff in Rails, but AFAIK it's just a performance optimization if you don't want to use the full Rails stack?

Or I guess we could also just create a Gitlab::BaseMetalDoorkeeperController.

Regarding @jeremymatos's point:

I don't know enough the usage of each route to know if 2FA should be checked on all of those, but yes the safe choice is by default to protect all of them with 2FA. And only deactivate it for the routes that should not have 2FA. I'm looping in @dblessing because he recently had to look for OAuth routes when fixing another security issue.

I realize now that the redirect only happens with an active user session anyway, so I think enabling it on all actions should be mostly fine: https://gitlab.com/gitlab-org/gitlab/blob/f9ef6bc7154711758464c1996f058539c1228807/app/controllers/concerns/enforces_two_factor_authentication.rb#L30

There might still be some edge cases, but as it would only affect users with a 2FA requirement that don't have 2FA enabled, it's probably not really a big deal.

@toupeira that makes sense, as long as those users can setup their 2FA it should be fine ;)

@toupeira the PR you have linked was included in 5.2.0 version .

We are in 5.0.3... do we need to upgrade?

@mksionek oh no, sorry I didn't see that!

Upgrading would definitely be nice as I see some CVEs have been fixed since 5.0.3, but I also see a couple IMPORTANT notes that might need some more scrutiny: https://github.com/doorkeeper-gem/doorkeeper/blob/master/CHANGELOG.md

As an alternative, maybe we could just add the concern to our Oauth::TokenInfoController, and add a subclass for Doorkeeper::TokensController where we can add the concern as well? We would need to specify the new controller in https://gitlab.com/gitlab-org/gitlab/-/blob/a64f6362e2a52f532f54c4afda0d0f60ed551f97/config/routes.rb#L25-28.

@toupeira one more time I need to pick your brain.

Both of this controllers: Oauth::TokenInfoController and Doorkeeper::TokensController inherit from Doorkeeper::ApplicationMetalController, which inherits from ActionController::API. We cannot use helper_method then, which is used in concern enforcing 2FA. Since both of this controllers are api-only, maybe we need to enforce 2FA in a different way?

@mksionek hmm check_two_factor_requirement only does a redirect which seems to be supported in metal controllers, and it looks like the helper methods are only used in app/views/profiles/two_factor_auths/show.html.haml which I don't think we need here.

So I think the easiest option would be to wrap that helper_method call in a if defined?(:helper_method) block, with a comment mentioning that this is needed to support ActionController::Metal.

Another approach might be to return a JSON response instead of doing a redirect, but personally I don't think that's really worth it as the 302 status from the redirect should be obvious enough too.

marked this issue as related to #222879 (closed)

mentioned in issue #222879 (closed)

marked #222879 (closed) as a duplicate of this issue

#222879 (comment 386589170) was closed as Duplicate: we want to make sure both the "create Oauth app" and "authorize app" screens are protected by 2FA

CVE requested

Assigned CVE-2020-13290

Fixed in release 13.2.3

closed

This HackerOne security issue was closed 30 days ago and may become public.

Please ensure the following items are true and add a reaction:

• Issue description and comments do not contain sensitive data belonging to GitLab.
• Issue does not reveal private information of the reporter (i.e. session IDs, passwords).

If the issue needs to stay confidential, please add the keep confidential label.

If you removed confidential data from the issue description before making it public, make sure that the description history entry is deleted.

made the issue visible to everyone

changed milestone to %Next 4-6 releases

mentioned in issue #436679 (closed)