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 --- Bugzilla.pm | 10 ++ Bugzilla/Auth/Persist/Cookie.pm | 2 + Bugzilla/CGI.pm | 28 ++++++ Bugzilla/Token.pm | 2 +- extensions/GitHubAuth/Extension.pm | 15 +-- extensions/GitHubAuth/lib/Client.pm | 44 +++------ extensions/GitHubAuth/lib/Login.pm | 91 +++++++++-------- extensions/GitHubAuth/lib/Util.pm | 35 ------- .../auth/login-additional_methods.html.tmpl | 10 +- .../auth/login-small-additional_methods.html.tmpl | 12 ++- .../hook/global/code-error-errors.html.tmpl | 12 ++- github.cgi | 110 +++++++++++++++++++++ 12 files changed, 228 insertions(+), 143 deletions(-) delete mode 100644 extensions/GitHubAuth/lib/Util.pm create mode 100755 github.cgi diff --git a/Bugzilla.pm b/Bugzilla.pm index 96f7cd0d2..a219d5bde 100644 --- a/Bugzilla.pm +++ b/Bugzilla.pm @@ -353,6 +353,16 @@ sub page_requires_login { return $_[0]->request_cache->{page_requires_login}; } +sub github_secret { + my ($class) = @_; + my $cache = $class->request_cache; + my $cgi = $class->cgi; + + $cache->{github_secret} //= $cgi->cookie('github_secret') // generate_random_password(16); + + return $cache->{github_secret}; +} + sub login { my ($class, $type) = @_; diff --git a/Bugzilla/Auth/Persist/Cookie.pm b/Bugzilla/Auth/Persist/Cookie.pm index 4adb00f96..fd910b118 100644 --- a/Bugzilla/Auth/Persist/Cookie.pm +++ b/Bugzilla/Auth/Persist/Cookie.pm @@ -90,6 +90,8 @@ sub persist_login { $cookieargs{'-secure'} = 1; } + $cgi->remove_cookie('github_secret'); + $cgi->remove_cookie('Bugzilla_login_request_cookie'); $cgi->send_cookie(-name => 'Bugzilla_login', -value => $user->id, %cookieargs); diff --git a/Bugzilla/CGI.pm b/Bugzilla/CGI.pm index a5a1afc5c..4deb5aa52 100644 --- a/Bugzilla/CGI.pm +++ b/Bugzilla/CGI.pm @@ -31,6 +31,7 @@ use Bugzilla::Util; use Bugzilla::Search::Recent; use File::Basename; +use URI; BEGIN { if (ON_WINDOWS) { @@ -125,6 +126,21 @@ sub new { return $self; } +sub target_uri { + my ($self) = @_; + + my $base = correct_urlbase(); + if (my $request_uri = $self->request_uri) { + my $base_uri = URI->new($base); + $base_uri->path(''); + $base_uri->query(undef); + return $base_uri . $request_uri; + } + else { + return $base . ($self->url(-relative => 1, -query => 1) || 'index.cgi'); + } +} + # We want this sorted plus the ability to exclude certain params sub canonicalise_query { my ($self, @exclude) = @_; @@ -355,6 +371,16 @@ sub header { %args); } + # 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. + # this prevents some types of redirection attacks. + unless ($user->id) { + $self->send_cookie(-name => 'github_secret', + -value => Bugzilla->github_secret, + -httponly => 1); + } + # Add the cookies in if we have any if (scalar(@{$self->{Bugzilla_cookie_list}})) { unshift(@_, '-cookie' => $self->{Bugzilla_cookie_list}); @@ -475,6 +501,8 @@ sub send_cookie { $paramhash{'-path'} = Bugzilla->params->{'cookiepath'}; $paramhash{'-domain'} = Bugzilla->params->{'cookiedomain'} if Bugzilla->params->{'cookiedomain'}; + $paramhash{'-secure'} = 1 + if Bugzilla->params->{'ssl_redirect'}; # Move the param list back into an array for the call to cookie(). foreach (keys(%paramhash)) { diff --git a/Bugzilla/Token.pm b/Bugzilla/Token.pm index 3c5261821..6e3095549 100644 --- a/Bugzilla/Token.pm +++ b/Bugzilla/Token.pm @@ -237,7 +237,7 @@ sub issue_short_lived_session_token { # the token to the caller. $user //= Bugzilla->user; - return _create_token($user->id, 'session.short', $data); + return _create_token($user->id ? $user->id : undef, 'session.short', $data); } sub issue_hash_token { 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') %]

- - + + + - +

[% 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') %] - - Sign in with GitHub or +
+ + + or +
[% 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 state 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 code 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 %] diff --git a/github.cgi b/github.cgi new file mode 100755 index 000000000..e2955e299 --- /dev/null +++ b/github.cgi @@ -0,0 +1,110 @@ +#!/usr/bin/perl -wT +# 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. + +use 5.10.1; +use strict; +use lib qw(. lib); + +use Bugzilla; +use Bugzilla::Util qw( correct_urlbase ); +use Bugzilla::Error; +use Bugzilla::Constants; +use Bugzilla::Token qw( issue_short_lived_session_token + set_token_extra_data + get_token_extra_data + delete_token ); +use URI; +use URI::QueryParam; +BEGIN { Bugzilla->extensions } +use Bugzilla::Extension::GitHubAuth::Client; + +my $cgi = Bugzilla->cgi; +my $urlbase = correct_urlbase(); + +if (lc($cgi->request_method) eq 'post') { + # POST requests come from Bugzilla itself and begin the GitHub login process + # by redirecting the user to GitHub's authentication endpoint. + + my $user = Bugzilla->login(LOGIN_OPTIONAL); + 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' }); + + ThrowCodeError("github_invalid_request", { reason => 'invalid secret' }) + unless $github_secret eq $github_secret2; + + ThrowCodeError("github_invalid_target", { target_uri => $target_uri }) + unless $target_uri =~ /^\Q$urlbase\E/; + + if ($user->id) { + print $cgi->redirect($target_uri); + exit; + } + + my $state = issue_short_lived_session_token("github_state"); + set_token_extra_data($state, { type => 'github_login', target_uri => $target_uri }); + + $cgi->send_cookie(-name => 'github_state', + -value => $state, + -httponly => 1); + print $cgi->redirect(Bugzilla::Extension::GitHubAuth::Client->authorize_uri($state)); +} +elsif (lc($cgi->request_method) eq 'get') { + # GET requests come from GitHub, with this script acting as the OAuth2 callback. + my $state_param = $cgi->param('state'); + my $state_cookie = $cgi->cookie('github_state'); + + # 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"); + exit; + } + + ThrowCodeError("github_invalid_request", { reason => 'invalid state param' }) + unless $state_param eq $state_cookie; + + my $state_data = get_token_extra_data($state_param); + ThrowCodeError("github_invalid_request", { reason => 'invalid state param' } ) + unless $state_data && $state_data->{type}; + + + $cgi->remove_cookie('github_state'); + delete_token($state_param); + + if ($state_data->{type} eq 'github_login') { + Bugzilla->request_cache->{github_action} = 'login'; + Bugzilla->request_cache->{github_target_uri} = $state_data->{target_uri}; + } + elsif ($state_data->{type} eq 'github_email') { + Bugzilla->request_cache->{github_action} = 'email'; + Bugzilla->request_cache->{github_emails} = $state_data->{emails}; + } + else { + ThrowCodeError("github_invalid_request", { reason => "invalid state param" }) + } + my $user = Bugzilla->login(LOGIN_REQUIRED); + + my $target_uri = URI->new($state_data->{target_uri}); + + # It makes very little sense to login to a page with the logout parameter. + # doing so would be a no-op, so we ignore the logout param here. + $target_uri->query_param_delete('logout'); + + if ($target_uri->path =~ /attachment\.cgi/) { + my $attachment_uri = URI->new($urlbase . "attachment.cgi"); + $attachment_uri->query_param(id => scalar $target_uri->query_param('id')); + if ($target_uri->query_param('action')) { + $attachment_uri->query_param(action => scalar $target_uri->query_param('action')); + } + print $cgi->redirect($attachment_uri); + } + else { + print $cgi->redirect($target_uri); + } +} -- cgit v1.2.3-24-g4f1b