From 534fc2123e40b7517aeaffd709faf72af97ac3b8 Mon Sep 17 00:00:00 2001 From: Dylan Hardison Date: Thu, 5 Nov 2015 00:28:14 -0500 Subject: Bug 1196743 - Fix information disclosure vulnerability that allows attacker to obtain victim's GitHub OAuth return code --- extensions/GitHubAuth/lib/Client.pm | 44 +++++------------- extensions/GitHubAuth/lib/Login.pm | 91 ++++++++++++++++++------------------- extensions/GitHubAuth/lib/Util.pm | 35 -------------- 3 files changed, 55 insertions(+), 115 deletions(-) delete mode 100644 extensions/GitHubAuth/lib/Util.pm (limited to 'extensions/GitHubAuth/lib') 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; -- cgit v1.2.3-24-g4f1b