1162302 - Bugzilla to Github 0auth CSRF

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[Muhammad Shahmeer]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.115 Safari/537.36 OPR/27.0.1689.76

Steps to reproduce:

This is Shahmeer and i found out about an in github 0auth that is about a CSRF in it's 0auth due the non validation of the state parameter in bugzilla

Consider this URL
https://github.com/login/oauth/authorize?client_id=53a879b058c4e4953f95&scope=user%3Aemail&state=86385dfdfd8e0a212ee486fd3e87c093e2f0e65a&redirect_uri=https%3A%2F%2Fbugzilla.mozilla.org%2Fenter_bug.cgi%3FGoAheadAndLogIn%3D1%26github_login%3D1

And this 
https://github.com/login/oauth/authorize?client_id=53a879b058c4e4953f95&scope=user%3Aemail&redirect_uri=https%3A%2F%2Fbugzilla.mozilla.org%2Fenter_bug.cgi%3FGoAheadAndLogIn%3D1%26github_login%3D1


Actual results:

The response of the request is not affected even if the state parameter is removed from the request that includes the sub parameters auth id as well
So the github 0auth is vulnerable to is vulnerable to CSRF because of the non validation of the state parameter


Expected results:

It should have showed an error

Comment 1

[Dylan Hardison [:dylan] (he/him)]

Thanks for the bug report! I am unable to confirm it at this time. Under both conditions there is an error page (with distinct errors for no state vs. an invalid state).

Can you provide more details, or even a working example against https://bugzilla-dev.allizom.org?

Comment 2

[Dylan Hardison [:dylan] (he/him)]

I am still unable to confirm this bug. Could you provide more details?

When either the state is not included, or does not match the expected hash, login does not proceed.

CC'ing dveditz so he is made aware of this unconfirmed bug.

Comment 3

[Muhammad Shahmeer]

Can you please reusing the state parameter on this attempt?

Comment 4

[Dylan Hardison [:dylan] (he/him)]

If I use your state parameter, I get:

"An invalid state parameter was passed to the GitHub OAuth2 callback."

However, I see the concern: state parameters would be the same across requests from the same location (to the same target URI). 

As a result, a person from a given location could redirect users at that location to any URL that Bugzilla displays a github auth link -- this would only happen on pages that non-logged-in users can access.

Users would be prompted by github (unless they've used github auth before) and would be redirected to the location in question as themselves.

I have disabled github auth for the time being. 

:dveditz, does it represent a security concern that the state parameter is the same for the same redirect_uri and remote ip in the context of an OAuth2 consumer?

Comment 5

[Dylan Hardison [:dylan] (he/him)]

As a proposed fix, we will 

1) restrict the github login method to *always* redirect to index.cgi
2) Recognizing that this is bad UX, in the fullness of time, we will improve the ux.

Comment 6

[Muhammad Shahmeer]

Great, I think that will fix the issue,
One more thing. Does this qualify for a bounty?

Comment 7

[:glob ✱]

> Does this qualify for a bounty?

i've set the right flag to draw it to our security team's attention.

> As a result, a person from a given location could redirect users
> at that location to any URL that Bugzilla displays a github auth
> link -- this would only happen on pages that non-logged-in users
> can access.

i would classify this as sec-low -- crsf protection mitigates any potential for mischief.

Comment 8

[Muhammad Shahmeer]

Okay.

Comment 9

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1162302_1.patch

Add randomization cookie, so that state (and email key) values are never the same.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

This was determined not to be bounty eligible because of the sec-low security rating.

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 8607016 [details] [diff] [review]
1162302_1.patch

Review of attachment 8607016 [details] [diff] [review]:
-----------------------------------------------------------------

Would rather the cookie be called Bugzilla_github_token or something similar. Bugzilla_secret sounds too generic and only GitHub auth is using the cookie. Fix on commit. r=dkl

Comment 12

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1162302_2.patch

Carrying forward r+ with cookie name changed.

glob will need to commit this for the next push.

Comment 13

[:glob ✱]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   d85abfe..07e47c4  master -> master