Bug 713926: (CVE-2014-1517) [SECURITY] Login form lacks CSRF protection

r=dkl a=justdave

12 files changed, 130 insertions, 62 deletions

diff --git a/Bugzilla/Auth.pm b/Bugzilla/Auth.pm
index 6b291d8ae..42e4ee784 100644
--- a/Bugzilla/Auth.pm
+++ b/Bugzilla/Auth.pm
@@ -153,7 +153,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 a55275a54..f9e3bbf18 100644
--- a/Bugzilla/Auth/Login/CGI.pm
+++ b/Bugzilla/Auth/Login/CGI.pm
@@ -17,19 +17,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/Auth/Persist/Cookie.pm b/Bugzilla/Auth/Persist/Cookie.pm
index 5a9857cba..6f4eac96d 100644
--- a/Bugzilla/Auth/Persist/Cookie.pm
+++ b/Bugzilla/Auth/Persist/Cookie.pm
@@ -54,6 +54,10 @@ sub persist_login {
 
     $dbh->bz_commit_transaction();
 
+    # We do not want WebServices to generate login cookies.
+    # All we need is the login token for User.login.
+    return $login_cookie if i_am_webservice();
+
     # Prevent JavaScript from accessing login cookies.
     my %cookieargs = ('-httponly' => 1);
 
diff --git a/Bugzilla/CGI.pm b/Bugzilla/CGI.pm
index d7e81d793..48b4fb0bf 100644
--- a/Bugzilla/CGI.pm
+++ b/Bugzilla/CGI.pm
@@ -291,7 +291,8 @@ sub header {
     my $self = shift;
 
     my %headers;
-   
+    my $user = Bugzilla->user;
+
     # If there's only one parameter, then it's a Content-Type.
     if (scalar(@_) == 1) {
         %headers = ('-type' => shift(@_));
@@ -304,6 +305,18 @@ sub header {
         $headers{'-content_disposition'} = $self->{'_content_disp'};
     }
 
+    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}})) {
         $headers{'-cookie'} = $self->{Bugzilla_cookie_list};
diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm
index b8fcae107..56d31dd2d 100644
--- a/Bugzilla/Template.pm
+++ b/Bugzilla/Template.pm
@@ -920,6 +920,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/WebService.pm b/Bugzilla/WebService.pm
index 9638d1132..ebad7930a 100644
--- a/Bugzilla/WebService.pm
+++ b/Bugzilla/WebService.pm
@@ -141,9 +141,7 @@ There are various ways to log in:
 =item C<User.login>
 
 You can use L<Bugzilla::WebService::User/login> to log in as a Bugzilla
-user. This issues standard HTTP cookies that you must then use in future
-calls, so your client must be capable of receiving and transmitting
-cookies.
+user. This issues a token that you must then use in future calls.
 
 =item C<Bugzilla_login> and C<Bugzilla_password>
 
@@ -163,30 +161,28 @@ WebService method to perform a login:
 =item C<Bugzilla_restrictlogin> (boolean) - Optional. If true,
 then your login will only be valid for your IP address.
 
-=item C<Bugzilla_rememberlogin> (boolean) - Optional. If true,
-then the cookie sent back to you with the method response will
-not expire.
-
 =back
 
-The C<Bugzilla_restrictlogin> and C<Bugzilla_rememberlogin> options
-are only used when you have also specified C<Bugzilla_login> and
-C<Bugzilla_password>.
-
-Note that Bugzilla will return HTTP cookies along with the method
-response when you use these arguments (just like the C<User.login> method
-above).
+The C<Bugzilla_restrictlogin> option is only used when you have also
+specified C<Bugzilla_login> and C<Bugzilla_password>.
 
-For REST, you may also use the C<username> and C<password> variable
+For REST, you may also use the C<login> and C<password> variable
 names instead of C<Bugzilla_login> and C<Bugzilla_password> as a
-convenience.
+convenience. You may also use C<token> instead of C<Bugzilla_token>.
+
+=item C<Bugzilla_token>
+
+B<Added in Bugzilla 5.0>
+
+You can specify C<Bugzilla_token> as argument to any WebService method,
+and you will be logged in as that user if the token is correct. This is
+the token returned when calling C<User.login> mentioned above.
 
-=item B<Added in Bugzilla 5.0>
+An error is thrown if you pass an invalid token and you will need to log
+in again to get a new token.
 
-An error is now thrown if you pass invalid cookies or an invalid token.
-You will need to log in again to get new cookies or a new token. Previous
-releases simply ignored invalid cookies and token support was added in
-Bugzilla B<5.0>.
+Token support was added in Bugzilla B<5.0> and support for login cookies
+has been dropped for security reasons.
 
 =back
 
diff --git a/Bugzilla/WebService/User.pm b/Bugzilla/WebService/User.pm
index f69ae8ed4..f8358f78d 100644
--- a/Bugzilla/WebService/User.pm
+++ b/Bugzilla/WebService/User.pm
@@ -51,7 +51,6 @@ use constant MAPPED_RETURNS => {
 
 sub login {
     my ($self, $params) = @_;
-    my $remember = $params->{remember};
 
     # Username and password params are required 
     foreach my $param ("login", "password") {
@@ -59,33 +58,18 @@ sub login {
             || ThrowCodeError('param_required', { param => $param });
     }
 
-    # Convert $remember from a boolean 0/1 value to a CGI-compatible one.
-    if (defined($remember)) {
-        $remember = $remember? 'on': '';
-    }
-    else {
-        # Use Bugzilla's default if $remember is not supplied.
-        $remember =
-            Bugzilla->params->{'rememberlogin'} eq 'defaulton'? 'on': '';
-    }
-
     # Make sure the CGI user info class works if necessary.
     my $input_params = Bugzilla->input_params;
     $input_params->{'Bugzilla_login'} =  $params->{login};
     $input_params->{'Bugzilla_password'} = $params->{password};
-    $input_params->{'Bugzilla_remember'} = $remember;
+    $input_params->{'Bugzilla_restrictlogin'} = $params->{restrict_login};
 
     my $user = Bugzilla->login();
 
     my $result = { id => $self->type('int', $user->id) };
 
-    # We will use the stored cookie value combined with the user id
-    # to create a token that can be used with future requests in the
-    # query parameters
-    my $login_cookie = first { $_->name eq 'Bugzilla_logincookie' }
-                              @{ Bugzilla->cgi->{'Bugzilla_cookie_list'} };
-    if ($login_cookie) {
-        $result->{'token'} = $user->id . "-" . $login_cookie->value;
+    if ($user->{_login_token}) {
+        $result->{'token'} = $user->id . "-" . $user->{_login_token};
     }
 
     return $result;
@@ -464,13 +448,9 @@ etc. This method logs in an user.
 
 =item C<password> (string) - The user's password.
 
-=item C<remember> (bool) B<Optional> - if the cookies returned by the
-call to login should expire with the session or not.  In order for
-this option to have effect the Bugzilla server must be configured to
-allow the user to set this option - the Bugzilla parameter
-I<rememberlogin> must be set to "defaulton" or
-"defaultoff". Addionally, the client application must implement
-management of cookies across sessions.
+=item C<restrict_login> (bool) B<Optional> - If set to a true value,
+the token returned by this method will only be valid from the IP address
+which called this method.
 
 =back
 
@@ -478,12 +458,9 @@ management of cookies across sessions.
 
 On success, a hash containing two items, C<id>, the numeric id of the
 user that was logged in, and a C<token> which can be passed in
-the parameters as authentication in other calls. A set of http cookies
-is also sent with the response. These cookies *or* the token can be sent
+the parameters as authentication in other calls. The token can be sent
 along with any future requests to the webservice, for the duration of the
-session. Note that cookies are not accepted for GET requests for JSONRPC
-and REST for security reasons. You may, however, use the token or valid
-login parameters for those requests.
+session, i.e. till L<User.logout|/logout> is called.
 
 =item B<Errors>
 
@@ -509,6 +486,19 @@ A login or password parameter was not provided.
 
 =back
 
+=item B<History>
+
+=over
+
+=item C<remember> was removed in Bugzilla B<5.0> as this method no longer
+creates a login cookie.
+
+=item C<restrict_login> was added in Bugzilla B<5.0>.
+
+=item C<token> was added in Bugzilla B<5.0>.
+
+=back
+
 =back
 
 =head2 logout
diff --git a/relogin.cgi b/relogin.cgi
index 52944a811..0c1cb9ad6 100755
--- a/relogin.cgi
+++ b/relogin.cgi
@@ -52,6 +52,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 32dbe431b..5868b8671 100644
--- a/template/en/default/account/auth/login-small.html.tmpl
+++ b/template/en/default/account/auth/login-small.html.tmpl
@@ -46,7 +46,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="#" onclick="return hide_mini_login_form('[% qs_suffix %]')">[x]</a>
   </form>
diff --git a/template/en/default/account/auth/login.html.tmpl b/template/en/default/account/auth/login.html.tmpl
index bf20edb8b..b6da535cc 100644
--- a/template/en/default/account/auth/login.html.tmpl
+++ b/template/en/default/account/auth/login.html.tmpl
@@ -76,8 +76,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 d4faf4ea7..bf0cd7b6f 100644
--- a/template/en/default/admin/sudo.html.tmpl
+++ b/template/en/default/admin/sudo.html.tmpl
@@ -68,9 +68,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" required>
+      <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 11d4f8ad1..0b5d66764 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -219,6 +219,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.