Last Comment Bug 1196743 - Fix information disclosure vulnerability that allows attacker to obtain victim's GitHub OAuth return code
: Fix information disclosure vulnerability that allows attacker to obtain victi...
Status: RESOLVED FIXED
sec-high
: sec-high, wsec-authentication
Product: bugzilla.mozilla.org
Classification: Other
Component: General (show other bugs)
: Production
: Unspecified Unspecified
-- normal (vote)
: ---
Assigned To: Dylan Hardison [:dylan]
:
:
Mentors:
Depends on: 1199087
Blocks:
  Show dependency treegraph
 
Reported: 2015-08-20 07:24 PDT by Mario Gomes
Modified: 2015-12-15 20:25 PST (History)
7 users (show)
rforbes: sec‑bounty+
See Also:
Due Date:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
1196743_2.patch (1.39 KB, patch)
2015-08-31 17:56 PDT, Dylan Hardison [:dylan]
no flags Details | Diff | Splinter Review
1196743_3.patch (1.72 KB, patch)
2015-08-31 20:53 PDT, Dylan Hardison [:dylan]
dkl: review-
Details | Diff | Splinter Review
1196743_4.patch (3.97 KB, patch)
2015-09-03 19:09 PDT, Dylan Hardison [:dylan]
glob: review-
Details | Diff | Splinter Review
1196743_5.patch (22.55 KB, patch)
2015-09-04 15:53 PDT, Dylan Hardison [:dylan]
glob: review-
Details | Diff | Splinter Review
1196743_6.patch (20.12 KB, patch)
2015-09-08 07:55 PDT, Dylan Hardison [:dylan]
no flags Details | Diff | Splinter Review
1196743_7.patch (20.45 KB, patch)
2015-10-05 07:35 PDT, Dylan Hardison [:dylan]
no flags Details | Diff | Splinter Review
1196743_8.patch (21.88 KB, patch)
2015-10-05 07:48 PDT, Dylan Hardison [:dylan]
glob: review-
Details | Diff | Splinter Review
1196743_9.patch (24.35 KB, patch)
2015-10-13 15:10 PDT, Dylan Hardison [:dylan]
glob: review-
glob: feedback+
Details | Diff | Splinter Review
1196743_10.patch (24.08 KB, patch)
2015-10-15 15:40 PDT, Dylan Hardison [:dylan]
glob: review+
glob: feedback+
Details | Diff | Splinter Review

Description User image Mario Gomes 2015-08-20 07:24:17 PDT
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 User image Mario Gomes 2015-08-20 07:34:39 PDT
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 3 User image Mario Gomes 2015-08-20 07:39:59 PDT
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 User image Mario Gomes 2015-08-31 06:57:05 PDT
Is there someone actually working on this??
Comment 5 User image Byron Jones ‹:glob› 2015-08-31 07:21:16 PDT
(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 User image Dylan Hardison [:dylan] 2015-08-31 17:56:04 PDT
Created attachment 8655169 [details] [diff] [review]
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 User image Mario Gomes 2015-08-31 18:44:57 PDT
(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 User image Dylan Hardison [:dylan] 2015-08-31 20:53:23 PDT
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.
Comment 9 User image David Lawrence [:dkl] 2015-09-02 21:20:16 PDT
(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 User image Dylan Hardison [:dylan] 2015-09-03 09:23:45 PDT
(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 User image David Lawrence [:dkl] 2015-09-03 13:21:19 PDT
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 User image Dylan Hardison [:dylan] 2015-09-03 19:09:28 PDT
Created attachment 8656936 [details] [diff] [review]
1196743_4.patch

Adds the rel=noreferrer tag to attachment links, and external links (which could also be vulnerable)
Comment 13 User image Dylan Hardison [:dylan] 2015-09-03 19:09:58 PDT
glob is the reviewer now due to US holiday and dkl's PTO
Comment 14 User image Byron Jones ‹:glob› 2015-09-03 20:40:25 PDT
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 User image Dylan Hardison [:dylan] 2015-09-04 15:53:04 PDT
Created attachment 8657351 [details] [diff] [review]
1196743_5.patch
Comment 16 User image Byron Jones ‹:glob› 2015-09-07 21:30:46 PDT
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 User image Dylan Hardison [:dylan] 2015-09-08 07:55:19 PDT
Created attachment 8658198 [details] [diff] [review]
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 User image Byron Jones ‹:glob› 2015-09-09 08:56:41 PDT
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 User image Mario Gomes 2015-09-09 09:02:01 PDT
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 User image Dylan Hardison [:dylan] 2015-09-10 11:28:36 PDT
(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 User image Dylan Hardison [:dylan] 2015-09-14 16:27:43 PDT
Per discussion, we'll implement this with a single endpoint -- but this requires tokendata from bug 1199087
Comment 22 User image Mario Gomes 2015-09-20 15:36:22 PDT
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 User image Byron Jones ‹:glob› 2015-09-21 20:02:39 PDT
(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 User image Mario Gomes 2015-10-05 04:22:30 PDT
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 User image Byron Jones ‹:glob› 2015-10-05 07:01:42 PDT
(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 User image Dylan Hardison [:dylan] 2015-10-05 07:35:42 PDT
Created attachment 8669680 [details] [diff] [review]
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 User image Dylan Hardison [:dylan] 2015-10-05 07:48:17 PDT
Created attachment 8669692 [details] [diff] [review]
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 User image Mario Gomes 2015-10-05 14:07:48 PDT
Thanks for the bounty!
Comment 29 User image Byron Jones ‹:glob› 2015-10-06 00:47:38 PDT
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 User image Byron Jones ‹:glob› 2015-10-07 22:31:07 PDT
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 User image Dylan Hardison [:dylan] 2015-10-13 15:10:36 PDT
Created attachment 8673358 [details] [diff] [review]
1196743_9.patch

Re-assign to dkl if you lack the time to finish this. thanks!
Comment 32 User image Byron Jones ‹:glob› 2015-10-14 23:48:38 PDT
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 User image Dylan Hardison [:dylan] 2015-10-15 15:40:18 PDT
Created attachment 8674576 [details] [diff] [review]
1196743_10.patch

This should address all the bits from the last review.
Comment 34 User image Byron Jones ‹:glob› 2015-11-02 19:21:29 PST
Comment on attachment 8674576 [details] [diff] [review]
1196743_10.patch

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

r=glob
Comment 35 User image Dylan Hardison [:dylan] 2015-11-04 21:29:00 PST
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   67d9618..534fc21  master -> master

Note You need to log in before you can comment on or make changes to this bug.