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

7 files changed, 77 insertions, 142 deletions

diff --git a/extensions/GitHubAuth/Extension.pm b/extensions/GitHubAuth/Extension.pm
index d68934be4..b813689cd 100644
--- a/extensions/GitHubAuth/Extension.pm
+++ b/extensions/GitHubAuth/Extension.pm
@@ -12,7 +12,6 @@ use strict;
 use parent qw(Bugzilla::Extension);
 
 use Bugzilla::Extension::GitHubAuth::Client;
-use Bugzilla::Extension::GitHubAuth::Util qw(target_uri);
 
 use Bugzilla::Error;
 use Bugzilla::Util qw(trick_taint);
@@ -30,7 +29,7 @@ BEGIN {
         my ($stack, $method) = @_;
 
         return undef if $method eq 'fail_nodata';
-        return $stack->UNIVERSAL::can($method);
+        return $stack->SUPER::can($method);
     };
 }
 
@@ -42,18 +41,6 @@ sub install_before_final_checks {
     }) unless Bugzilla::Group->new({ name => 'no-github-auth' });
 }
 
-sub template_before_create {
-    my ($self, $args) = @_;
-
-    return if Bugzilla->user->id && !Bugzilla->cgi->param('logout');
-
-    $args->{config}{VARIABLES}{github_auth} = {
-        login => sub {
-            return Bugzilla::Extension::GitHubAuth::Client->login_uri(target_uri());
-        },
-    };
-}
-
 sub attachment_should_redirect_login {
     my ($self, $args) = @_;
     my $cgi = Bugzilla->cgi;
diff --git a/extensions/GitHubAuth/lib/Client.pm b/extensions/GitHubAuth/lib/Client.pm
index 338a5b639..77c8a6c61 100644
--- a/extensions/GitHubAuth/lib/Client.pm
+++ b/extensions/GitHubAuth/lib/Client.pm
@@ -15,7 +15,7 @@ use URI::QueryParam;
 use Digest;
 
 use Bugzilla::Extension::GitHubAuth::Client::Error qw(ThrowUserError ThrowCodeError);
-use Bugzilla::Util qw(remote_ip);
+use Bugzilla::Util qw(remote_ip correct_urlbase);
 
 use constant DIGEST_HASH => 'SHA1';
 
@@ -35,19 +35,22 @@ sub new {
 }
 
 sub login_uri {
-    my ($self, $target) = @_;
+    my ($class, $target_uri) = @_;
 
-    $target->query_param(GoAheadAndLogIn => 1);
-    $target->query_param(github_login => 1);
-    $target->query_param_delete('logout');
+    my $uri = URI->new(correct_urlbase() . "github.cgi");
+    $uri->query_form(target_uri => $target_uri);
+    return $uri;
+}
 
-    my $uri = URI->new(GH_AUTHORIZE_URI);
+sub authorize_uri {
+    my ($class, $state) = @_;
 
+    my $uri = URI->new(GH_AUTHORIZE_URI);
     $uri->query_form(
         client_id    => Bugzilla->params->{github_client_id},
         scope        => 'user:email',
-        state        => $self->get_state($target),
-        redirect_uri => $target,
+        state        => $state,
+        redirect_uri => correct_urlbase() . "github.cgi",
     );
 
     return $uri;
@@ -65,31 +68,6 @@ sub get_email_key {
     return $digest->hexdigest;
 }
 
-sub get_state {
-    my ($class, $target) = @_;
-    my $sorted_target = $target->clone;
-    $sorted_target->query_form({});
-
-    foreach my $key (sort $target->query_param) {
-        $sorted_target->query_param($key, $target->query_param($key));
-    }
-
-    $sorted_target->query_param_delete("code");
-    $sorted_target->query_param_delete("state");
-    $sorted_target->query_param_delete('github_email_key');
-    $sorted_target->query_param_delete('github_email');
-    $sorted_target->query_param_delete('GoAheadAndLogIn');
-    $sorted_target->query_param_delete('github_login');
-
-    my $cgi    = Bugzilla->cgi;
-    my $digest = Digest->new(DIGEST_HASH);
-    $digest->add($sorted_target->as_string);
-    $digest->add(remote_ip());
-    $digest->add($cgi->cookie('Bugzilla_github_token') // Bugzilla->request_cache->{github_token} // '');
-    $digest->add(Bugzilla->localconfig->{site_wide_secret});
-    return $digest->hexdigest;
-}
-
 sub _handle_response {
     my ($self, $response) = @_;
     my $data = eval {
diff --git a/extensions/GitHubAuth/lib/Login.pm b/extensions/GitHubAuth/lib/Login.pm
index 6e105c33a..f1effb4e1 100644
--- a/extensions/GitHubAuth/lib/Login.pm
+++ b/extensions/GitHubAuth/lib/Login.pm
@@ -13,11 +13,12 @@ use fields qw(github_failure);
 
 use Scalar::Util qw(blessed);
 
-use Bugzilla::Constants qw(AUTH_NODATA AUTH_ERROR USAGE_MODE_BROWSER );
+use Bugzilla::Constants qw(AUTH_NODATA AUTH_ERROR USAGE_MODE_BROWSER);
 use Bugzilla::Util qw(trick_taint correct_urlbase generate_random_password);
+use Bugzilla::Token qw(issue_short_lived_session_token set_token_extra_data);
+use List::MoreUtils qw(any);
 use Bugzilla::Extension::GitHubAuth::Client;
 use Bugzilla::Extension::GitHubAuth::Client::Error ();
-use Bugzilla::Extension::GitHubAuth::Util qw(target_uri);
 use Bugzilla::Error;
 
 use constant { requires_verification   => 1,
@@ -27,30 +28,16 @@ use constant { requires_verification   => 1,
 sub get_login_info {
     my ($self) = @_;
     my $cgi              = Bugzilla->cgi;
-    my $github_login     = $cgi->param('github_login');
-    my $github_email     = $cgi->param('github_email');
-    my $github_email_key = $cgi->param('github_email_key');
-
-    my $cookie = $cgi->cookie('Bugzilla_github_token');
-    unless ($cookie) {
-        my $token = generate_random_password();
-        $cgi->send_cookie(-name     => 'Bugzilla_github_token',
-                          -value    => $token,
-                          Bugzilla->params->{'ssl_redirect'} ? ( -secure => 1 ) : (),
-                          -httponly => 1);
-        Bugzilla->request_cache->{github_token} = $token;
-    }
+    my $github_action    = Bugzilla->request_cache->{github_action};
 
-    return { failure => AUTH_NODATA } unless $github_login;
+    return { failure => AUTH_NODATA } unless $github_action;
 
     my $response;
-    if ($github_email_key && $github_email) {
-        trick_taint($github_email);
-        trick_taint($github_email_key);
-        $response = $self->_get_login_info_from_email($github_email, $github_email_key);
+    if ($github_action eq 'login') {
+        $response = $self->_get_login_info_from_github();
     }
-    else {
-       $response = $self->_get_login_info_from_github();
+    elsif ($github_action eq 'email') {
+        $response = $self->_get_login_info_from_email();
     }
 
     if (!exists $response->{failure}) {
@@ -84,20 +71,13 @@ sub _get_login_info_from_github {
     my ($self) = @_;
     my $cgi      = Bugzilla->cgi;
     my $template = Bugzilla->template;
-    my $state    = $cgi->param('state');
     my $code     = $cgi->param('code');
 
     return { failure => AUTH_ERROR, error => 'github_missing_code' } unless $code;
-    return { failure => AUTH_ERROR, error => 'github_invalid_state' } unless $state;
 
     trick_taint($code);
-    trick_taint($state);
 
-    my $target = target_uri();
     my $client = Bugzilla::Extension::GitHubAuth::Client->new;
-    if ($state ne $client->get_state($target)) {
-        return { failure => AUTH_ERROR, error => 'github_invalid_state' };
-    }
 
     my ($access_token, $emails);
     eval {
@@ -119,15 +99,6 @@ sub _get_login_info_from_github {
     my @emails = map { $_->{email} }
                  grep { $_->{verified} && $_->{email} !~ /\@users\.noreply\.github\.com$/ } @$emails;
 
-    my $choose_email = sub {
-        my ($email) = @_;
-        my $uri  = $target->clone;
-        my $key = Bugzilla::Extension::GitHubAuth::Client->get_email_key($email);
-        $uri->query_param(github_email => $email);
-        $uri->query_param(github_email_key => $key);
-        return $uri;
-    };
-
     my @bugzilla_users;
     my @github_emails;
     foreach my $email (@emails) {
@@ -143,7 +114,6 @@ sub _get_login_info_from_github {
 
     if (@allowed_bugzilla_users == 1) {
         my ($user) = @allowed_bugzilla_users;
-        $cgi->remove_cookie('Bugzilla_github_token');
         return { user => $user };
     }
     elsif (@allowed_bugzilla_users > 1) {
@@ -151,7 +121,7 @@ sub _get_login_info_from_github {
             template => 'account/auth/github-verify-account.html.tmpl',
             vars     => {
                 bugzilla_users => \@allowed_bugzilla_users,
-                choose_email   => $choose_email,
+                choose_email   => _mk_choose_email(\@emails),
             },
         };
         return { failure => AUTH_NODATA };
@@ -165,7 +135,7 @@ sub _get_login_info_from_github {
             template => 'account/auth/github-verify-account.html.tmpl',
             vars     => {
                 github_emails => \@github_emails,
-                choose_email  => $choose_email,
+                choose_email   => _mk_choose_email(\@emails),
             },
         };
         return { failure => AUTH_NODATA };
@@ -176,19 +146,22 @@ sub _get_login_info_from_github {
 }
 
 sub _get_login_info_from_email {
-    my ($self, $github_email, $github_email_key) = @_;
+    my ($self) = @_;
     my $cgi = Bugzilla->cgi;
+    my $email = $cgi->param('email') or return { failure => AUTH_ERROR,
+                                                 user_error => 'github_invalid_email',
+                                                 details => { email => '' } };
+    trick_taint($email);
 
-    my $key = Bugzilla::Extension::GitHubAuth::Client->get_email_key($github_email);
-    unless ($github_email_key eq $key) {
+    unless (any { $_ eq $email } @{ Bugzilla->request_cache->{github_emails} }) {
         return { failure    => AUTH_ERROR,
                  user_error => 'github_invalid_email',
-                 details    => { email => $github_email }};
+                 details    => { email => $email }};
     }
 
-    my $user = Bugzilla::User->new({name => $github_email, cache => 1});
+    my $user = Bugzilla::User->new({name => $email, cache => 1});
     $cgi->remove_cookie('Bugzilla_github_token');
-    return $user ? { user => $user } : { email => $github_email };
+    return $user ? { user => $user } : { email => $email };
 }
 
 sub fail_nodata {
@@ -206,5 +179,29 @@ sub fail_nodata {
     exit;
 }
 
+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} });
+
+    Bugzilla->cgi->send_cookie(-name     => 'github_state',
+                               -value    => $state,
+                               -httponly => 1);
+    return $state;
+}
+
+sub _mk_choose_email {
+    my ($emails) = @_;
+    my $state = _store_emails($emails);
+
+    return sub {
+        my $email = shift;
+        my $uri = URI->new(correct_urlbase() . "github.cgi");
+        $uri->query_form( state => $state, email => $email );
+        return $uri;
+    };
+}
 
 1;
diff --git a/extensions/GitHubAuth/lib/Util.pm b/extensions/GitHubAuth/lib/Util.pm
deleted file mode 100644
index bda76a420..000000000
--- a/extensions/GitHubAuth/lib/Util.pm
+++ /dev/null
@@ -1,35 +0,0 @@
-# This Source Code Form is subject to the terms of the Mozilla Public
-# License, v. 2.0. If a copy of the MPL was not distributed with this
-# file, You can obtain one at http://mozilla.org/MPL/2.0/.
-#
-# This Source Code Form is "Incompatible With Secondary Licenses", as
-# defined by the Mozilla Public License, v. 2.0.
-
-package Bugzilla::Extension::GitHubAuth::Util;
-
-use strict;
-use warnings;
-
-use Bugzilla::Util qw(correct_urlbase);
-use URI;
-
-use base qw(Exporter);
-our @EXPORT = qw( target_uri );
-
-
-# this is like correct_urlbase() except it returns the *requested* uri, before http url rewrites have been applied.
-# needed to generate github's redirect_uri.
-sub target_uri {
-    my $cgi = Bugzilla->cgi;
-    my $base = URI->new(correct_urlbase());
-    if (my $request_uri = $cgi->request_uri) {
-        $base->path('');
-        $request_uri =~ s!^/+!!;
-        return URI->new($base . "/" . $request_uri);
-    }
-    else {
-        return URI->new(correct_urlbase() . $cgi->url(-relative => 1, query => ));
-    }
-}
-
-1;
diff --git a/extensions/GitHubAuth/template/en/default/hook/account/auth/login-additional_methods.html.tmpl b/extensions/GitHubAuth/template/en/default/hook/account/auth/login-additional_methods.html.tmpl
index 26eb8d63b..609b86159 100644
--- a/extensions/GitHubAuth/template/en/default/hook/account/auth/login-additional_methods.html.tmpl
+++ b/extensions/GitHubAuth/template/en/default/hook/account/auth/login-additional_methods.html.tmpl
@@ -5,14 +5,16 @@
   # This Source Code Form is "Incompatible With Secondary Licenses", as
   # defined by the Mozilla Public License, v. 2.0.
   #%]
-
+[% USE Bugzilla %]
 [% IF Param('user_info_class').split(',').contains('GitHubAuth') %]
   <p>
-    <a href="[% github_auth.login FILTER html %]">
-      <img src="extensions/GitHubAuth/web/images/github_sign_in.png"
+    <form method="post" action="[% urlbase FILTER html %]github.cgi">
+      <input type="hidden" name="github_secret" value="[% Bugzilla.github_secret FILTER html %]">
+      <input type="hidden" name="target_uri" value="[% Bugzilla.cgi.target_uri FILTER html %]">
+      <input type="image" src="extensions/GitHubAuth/web/images/github_sign_in.png"
            alt="Sign in with GitHub"
            title="Sign in with GitHub"
            width="185" height="25">
-    </a>
+    </form>
   </p>
 [% END %]
diff --git a/extensions/GitHubAuth/template/en/default/hook/account/auth/login-small-additional_methods.html.tmpl b/extensions/GitHubAuth/template/en/default/hook/account/auth/login-small-additional_methods.html.tmpl
index 6c4582b70..f32b34a59 100644
--- a/extensions/GitHubAuth/template/en/default/hook/account/auth/login-small-additional_methods.html.tmpl
+++ b/extensions/GitHubAuth/template/en/default/hook/account/auth/login-small-additional_methods.html.tmpl
@@ -5,6 +5,7 @@
   # This Source Code Form is "Incompatible With Secondary Licenses", as
   # defined by the Mozilla Public License, v. 2.0.
   #%]
+[% USE Bugzilla %]
 
 [% IF Param('user_info_class').split(',').contains('GitHubAuth') %]
   <script type="text/javascript">
@@ -18,9 +19,12 @@
    });
   </script>
   <span id="github_mini_login[% qs_suffix FILTER html %]" class="bz_default_hidden">
-    <a href="[% github_auth.login FILTER html %]">
-      <img src="extensions/GitHubAuth/web/images/sign_in.png" height="22" width="75" align="absmiddle"
-           alt="Sign in with GitHub"
-           title="Sign in with GitHub"></a> or
+    <form method="post" action="[% urlbase FILTER html %]github.cgi">
+      <input type="hidden" name="github_secret" value="[% Bugzilla.github_secret FILTER html %]">
+      <input type="hidden" name="target_uri" value="[% Bugzilla.cgi.target_uri FILTER html %]">
+        <input type="image" src="extensions/GitHubAuth/web/images/sign_in.png" height="22" width="75" align="absmiddle"
+             alt="Sign in with GitHub"
+             title="Sign in with GitHub"></a> or
+    </form>
   </span>
 [% END %]
diff --git a/extensions/GitHubAuth/template/en/default/hook/global/code-error-errors.html.tmpl b/extensions/GitHubAuth/template/en/default/hook/global/code-error-errors.html.tmpl
index 5f6672e2b..aaf9b6fa3 100644
--- a/extensions/GitHubAuth/template/en/default/hook/global/code-error-errors.html.tmpl
+++ b/extensions/GitHubAuth/template/en/default/hook/global/code-error-errors.html.tmpl
@@ -6,11 +6,7 @@
   # defined by the Mozilla Public License, v. 2.0.
   #%]
 
-[% IF error == "github_invalid_state" %]
-  [% title = "Invalid State Parameter" %]
-  An invalid <em>state</em> parameter was passed to the GitHub OAuth2 callback.
-
-[% ELSIF error == "github_missing_code" %]
+[% IF error == "github_missing_code" %]
   [% title = "Missing GitHub Auth Code" %]
   Expected a <em>code</em> parameter in the GitHub OAuth2 callback.
 
@@ -22,4 +18,10 @@
   [% 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.
+
+[% ELSIF error == "github_invalid_request" %]
+  Invalid GitHub log in attempt (reason: [% reason FILTER html %])
+
 [% END %]