summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDavid Lawrence <dkl@mozilla.com>2014-11-04 04:11:09 +0100
committerByron Jones <glob@mozilla.com>2014-11-04 04:11:09 +0100
commit4e1941fedbe46bafce9aded3a0a38d272fec37a2 (patch)
tree633351ada50932ec6b747705b95e0bd04e39f05e
parentd6ee5ade172abe24389aca15eba9fe922b5697c7 (diff)
downloadbugzilla-4e1941fedbe46bafce9aded3a0a38d272fec37a2.tar.gz
bugzilla-4e1941fedbe46bafce9aded3a0a38d272fec37a2.tar.xz
Bug 1090427: Backport bug 713926 to bmo/4.2 to protect against csrf for login forms
-rw-r--r--Bugzilla/Auth.pm2
-rw-r--r--Bugzilla/Auth/Login/CGI.pm41
-rw-r--r--Bugzilla/CGI.pm13
-rw-r--r--Bugzilla/Template.pm5
-rw-r--r--Bugzilla/Util.pm10
-rwxr-xr-xrelogin.cgi13
-rw-r--r--template/en/default/account/auth/login-small.html.tmpl4
-rw-r--r--template/en/default/account/auth/login.html.tmpl4
-rw-r--r--template/en/default/admin/sudo.html.tmpl5
-rw-r--r--template/en/default/global/user-error.html.tmpl9
10 files changed, 95 insertions, 11 deletions
diff --git a/Bugzilla/Auth.pm b/Bugzilla/Auth.pm
index 2c58b52a8..9f4fb8fa3 100644
--- a/Bugzilla/Auth.pm
+++ b/Bugzilla/Auth.pm
@@ -168,7 +168,7 @@ sub _handle_login_result {
if ($self->{_info_getter}->{successful}->requires_persistence
and !Bugzilla->request_cache->{auth_no_automatic_login})
{
- $self->{_persister}->persist_login($user);
+ $user->{_login_token} = $self->{_persister}->persist_login($user);
}
}
elsif ($fail_code == AUTH_ERROR) {
diff --git a/Bugzilla/Auth/Login/CGI.pm b/Bugzilla/Auth/Login/CGI.pm
index 8e877b951..12b59d68b 100644
--- a/Bugzilla/Auth/Login/CGI.pm
+++ b/Bugzilla/Auth/Login/CGI.pm
@@ -37,19 +37,52 @@ use Bugzilla::Constants;
use Bugzilla::WebService::Constants;
use Bugzilla::Util;
use Bugzilla::Error;
+use Bugzilla::Token;
sub get_login_info {
my ($self) = @_;
my $params = Bugzilla->input_params;
+ my $cgi = Bugzilla->cgi;
+
+ my $login = trim(delete $params->{'Bugzilla_login'});
+ my $password = delete $params->{'Bugzilla_password'};
+ # The token must match the cookie to authenticate the request.
+ my $login_token = delete $params->{'Bugzilla_login_token'};
+ my $login_cookie = $cgi->cookie('Bugzilla_login_request_cookie');
- my $username = trim(delete $params->{"Bugzilla_login"});
- my $password = delete $params->{"Bugzilla_password"};
+ my $valid = 0;
+ # If the web browser accepts cookies, use them.
+ if ($login_token && $login_cookie) {
+ my ($time, undef) = split(/-/, $login_token);
+ # Regenerate the token based on the information we have.
+ my $expected_token = issue_hash_token(['login_request', $login_cookie], $time);
+ $valid = 1 if $expected_token eq $login_token;
+ $cgi->remove_cookie('Bugzilla_login_request_cookie');
+ }
+ # WebServices and other local scripts can bypass this check.
+ # This is safe because we won't store a login cookie in this case.
+ elsif (Bugzilla->usage_mode != USAGE_MODE_BROWSER) {
+ $valid = 1;
+ }
+ # Else falls back to the Referer header and accept local URLs.
+ # Attachments are served from a separate host (ideally), and so
+ # an evil attachment cannot abuse this check with a redirect.
+ elsif (my $referer = $cgi->referer) {
+ my $urlbase = correct_urlbase();
+ $valid = 1 if $referer =~ /^\Q$urlbase\E/;
+ }
+ # If the web browser doesn't accept cookies and the Referer header
+ # is missing, we have no way to make sure that the authentication
+ # request comes from the user.
+ elsif ($login && $password) {
+ ThrowUserError('auth_untrusted_request', { login => $login });
+ }
- if (!defined $username || !defined $password) {
+ if (!$login || !$password || !$valid) {
return { failure => AUTH_NODATA };
}
- return { username => $username, password => $password };
+ return { username => $login, password => $password };
}
sub fail_nodata {
diff --git a/Bugzilla/CGI.pm b/Bugzilla/CGI.pm
index a12fb284b..552da28ea 100644
--- a/Bugzilla/CGI.pm
+++ b/Bugzilla/CGI.pm
@@ -331,6 +331,7 @@ sub close_standby_message {
# Override header so we can add the cookies in
sub header {
my $self = shift;
+ my $user = Bugzilla->user;
# If there's only one parameter, then it's a Content-Type.
if (scalar(@_) == 1) {
@@ -338,6 +339,18 @@ sub header {
unshift(@_, '-type' => shift(@_));
}
+ if (!$user->id && $user->authorizer->can_login
+ && !$self->cookie('Bugzilla_login_request_cookie'))
+ {
+ my %args;
+ $args{'-secure'} = 1 if Bugzilla->params->{ssl_redirect};
+
+ $self->send_cookie(-name => 'Bugzilla_login_request_cookie',
+ -value => generate_random_password(),
+ -httponly => 1,
+ %args);
+ }
+
# Add the cookies in if we have any
if (scalar(@{$self->{Bugzilla_cookie_list}})) {
unshift(@_, '-cookie' => $self->{Bugzilla_cookie_list});
diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm
index 9bd0c51bd..d7e063f67 100644
--- a/Bugzilla/Template.pm
+++ b/Bugzilla/Template.pm
@@ -1036,6 +1036,11 @@ sub create {
# Allow templates to generate a token themselves.
'issue_hash_token' => \&Bugzilla::Token::issue_hash_token,
+ 'get_login_request_token' => sub {
+ my $cookie = Bugzilla->cgi->cookie('Bugzilla_login_request_cookie');
+ return $cookie ? issue_hash_token(['login_request', $cookie]) : '';
+ },
+
# A way for all templates to get at Field data, cached.
'bug_fields' => sub {
my $cache = Bugzilla->request_cache;
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index 2349dc9e9..67798d470 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -36,8 +36,8 @@ use base qw(Exporter);
detaint_signed
html_quote url_quote xml_quote
css_class_quote html_light_quote
- i_am_cgi i_am_webservice correct_urlbase remote_ip validate_ip
- do_ssl_redirect_if_required use_attachbase
+ i_am_cgi i_am_webservice correct_urlbase remote_ip
+ validate_ip do_ssl_redirect_if_required use_attachbase
diff_arrays on_main_db
trim wrap_hard wrap_comment find_wrap_point
format_time validate_date validate_time datetime_from
@@ -875,6 +875,7 @@ Bugzilla::Util - Generic utility functions for bugzilla
# Functions that tell you about your environment
my $is_cgi = i_am_cgi();
+ my $is_webservice = i_am_webservice();
my $urlbase = correct_urlbase();
# Data manipulation
@@ -1004,6 +1005,11 @@ Tells you whether or not you are being run as a CGI script in a web
server. For example, it would return false if the caller is running
in a command-line script.
+=item C<i_am_webservice()>
+
+Tells you whether or not the current usage mode is WebServices related
+such as JSONRPC or XMLRPC.
+
=item C<correct_urlbase()>
Returns either the C<sslbase> or C<urlbase> parameter, depending on the
diff --git a/relogin.cgi b/relogin.cgi
index 6eb798205..9eea67bd5 100755
--- a/relogin.cgi
+++ b/relogin.cgi
@@ -67,6 +67,19 @@ elsif ($action eq 'prepare-sudo') {
# Keep a temporary record of the user visiting this page
$vars->{'token'} = issue_session_token('sudo_prepared');
+ if ($user->authorizer->can_login) {
+ my $value = generate_random_password();
+ my %args;
+ $args{'-secure'} = 1 if Bugzilla->params->{ssl_redirect};
+
+ $cgi->send_cookie(-name => 'Bugzilla_login_request_cookie',
+ -value => $value,
+ -httponly => 1,
+ %args);
+
+ $vars->{'login_request_token'} = issue_hash_token(['login_request', $value]);
+ }
+
# Show the sudo page
$vars->{'target_login_default'} = $cgi->param('target_login');
$vars->{'reason_default'} = $cgi->param('reason');
diff --git a/template/en/default/account/auth/login-small.html.tmpl b/template/en/default/account/auth/login-small.html.tmpl
index 220eb5f21..111aca0dd 100644
--- a/template/en/default/account/auth/login-small.html.tmpl
+++ b/template/en/default/account/auth/login-small.html.tmpl
@@ -72,7 +72,9 @@
[%+ "checked" IF Param('rememberlogin') == "defaulton" %]>
<label for="Bugzilla_remember[% qs_suffix %]">Remember</label>
[% END %]
- <input type="submit" name="GoAheadAndLogIn" value="Log in"
+ <input type="hidden" name="Bugzilla_login_token"
+ value="[% get_login_request_token() FILTER html %]">
+ <input type="submit" name="GoAheadAndLogIn" value="Log in"
id="log_in[% qs_suffix %]">
<a href="#" id="hide_mini_login[% qs_suffix FILTER html %]"
onclick="return hide_mini_login_form('[% qs_suffix %]')">[x]</a>
diff --git a/template/en/default/account/auth/login.html.tmpl b/template/en/default/account/auth/login.html.tmpl
index 0aac403a5..4501a3962 100644
--- a/template/en/default/account/auth/login.html.tmpl
+++ b/template/en/default/account/auth/login.html.tmpl
@@ -83,8 +83,10 @@
[% PROCESS "global/hidden-fields.html.tmpl"
exclude="^Bugzilla_(login|password|restrictlogin)$" %]
+ <input type="hidden" name="Bugzilla_login_token"
+ value="[% get_login_request_token() FILTER html %]">
<input type="submit" name="GoAheadAndLogIn" value="Log in" id="log_in">
-
+
<p>
(Note: you should make sure cookies are enabled for this site.
Otherwise, you will be required to log in frequently.)
diff --git a/template/en/default/admin/sudo.html.tmpl b/template/en/default/admin/sudo.html.tmpl
index 676959c34..beb7ba510 100644
--- a/template/en/default/admin/sudo.html.tmpl
+++ b/template/en/default/admin/sudo.html.tmpl
@@ -81,9 +81,10 @@
<p>
Finally, enter <label for="Bugzilla_password">your [% terms.Bugzilla %]
password</label>:
- <input type="hidden" name="Bugzilla_login" value="
- [%- user.login FILTER html %]">
+ <input type="hidden" name="Bugzilla_login" value="[% user.login FILTER html %]">
<input type="password" id="Bugzilla_password" name="Bugzilla_password" size="20">
+ <input type="hidden" name="Bugzilla_login_token"
+ value="[% login_request_token FILTER html %]">
<br>
This is done for two reasons. First of all, it is done to reduce
the chances of someone doing large amounts of damage using your
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index cddf1c5b7..a39bb91b7 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -233,6 +233,15 @@
[% Hook.process("auth_failure") %]
+ [% ELSIF error == "auth_untrusted_request" %]
+ [% title = "Untrusted Authentication Request" %]
+ You tried to log in using the <em>[% login FILTER html %]</em> account,
+ but [% terms.Bugzilla %] is unable to trust your request. Make sure
+ your web browser accepts cookies and that you haven't been redirected
+ here from an external web site.
+ <a href="index.cgi?GoAheadAndLogIn=1">Click here</a> if you really want
+ to log in.
+
[% ELSIF error == "attachment_deletion_disabled" %]
[% title = "Attachment Deletion Disabled" %]
Attachment deletion is disabled on this installation.