1196743 - Fix information disclosure vulnerability that allows attacker to obtain victim's GitHub OAuth return code

Status: RESOLVED FIXED

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

Description

[Mario Gomes]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.155 Safari/537.36

Steps to reproduce:

Hi,

Bug 1195544 didn't fix properly the vulnerability as result of that there's still a easy way for exploit this vulnerability(it isn't as easy as 1195544 to reproduce it).

i'll post how to reproduce it in the next comment.

Comment 1

[Mario Gomes]

Reproduce:
1. Once you've already logged in Bugzilla using GitHub, and you already allowed the Bugzilla APP access your Github data the autentification proccess will be done automatically.
2. Log in your GitHub account.
3. Log in your Bugzilla account(this account must have access to this bug otherwise it won't work).
4. Now go to https://github.com/login/oauth/authorize?client_id=53a879b058c4e4953f95&scope=user%3Aemail&state=INVALID_STATE&redirect_uri=﷒0﷓
5. You'll redirected back to https://bugzilla.mozilla.org/show_bug.cgi?id=1196743 , but this time you'll have the return code in the page's URL.
6. Now click here: https://bug1120074.bmoattachments.org/attachment.cgi?id=8650462
8. See the alert box with you URL containning the OAut code.

This vulnerability occurs 'cause I can point the address of the bug in the 'redirect_uri' on Github's OAuth, then when the user is redirected back to the bug containning the oauth code I can convince the victim to link on a link inside this bug and get the "oauth code" as it's gonna be passed through the "referer" header.

A patch to this vulnerability would be add "rel = noreferrer" to all anchor tags posted by users.

Would if this would be eligible for a bounty?

Cheers,
Mario

Comment 2

[Mario Gomes]

Just a correction in the step 4 the right url is: https://github.com/login/oauth/authorize?client_id=53a879b058c4e4953f95&scope=user%3Aemail&state=INVALID_STATE&redirect_uri=﷒0﷓

Comment 3

[Mario Gomes]

Some crazy bug isn't allowing me to post full github oauth url then I'll have to this a URL shortener to post it here. Please follow this URL in the step 4: https://goo.gl/AV1RGn

Comment 4

[Mario Gomes]

Is there someone actually working on this??

Comment 5

[:glob ✱]

(In reply to Mario Gomes from comment #4)
> Is there someone actually working on this??

just so you know this is still on our radar.  i'm expecting to have someone look at it this week.

Comment 6

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

Attachment: 1196743_2.patch

Taking a step back from this, it occurs to me that the auth code *should not* be valid when an invalid state is passed. Adding rel=noreferer is *a* solution, but it would be better to fix the underlying problem. Github auth codes are single use, so if we use them when we get an invalid state they cannot be used even if leaked.

setting dkl as r? as he has the most exposure to the github code.

Comment 7

[Mario Gomes]

(In reply to Dylan William Hardison [:dylan] from comment #6)
> Taking a step back from this, it occurs to me that the auth code *should
> not* be valid when an invalid state is passed. 

Do you realize that Bugzilla checks for "github_login=1" in the URL? then it'll validate the "state" value. That's why I changed the original "redirect_uri" removing the "github_login" to force Bugzilla to ignore the "state" and the "code".

Comment 8

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

Attachment: 1196743_3.patch

I really wish we had implemented this with a static callback, as the original draft implementation had. (from Bug 1118365 comment #15)

That said, this patch detects github login attempts via the code= parameter.

Comment 9

[David Lawrence [:dkl]]

(In reply to Dylan William Hardison [:dylan] from comment #8)
> Created attachment 8655231 [details] [diff] [review]
> 1196743_3.patch
> 
> I really wish we had implemented this with a static callback, as the
> original draft implementation had. (from Bug 1118365 comment #15)
> 
> That said, this patch detects github login attempts via the code= parameter.

Just to verify I am thinking about this in the right way. I have observed that github logins continue to work with the patch. It is just that now the code is invalidated and could not be used again (invalidate_auth_code). So it doesn't matter that the info is still in the referrer when viewing the custom attachment?


dkl

Comment 10

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

(In reply to David Lawrence [:dkl] from comment #9)
> (In reply to Dylan William Hardison [:dylan] from comment #8)
> > Created attachment 8655231 [details] [diff] [review]
> > 1196743_3.patch
> > 
> > I really wish we had implemented this with a static callback, as the
> > original draft implementation had. (from Bug 1118365 comment #15)
> > 
> > That said, this patch detects github login attempts via the code= parameter.
> 
> Just to verify I am thinking about this in the right way. I have observed
> that github logins continue to work with the patch. It is just that now the
> code is invalidated and could not be used again (invalidate_auth_code). So
> it doesn't matter that the info is still in the referrer when viewing the
> custom attachment?

That is the case. I would like a confirmation that this is the behavior -- it seems to be to me.

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 8655231 [details] [diff] [review]
1196743_3.patch

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

Looks good and according to my testing, seems to expire the code and not allow for re-use. I would also make the attachment views set rel=noreferer so to mitigate any other possible problems.

Comment 12

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

Attachment: 1196743_4.patch

Adds the rel=noreferrer tag to attachment links, and external links (which could also be vulnerable)

Comment 13

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

glob is the reviewer now due to US holiday and dkl's PTO

Comment 14

[:glob ✱]

Comment on attachment 8656936 [details] [diff] [review]
1196743_4.patch

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

there are places where we link to an attachment that don't have noreferrer protection (looks like just the links in the bugmodal table got the treatment).

extensions/BMO/template/en/default/pages/user_activity.html.tmpl
extensions/BugModal/template/en/default/bug_modal/activity_stream.html.tmpl
extensions/Splinter/web/splinter.js
template/en/default/bug/show-multiple.html.tmpl
template/en/default/bug/activity/table.html.tmpl
template/en/default/request/queue.html.tmpl
template/en/default/attachment/diff-header.html.tmpl
template/en/default/attachment/show-multiple.html.tmpl
template/en/default/attachment/confirm-delete.html.tmpl
template/en/default/attachment/edit.html.tmpl
template/en/default/attachment/created.html.tmpl
template/en/default/attachment/list.html.tmpl

::: Bugzilla/Template.pm
@@ +221,4 @@
>      my $safe_protocols = SAFE_URL_REGEXP();
>      $text =~ s~\b($safe_protocols)
>                ~($tmp = html_quote($1)) &&
> +               ($things[$count++] = "<a rel='noreferrer' href=\"$tmp\">$tmp</a>") &&

nit: i'd prefer to see this written as  qq|<a rel="noreferrer" href="$temp">$tmp</a>| to keep the quote char consistent.

Comment 15

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

Attachment: 1196743_5.patch

Comment 16

[:glob ✱]

Comment on attachment 8657351 [details] [diff] [review]
1196743_5.patch

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

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#attr-rel "The value is a comma-separated list of link types values"
you've used space as a delimiter.

::: extensions/InlineHistory/template/en/default/hook/bug/comments-aftercomments.html.tmpl
@@ +47,4 @@
>        [%# make attachment changes better %]
>        [% IF change.attachid %]
>          html += '<a '
> +                + 'rel="noreferrer nofollow"'

missing a space at the end of the string

::: extensions/REMO/template/en/default/hook/global/user-error-errors.html.tmpl
@@ +31,4 @@
>  [% ELSIF error == "remo_payment_cancel_dupe" %]
>     [% title = "Already filed payment request" %]
>     You already used the form to file
> +   <a rel="noreferrer nofollow" href="[% urlbase FILTER html %]attachment.cgi?id=[% attachid FILTER uri %]&action=edit">

nit: put href as the first attribute (it's the most important).  there are multiple occurrences of this.

::: template/en/default/attachment/list.html.tmpl
@@ +79,4 @@
>          <td valign="top">
>            [% IF attachment.datasize %]
>              <a href="attachment.cgi?id=[% attachment.id %]"
> +               rel="noreferrer nofollow" 

remove trailing whitespace

Comment 17

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

Attachment: 1196743_6.patch

Fixed rel (always check mdn, never trust memory), and re-ordered them so that href is first. I also removed rel= from a few links that didn't need it.

Comment 18

[:glob ✱]

Comment on attachment 8658198 [details] [diff] [review]
1196743_6.patch

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

i'm still working on a full review, but checking for the 'code' field during login conflicts with 2fa's 'code' field.

Comment 19

[Mario Gomes]

A better patch would be prepare a spefic directory to handle with oauth return(eg, https://bugzilla.mozilla.org/oauth/return/) and configure the app on GitHub to only accept "rediredct_uri" to this url) then it wouldn't be possible to point it to some other directory and steal the returned "code".

Comment 20

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

(In reply to Mario Gomes from comment #19)
> A better patch would be prepare a spefic directory to handle with oauth
> return(eg, https://bugzilla.mozilla.org/oauth/return/) and configure the app
> on GitHub to only accept "rediredct_uri" to this url) then it wouldn't be
> possible to point it to some other directory and steal the returned "code".

This was in fact the original design. See Bug 1118365 for why this isn't done.

Comment 21

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

Per discussion, we'll implement this with a single endpoint -- but this requires tokendata from bug 1199087

Comment 22

[Mario Gomes]

Any updates on this? It seems like the patch using "rel=noreferer" has been issued. Why is this still open?Regarding bug bounty award, has this qualified for a bug bounty award?

Comment 23

[:glob ✱]

(In reply to Mario Gomes from comment #22)
> Any updates on this? It seems like the patch using "rel=noreferer" has been
> issued.

comment 21 has the latest.

bug 1199087 adds the ability for us to bind arbitrary data to tokens.  this means this issue can be fixed by moving to a single endpoint with the post-auth url stored in the token.  this is a better overall solution than trying to catch all attachment links.

> Regarding bug bounty award, has this qualified for a bug bounty award?

as always, please email security@mozilla bounty questions.

Comment 24

[Mario Gomes]

bug 1199087 has been fixed. Does this mean this bug is fixed too? Is there anyone actually working on it?? This can be fixed, dont understand this is taking this long.

Comment 25

[:glob ✱]

(In reply to Mario Gomes from comment #24)
> bug 1199087 has been fixed. Does this mean this bug is fixed too? Is there
> anyone actually working on it??

this is actively being worked on.

Comment 26

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

Attachment: 1196743_7.patch

This refactors the github login code to use a single endpoint (github.cgi)
and makes use of tokendata extensively to keep track of information about the client. 

It will work with an unmodified github dev account setup, but in production the callback_url will be set to github.cgi (although it is not possible for the github login code to be called from anywhere but github.cgi). 

MFA checks continue to work as before (you cannot use MFA with github)
and one can login to any page (although we can filter out any such requests).

Comment 27

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

Attachment: 1196743_8.patch

the other login form works. For this to work, though, I have to move the github_secret generation so that it always happens, even when a user is logged in. If that's not acceptable, we'll need to discuss how it can be made to work.

Comment 28

[Mario Gomes]

Thanks for the bounty!

Comment 29

[:glob ✱]

Comment on attachment 8669692 [details] [diff] [review]
1196743_8.patch

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

from an initial reading this looks sane; just a few minor notes which can be addressed after my full review.

::: Bugzilla/CGI.pm
@@ +136,5 @@
> +        $base_uri->query(undef);
> +        return $base_uri . $request_uri;
> +    }
> +    else {
> +        return $base . $self->url(-relative => 1, -query => 1) || 'index.cgi';

this should probably be:
  return $base . ($self->url(..) || 'index.cgi');

::: extensions/GitHubAuth/Extension.pm
@@ +18,4 @@
>  use List::Util qw(first);
>  use URI;
>  use URI::QueryParam;
> +use Digest::SHA qw(hmac_sha256_base64);

you're not using correctl_urlbase or digest::sha here

::: extensions/GitHubAuth/lib/Login.pm
@@ +181,5 @@
>  
> +sub _store_emails {
> +    my ($emails) = @_;
> +    my $state = issue_short_lived_session_token("github_email");
> +    set_token_extra_data($state, { type => 'github_email', emails => $emails, target_uri => Bugzilla->request_cache->{github_target_uri} });

this is a super long line

::: extensions/GitHubAuth/template/en/default/hook/global/code-error-errors.html.tmpl
@@ +22,5 @@
>    [% title = "GitHub Error" %]
>    GitHub returned an error: [% response.message FILTER html %]
>  
> +[% ELSIF error == "github_invalid_target" %]
> +  [% terms.Bugzilla %] cannot log you into an external site via GitHub.

this is a weird message; throw github_invalid_secret

@@ +28,5 @@
> +[% ELSIF error == "github_invalid_secret" %]
> +  Invalid GitHub log in attempt
> +
> +[% ELSIF error == "github_already_logged_in" %]
> +  You cannot log in to [% terms.Bugzilla %] if you are already logged in.

let's not throw this, just no-op it

::: github.cgi
@@ +16,5 @@
> +use Bugzilla::Constants;
> +use Bugzilla::Token qw( issue_short_lived_session_token
> +                        set_token_extra_data
> +                        get_token_extra_data );
> +use Digest::SHA qw( hmac_sha256_base64 );

digest::sha isn't used

@@ +37,5 @@
> +    ThrowCodeError("github_invalid_secret")    if $github_secret ne $github_secret2;
> +    ThrowCodeError("github_already_logged_in") if $user->id;
> +    ThrowCodeError("github_invalid_target", { target_uri => $target_uri })
> +      unless $target_uri =~ /^\Q$urlbase\E/;
> +    

remove trailing whitespace

@@ +39,5 @@
> +    ThrowCodeError("github_invalid_target", { target_uri => $target_uri })
> +      unless $target_uri =~ /^\Q$urlbase\E/;
> +    
> +    my $state = issue_short_lived_session_token("github_state");
> +    set_token_extra_data($state, { type => 'github_login', target_uri => $target_uri });

you should delete the state token here instead of letting it expire

Comment 30

[:glob ✱]

Comment on attachment 8669692 [details] [diff] [review]
1196743_8.patch

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

this mostly looks good, but the setting of the github_state cookie isn't correct.

::: Bugzilla.pm
@@ +353,4 @@
>  
>      return $class->user if $class->user->id;
>  
> +    unless ($class->user->id) {

this code needs a comment.

the secret is generated even if you're logged in, and it's in a weird location.

a better spot would be Bugzilla::CGI->header, where Bugzilla_login_request_cookie is set.

both of these pre-login cookies should be deleted once login is successful

::: extensions/GitHubAuth/lib/Login.pm
@@ +186,5 @@
> +
> +    Bugzilla->cgi->send_cookie(-name     => 'github_state',
> +                               -value    => $state,
> +                               -httponly => 1,
> +                               Bugzilla->params->{'ssl_redirect'} ? ( -secure => 1 ) : ());

as all our cookies should set -secure is ssl_redirect is enabled let's do that in Bugzilla::CGI::send_cookie.  no need to fix up any existing send_cookie calls, just set it if it's missing.

::: extensions/GitHubAuth/template/en/default/hook/global/code-error-errors.html.tmpl
@@ +28,5 @@
> +[% ELSIF error == "github_invalid_secret" %]
> +  Invalid GitHub log in attempt
> +
> +[% ELSIF error == "github_already_logged_in" %]
> +  You cannot log in to [% terms.Bugzilla %] if you are already logged in.

to clarify - if you're already logged in when you post to github.cgi we should redirect back to the target_uri instead of throwing an error

::: github.cgi
@@ +25,5 @@
> +
> +my $cgi     = Bugzilla->cgi;
> +my $urlbase = correct_urlbase();
> +
> +if (lc($cgi->request_method) eq 'post') {

the two code paths could do with comments to indicate that one deals with internal from-login requests, and the other is the github callback

@@ +72,5 @@
> +    }
> +    my $user = Bugzilla->login(LOGIN_REQUIRED);
> +
> +    my $target_uri = URI->new($state_data->{target_uri});
> +    $target_uri->query_param_delete('logout');

add a comment to clarify why the logout param gets special treatment

Comment 31

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

Attachment: 1196743_9.patch

Re-assign to dkl if you lack the time to finish this. thanks!

Comment 32

[:glob ✱]

Comment on attachment 8673358 [details] [diff] [review]
1196743_9.patch

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

this mostly looks great, however the cookie removal doesn't work.

::: Bugzilla/Attachment/PatchReader.pm
@@ +39,4 @@
>          require Bugzilla::PatchReader::DiffPrinter::raw;
>          $last_reader->sends_data_to(new Bugzilla::PatchReader::DiffPrinter::raw());
>          # Actually print out the patch.
> +

no need to touch this file :)

::: Bugzilla/CGI.pm
@@ +373,5 @@
>  
> +    # We generate a cookie and store it in the request cache
> +    # To initiate github login, a form POSTs to github.cgi with the
> +    # github_secret as a parameter. It must match the github_secret cookie.
> +    # it is believed this prevents some times of redirection attacks.

s/it is believed// and s/times/types/

::: extensions/GitHubAuth/lib/Login.pm
@@ +188,5 @@
> +
> +    Bugzilla->cgi->send_cookie(-name     => 'github_state',
> +                               -value    => $state,
> +                               -httponly => 1,
> +                               Bugzilla->params->{'ssl_redirect'} ? ( -secure => 1 ) : ());

-secure isn't needed here

::: github.cgi
@@ +17,5 @@
> +use Bugzilla::Token qw( issue_short_lived_session_token
> +                        set_token_extra_data
> +                        get_token_extra_data
> +                        delete_token );
> +use Digest::SHA qw( hmac_sha256_base64 );

digest::sha isn't used here

@@ +35,5 @@
> +    my $target_uri     = $cgi->param('target_uri')    or ThrowCodeError("github_invalid_target");
> +    my $github_secret  = $cgi->param('github_secret') or ThrowCodeError("github_invalid_request", { reason => 'invalid secret' });
> +    my $github_secret2 = Bugzilla->github_secret      or ThrowCodeError("github_invalid_request", { reason => 'invalid secret' });
> +
> +    trick_taint($target_uri);

no need to untaint target_uri

@@ +50,5 @@
> +
> +    my $state = issue_short_lived_session_token("github_state");
> +    set_token_extra_data($state, { type => 'github_login', target_uri => $target_uri });
> +
> +    $cgi->remove_cookie('github_secret');

this cookie should be removed regardless of the auth method (maybe in Bugzilla::Auth::Persist::Cookie->persist_login?), and the cookie isn't actually being removed (looks like it's being removed by github.cgi, then set again once the redirect is followed).

the Bugzilla_login_request_cookie should be removed at the same time.

@@ +64,5 @@
> +
> +    # If the state or params are missing, or the github_state cookie is missing
> +    # we just redirect to index.cgi.
> +    unless ($state_param && $state_cookie && $cgi->param('code')) {
> +        print $cgi->redirect( $urlbase . "index.cgi" );

remove spaces after ( and before )

Comment 33

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

Attachment: 1196743_10.patch

This should address all the bits from the last review.

Comment 34

[:glob ✱]

Comment on attachment 8674576 [details] [diff] [review]
1196743_10.patch

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

r=glob

Comment 35

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

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   67d9618..534fc21  master -> master