Skip to content

Commit 534fc21

Browse files
author
Dylan Hardison
committed
Bug 1196743 - Fix information disclosure vulnerability that allows attacker to obtain victim's GitHub OAuth return code
1 parent 67d9618 commit 534fc21

File tree

12 files changed

+228
-143
lines changed

12 files changed

+228
-143
lines changed

‎Bugzilla.pm

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,16 @@ sub page_requires_login {
353353
return $_[0]->request_cache->{page_requires_login};
354354
}
355355

356+
sub github_secret {
357+
my ($class) = @_;
358+
my $cache = $class->request_cache;
359+
my $cgi = $class->cgi;
360+
361+
$cache->{github_secret} //= $cgi->cookie('github_secret') // generate_random_password(16);
362+
363+
return $cache->{github_secret};
364+
}
365+
356366
sub login {
357367
my ($class, $type) = @_;
358368

‎Bugzilla/Auth/Persist/Cookie.pm

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,8 @@ sub persist_login {
9090
$cookieargs{'-secure'} = 1;
9191
}
9292

93+
$cgi->remove_cookie('github_secret');
94+
$cgi->remove_cookie('Bugzilla_login_request_cookie');
9395
$cgi->send_cookie(-name => 'Bugzilla_login',
9496
-value => $user->id,
9597
%cookieargs);

‎Bugzilla/CGI.pm

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ use Bugzilla::Util;
3131
use Bugzilla::Search::Recent;
3232

3333
use File::Basename;
34+
use URI;
3435

3536
BEGIN {
3637
if (ON_WINDOWS) {
@@ -125,6 +126,21 @@ sub new {
125126
return $self;
126127
}
127128

129+
sub target_uri {
130+
my ($self) = @_;
131+
132+
my $base = correct_urlbase();
133+
if (my $request_uri = $self->request_uri) {
134+
my $base_uri = URI->new($base);
135+
$base_uri->path('');
136+
$base_uri->query(undef);
137+
return $base_uri . $request_uri;
138+
}
139+
else {
140+
return $base . ($self->url(-relative => 1, -query => 1) || 'index.cgi');
141+
}
142+
}
143+
128144
# We want this sorted plus the ability to exclude certain params
129145
sub canonicalise_query {
130146
my ($self, @exclude) = @_;
@@ -355,6 +371,16 @@ sub header {
355371
%args);
356372
}
357373

374+
# We generate a cookie and store it in the request cache
375+
# To initiate github login, a form POSTs to github.cgi with the
376+
# github_secret as a parameter. It must match the github_secret cookie.
377+
# this prevents some types of redirection attacks.
378+
unless ($user->id) {
379+
$self->send_cookie(-name => 'github_secret',
380+
-value => Bugzilla->github_secret,
381+
-httponly => 1);
382+
}
383+
358384
# Add the cookies in if we have any
359385
if (scalar(@{$self->{Bugzilla_cookie_list}})) {
360386
unshift(@_, '-cookie' => $self->{Bugzilla_cookie_list});
@@ -475,6 +501,8 @@ sub send_cookie {
475501
$paramhash{'-path'} = Bugzilla->params->{'cookiepath'};
476502
$paramhash{'-domain'} = Bugzilla->params->{'cookiedomain'}
477503
if Bugzilla->params->{'cookiedomain'};
504+
$paramhash{'-secure'} = 1
505+
if Bugzilla->params->{'ssl_redirect'};
478506

479507
# Move the param list back into an array for the call to cookie().
480508
foreach (keys(%paramhash)) {

‎Bugzilla/Token.pm

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ sub issue_short_lived_session_token {
237237
# the token to the caller.
238238

239239
$user //= Bugzilla->user;
240-
return _create_token($user->id, 'session.short', $data);
240+
return _create_token($user->id ? $user->id : undef, 'session.short', $data);
241241
}
242242

243243
sub issue_hash_token {

‎extensions/GitHubAuth/Extension.pm

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ use strict;
1212
use parent qw(Bugzilla::Extension);
1313

1414
use Bugzilla::Extension::GitHubAuth::Client;
15-
use Bugzilla::Extension::GitHubAuth::Util qw(target_uri);
1615

1716
use Bugzilla::Error;
1817
use Bugzilla::Util qw(trick_taint);
@@ -30,7 +29,7 @@ BEGIN {
3029
my ($stack, $method) = @_;
3130

3231
return undef if $method eq 'fail_nodata';
33-
return $stack->UNIVERSAL::can($method);
32+
return $stack->SUPER::can($method);
3433
};
3534
}
3635

@@ -42,18 +41,6 @@ sub install_before_final_checks {
4241
}) unless Bugzilla::Group->new({ name => 'no-github-auth' });
4342
}
4443

45-
sub template_before_create {
46-
my ($self, $args) = @_;
47-
48-
return if Bugzilla->user->id && !Bugzilla->cgi->param('logout');
49-
50-
$args->{config}{VARIABLES}{github_auth} = {
51-
login => sub {
52-
return Bugzilla::Extension::GitHubAuth::Client->login_uri(target_uri());
53-
},
54-
};
55-
}
56-
5744
sub attachment_should_redirect_login {
5845
my ($self, $args) = @_;
5946
my $cgi = Bugzilla->cgi;

‎extensions/GitHubAuth/lib/Client.pm

Lines changed: 11 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ use URI::QueryParam;
1515
use Digest;
1616

1717
use Bugzilla::Extension::GitHubAuth::Client::Error qw(ThrowUserError ThrowCodeError);
18-
use Bugzilla::Util qw(remote_ip);
18+
use Bugzilla::Util qw(remote_ip correct_urlbase);
1919

2020
use constant DIGEST_HASH => 'SHA1';
2121

@@ -35,19 +35,22 @@ sub new {
3535
}
3636

3737
sub login_uri {
38-
my ($self, $target) = @_;
38+
my ($class, $target_uri) = @_;
3939

40-
$target->query_param(GoAheadAndLogIn => 1);
41-
$target->query_param(github_login => 1);
42-
$target->query_param_delete('logout');
40+
my $uri = URI->new(correct_urlbase() . "github.cgi");
41+
$uri->query_form(target_uri => $target_uri);
42+
return $uri;
43+
}
4344

44-
my $uri = URI->new(GH_AUTHORIZE_URI);
45+
sub authorize_uri {
46+
my ($class, $state) = @_;
4547

48+
my $uri = URI->new(GH_AUTHORIZE_URI);
4649
$uri->query_form(
4750
client_id => Bugzilla->params->{github_client_id},
4851
scope => 'user:email',
49-
state => $self->get_state($target),
50-
redirect_uri => $target,
52+
state => $state,
53+
redirect_uri => correct_urlbase() . "github.cgi",
5154
);
5255

5356
return $uri;
@@ -65,31 +68,6 @@ sub get_email_key {
6568
return $digest->hexdigest;
6669
}
6770

68-
sub get_state {
69-
my ($class, $target) = @_;
70-
my $sorted_target = $target->clone;
71-
$sorted_target->query_form({});
72-
73-
foreach my $key (sort $target->query_param) {
74-
$sorted_target->query_param($key, $target->query_param($key));
75-
}
76-
77-
$sorted_target->query_param_delete("code");
78-
$sorted_target->query_param_delete("state");
79-
$sorted_target->query_param_delete('github_email_key');
80-
$sorted_target->query_param_delete('github_email');
81-
$sorted_target->query_param_delete('GoAheadAndLogIn');
82-
$sorted_target->query_param_delete('github_login');
83-
84-
my $cgi = Bugzilla->cgi;
85-
my $digest = Digest->new(DIGEST_HASH);
86-
$digest->add($sorted_target->as_string);
87-
$digest->add(remote_ip());
88-
$digest->add($cgi->cookie('Bugzilla_github_token') // Bugzilla->request_cache->{github_token} // '');
89-
$digest->add(Bugzilla->localconfig->{site_wide_secret});
90-
return $digest->hexdigest;
91-
}
92-
9371
sub _handle_response {
9472
my ($self, $response) = @_;
9573
my $data = eval {

‎extensions/GitHubAuth/lib/Login.pm

Lines changed: 44 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,12 @@ use fields qw(github_failure);
1313

1414
use Scalar::Util qw(blessed);
1515

16-
use Bugzilla::Constants qw(AUTH_NODATA AUTH_ERROR USAGE_MODE_BROWSER );
16+
use Bugzilla::Constants qw(AUTH_NODATA AUTH_ERROR USAGE_MODE_BROWSER);
1717
use Bugzilla::Util qw(trick_taint correct_urlbase generate_random_password);
18+
use Bugzilla::Token qw(issue_short_lived_session_token set_token_extra_data);
19+
use List::MoreUtils qw(any);
1820
use Bugzilla::Extension::GitHubAuth::Client;
1921
use Bugzilla::Extension::GitHubAuth::Client::Error ();
20-
use Bugzilla::Extension::GitHubAuth::Util qw(target_uri);
2122
use Bugzilla::Error;
2223

2324
use constant { requires_verification => 1,
@@ -27,30 +28,16 @@ use constant { requires_verification => 1,
2728
sub get_login_info {
2829
my ($self) = @_;
2930
my $cgi = Bugzilla->cgi;
30-
my $github_login = $cgi->param('github_login');
31-
my $github_email = $cgi->param('github_email');
32-
my $github_email_key = $cgi->param('github_email_key');
33-
34-
my $cookie = $cgi->cookie('Bugzilla_github_token');
35-
unless ($cookie) {
36-
my $token = generate_random_password();
37-
$cgi->send_cookie(-name => 'Bugzilla_github_token',
38-
-value => $token,
39-
Bugzilla->params->{'ssl_redirect'} ? ( -secure => 1 ) : (),
40-
-httponly => 1);
41-
Bugzilla->request_cache->{github_token} = $token;
42-
}
31+
my $github_action = Bugzilla->request_cache->{github_action};
4332

44-
return { failure => AUTH_NODATA } unless $github_login;
33+
return { failure => AUTH_NODATA } unless $github_action;
4534

4635
my $response;
47-
if ($github_email_key && $github_email) {
48-
trick_taint($github_email);
49-
trick_taint($github_email_key);
50-
$response = $self->_get_login_info_from_email($github_email, $github_email_key);
36+
if ($github_action eq 'login') {
37+
$response = $self->_get_login_info_from_github();
5138
}
52-
else {
53-
$response = $self->_get_login_info_from_github();
39+
elsif ($github_action eq 'email') {
40+
$response = $self->_get_login_info_from_email();
5441
}
5542

5643
if (!exists $response->{failure}) {
@@ -84,20 +71,13 @@ sub _get_login_info_from_github {
8471
my ($self) = @_;
8572
my $cgi = Bugzilla->cgi;
8673
my $template = Bugzilla->template;
87-
my $state = $cgi->param('state');
8874
my $code = $cgi->param('code');
8975

9076
return { failure => AUTH_ERROR, error => 'github_missing_code' } unless $code;
91-
return { failure => AUTH_ERROR, error => 'github_invalid_state' } unless $state;
9277

9378
trick_taint($code);
94-
trick_taint($state);
9579

96-
my $target = target_uri();
9780
my $client = Bugzilla::Extension::GitHubAuth::Client->new;
98-
if ($state ne $client->get_state($target)) {
99-
return { failure => AUTH_ERROR, error => 'github_invalid_state' };
100-
}
10181

10282
my ($access_token, $emails);
10383
eval {
@@ -119,15 +99,6 @@ sub _get_login_info_from_github {
11999
my @emails = map { $_->{email} }
120100
grep { $_->{verified} && $_->{email} !~ /\@users\.noreply\.github\.com$/ } @$emails;
121101

122-
my $choose_email = sub {
123-
my ($email) = @_;
124-
my $uri = $target->clone;
125-
my $key = Bugzilla::Extension::GitHubAuth::Client->get_email_key($email);
126-
$uri->query_param(github_email => $email);
127-
$uri->query_param(github_email_key => $key);
128-
return $uri;
129-
};
130-
131102
my @bugzilla_users;
132103
my @github_emails;
133104
foreach my $email (@emails) {
@@ -143,15 +114,14 @@ sub _get_login_info_from_github {
143114

144115
if (@allowed_bugzilla_users == 1) {
145116
my ($user) = @allowed_bugzilla_users;
146-
$cgi->remove_cookie('Bugzilla_github_token');
147117
return { user => $user };
148118
}
149119
elsif (@allowed_bugzilla_users > 1) {
150120
$self->{github_failure} = {
151121
template => 'account/auth/github-verify-account.html.tmpl',
152122
vars => {
153123
bugzilla_users => \@allowed_bugzilla_users,
154-
choose_email => $choose_email,
124+
choose_email => _mk_choose_email(\@emails),
155125
},
156126
};
157127
return { failure => AUTH_NODATA };
@@ -165,7 +135,7 @@ sub _get_login_info_from_github {
165135
template => 'account/auth/github-verify-account.html.tmpl',
166136
vars => {
167137
github_emails => \@github_emails,
168-
choose_email => $choose_email,
138+
choose_email => _mk_choose_email(\@emails),
169139
},
170140
};
171141
return { failure => AUTH_NODATA };
@@ -176,19 +146,22 @@ sub _get_login_info_from_github {
176146
}
177147

178148
sub _get_login_info_from_email {
179-
my ($self, $github_email, $github_email_key) = @_;
149+
my ($self) = @_;
180150
my $cgi = Bugzilla->cgi;
151+
my $email = $cgi->param('email') or return { failure => AUTH_ERROR,
152+
user_error => 'github_invalid_email',
153+
details => { email => '' } };
154+
trick_taint($email);
181155

182-
my $key = Bugzilla::Extension::GitHubAuth::Client->get_email_key($github_email);
183-
unless ($github_email_key eq $key) {
156+
unless (any { $_ eq $email } @{ Bugzilla->request_cache->{github_emails} }) {
184157
return { failure => AUTH_ERROR,
185158
user_error => 'github_invalid_email',
186-
details => { email => $github_email }};
159+
details => { email => $email }};
187160
}
188161

189-
my $user = Bugzilla::User->new({name => $github_email, cache => 1});
162+
my $user = Bugzilla::User->new({name => $email, cache => 1});
190163
$cgi->remove_cookie('Bugzilla_github_token');
191-
return $user ? { user => $user } : { email => $github_email };
164+
return $user ? { user => $user } : { email => $email };
192165
}
193166

194167
sub fail_nodata {
@@ -206,5 +179,29 @@ sub fail_nodata {
206179
exit;
207180
}
208181

182+
sub _store_emails {
183+
my ($emails) = @_;
184+
my $state = issue_short_lived_session_token("github_email");
185+
set_token_extra_data($state, { type => 'github_email',
186+
emails => $emails,
187+
target_uri => Bugzilla->request_cache->{github_target_uri} });
188+
189+
Bugzilla->cgi->send_cookie(-name => 'github_state',
190+
-value => $state,
191+
-httponly => 1);
192+
return $state;
193+
}
194+
195+
sub _mk_choose_email {
196+
my ($emails) = @_;
197+
my $state = _store_emails($emails);
198+
199+
return sub {
200+
my $email = shift;
201+
my $uri = URI->new(correct_urlbase() . "github.cgi");
202+
$uri->query_form( state => $state, email => $email );
203+
return $uri;
204+
};
205+
}
209206

210207
1;

0 commit comments

Comments
 (0)