From 283be21f66e638667bc2ec7720cab459ecf1f698 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Fri, 24 Apr 2015 16:56:26 +0100 Subject: Bug 1157395: CSRF in log in form --- Bugzilla/Auth/Login/CGI.pm | 43 +++++++++++++++++++--- Bugzilla/CGI.pm | 13 +++++++ Bugzilla/Template.pm | 5 +++ Bugzilla/Util.pm | 10 ++++- relogin.cgi | 18 ++++++++- .../en/default/account/auth/login-small.html.tmpl | 4 +- template/en/default/account/auth/login.html.tmpl | 4 +- template/en/default/admin/sudo.html.tmpl | 5 ++- template/en/default/global/user-error.html.tmpl | 9 +++++ 9 files changed, 99 insertions(+), 12 deletions(-) diff --git a/Bugzilla/Auth/Login/CGI.pm b/Bugzilla/Auth/Login/CGI.pm index 8e877b951..240698277 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 $username = trim(delete $params->{"Bugzilla_login"}); - my $password = delete $params->{"Bugzilla_password"}; + 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'); - if (!defined $username || !defined $password) { - return { failure => AUTH_NODATA }; + 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($login) || !defined($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 3664fca81..608d612b8 100644 --- a/Bugzilla/Template.pm +++ b/Bugzilla/Template.pm @@ -1040,6 +1040,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]) : ''; + }, + 'get_api_token' => sub { return '' unless Bugzilla->user->id; 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 + +Tells you whether or not the current usage mode is WebServices related +such as JSONRPC or XMLRPC. + =item C Returns either the C or C parameter, depending on the diff --git a/relogin.cgi b/relogin.cgi index 6eb798205..cfbb52b34 100755 --- a/relogin.cgi +++ b/relogin.cgi @@ -58,7 +58,7 @@ elsif ($action eq 'prepare-sudo') { object => 'sudo_session' } ); } - + # Do not try to start a new session if one is already in progress! if (defined(Bugzilla->sudoer)) { ThrowUserError('sudo_in_progress', { target => $user->login }); @@ -67,6 +67,22 @@ 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); + + # The user ID must not be set when generating the token, because + # that information will not be available when validating it. + local Bugzilla->user->{userid} = 0; + $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" %]> [% END %] - + [x] diff --git a/template/en/default/account/auth/login.html.tmpl b/template/en/default/account/auth/login.html.tmpl index dac24bcdc..922a55bd4 100644 --- a/template/en/default/account/auth/login.html.tmpl +++ b/template/en/default/account/auth/login.html.tmpl @@ -84,8 +84,10 @@ [% PROCESS "global/hidden-fields.html.tmpl" exclude="^Bugzilla_(login|password|restrictlogin)$" %] + - +

(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 c96a68ec1..9c0605567 100644 --- a/template/en/default/admin/sudo.html.tmpl +++ b/template/en/default/admin/sudo.html.tmpl @@ -82,9 +82,10 @@

Finally, enter : - + +
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 250ab0e1d..5e83eef14 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -244,6 +244,15 @@ [% Hook.process("auth_failure") %] + [% ELSIF error == "auth_untrusted_request" %] + [% title = "Untrusted Authentication Request" %] + You tried to log in using the [% login FILTER html %] 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. + Click here if you really want + to log in. + [% ELSIF error == "auth_invalid_token" %] [% title = 'A token error occurred' %] The token is not valid. It could be because you loaded this page more than -- cgit v1.2.3-24-g4f1b