Bug 1157395: CSRF in log in form

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<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..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" %]>
       <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 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)$" %]
 
+  <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 c96a68ec1..9c0605567 100644
--- a/template/en/default/admin/sudo.html.tmpl
+++ b/template/en/default/admin/sudo.html.tmpl
@@ -82,9 +82,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 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 <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 == "auth_invalid_token" %]
     [% title = 'A token error occurred' %]
     The token is not valid. It could be because you loaded this page more than