Bug 355283: Lock out a user account on a particular IP for 30 minutes if they fail to log in 5 times from that IP.

Patch by Max Kanat-Alexander <mkanat@bugzilla.org> r=LpSolit, a=LpSolit

7 files changed, 212 insertions, 19 deletions

diff --git a/Bugzilla/Auth.pm b/Bugzilla/Auth.pm
index 8e18f8699..40150f0ed 100644
--- a/Bugzilla/Auth.pm
+++ b/Bugzilla/Auth.pm
@@ -32,6 +32,9 @@ use fields qw(
 
 use Bugzilla::Constants;
 use Bugzilla::Error;
+use Bugzilla::Mailer;
+use Bugzilla::Util qw(datetime_from);
+use Bugzilla::User::Setting ();
 use Bugzilla::Auth::Login::Stack;
 use Bugzilla::Auth::Verify::Stack;
 use Bugzilla::Auth::Persist::Cookie;
@@ -162,7 +165,10 @@ sub _handle_login_result {
     # the password was just wrong. (This makes it harder for a cracker
     # to find account names by brute force)
     elsif ($fail_code == AUTH_LOGINFAILED or $fail_code == AUTH_NO_SUCH_USER) {
-        ThrowUserError("invalid_username_or_password");
+        my $remaining_attempts = MAX_LOGIN_ATTEMPTS 
+                                 - ($result->{failure_count} || 0);
+        ThrowUserError("invalid_username_or_password", 
+                       { remaining => $remaining_attempts });
     }
     # The account may be disabled
     elsif ($fail_code == AUTH_DISABLED) {
@@ -173,6 +179,40 @@ sub _handle_login_result {
         ThrowUserError("account_disabled",
             {'disabled_reason' => $result->{user}->disabledtext});
     }
+    elsif ($fail_code == AUTH_LOCKOUT) {
+        my $attempts = $user->account_ip_login_failures;
+
+        # We want to know when the account will be unlocked. This is 
+        # determined by the 5th-from-last login failure (or more/less than
+        # 5th, if MAX_LOGIN_ATTEMPTS is not 5).
+        my $determiner = $attempts->[scalar(@$attempts) - MAX_LOGIN_ATTEMPTS];
+        my $unlock_at = datetime_from($determiner->{login_time}, 
+                                      Bugzilla->local_timezone);
+        $unlock_at->add(minutes => LOGIN_LOCKOUT_INTERVAL);
+
+        # If we were *just* locked out, notify the maintainer about the
+        # lockout.
+        if ($result->{just_locked_out}) {
+            # We're sending to the maintainer, who may be not a Bugzilla 
+            # account, but just an email address. So we use the
+            # installation's default language for sending the email.
+            my $default_settings = Bugzilla::User::Setting::get_defaults();
+            my $template = Bugzilla->template_inner($default_settings->{lang});
+            my $vars = {
+                locked_user => $user,
+                attempts    => $attempts,
+                unlock_at   => $unlock_at,
+            };
+            my $message;
+            $template->process('email/lockout.txt.tmpl', $vars, \$message)
+                || ThrowTemplateError($template->error);
+            MessageToMTA($message);
+        }
+
+        $unlock_at->set_time_zone($user->timezone);
+        ThrowUserError('account_locked', 
+            { ip_addr => $determiner->{ip_addr}, unlock_at => $unlock_at });
+    }
     # If we get here, then we've run out of options, which shouldn't happen.
     else {
         ThrowCodeError("authres_unhandled", { value => $fail_code });
@@ -234,6 +274,11 @@ various fields to be used in the error message.
 
 An incorrect username or password was given.
 
+The hashref may also contain a C<failure_count> element, which specifies
+how many times the account has failed to log in within the lockout
+period (see L</AUTH_LOCKOUT>). This is used to warn the user when
+he is getting close to being locked out.
+
 =head2 C<AUTH_NO_SUCH_USER>
 
 This is an optional more-specific version of C<AUTH_LOGINFAILED>.
@@ -251,6 +296,15 @@ should never be communicated to the user, for security reasons.
 The user successfully logged in, but their account has been disabled.
 Usually this is throw only by C<Bugzilla::Auth::login>.
 
+=head2 C<AUTH_LOCKOUT>
+
+The user's account is locked out after having failed to log in too many
+times within a certain period of time (as specified by
+L<Bugzilla::Constants/LOGIN_LOCKOUT_INTERVAL>).
+
+The hashref will also contain a C<user> element, representing the
+L<Bugzilla:User> whose account is locked out.
+
 =head1 LOGIN TYPES
 
 The C<login> function (below) can do different types of login, depending
diff --git a/Bugzilla/Auth/Verify/DB.pm b/Bugzilla/Auth/Verify/DB.pm
index 695671a31..d8794472e 100644
--- a/Bugzilla/Auth/Verify/DB.pm
+++ b/Bugzilla/Auth/Verify/DB.pm
@@ -41,37 +41,51 @@ sub check_credentials {
     my $dbh = Bugzilla->dbh;
 
     my $username = $login_data->{username};
-    my $user_id  = login_to_id($username);
+    my $user = new Bugzilla::User({ name => $username });
 
-    return { failure => AUTH_NO_SUCH_USER } unless $user_id;
+    return { failure => AUTH_NO_SUCH_USER } unless $user;
 
-    $login_data->{bz_username} = $username;
-    my $password = $login_data->{password};
+    $login_data->{user} = $user;
+    $login_data->{bz_username} = $user->login;
+
+    if ($user->account_is_locked_out) {
+        return { failure => AUTH_LOCKOUT, user => $user };
+    }
 
-    trick_taint($username);
-    my ($real_password_crypted) = $dbh->selectrow_array(
-        "SELECT cryptpassword FROM profiles WHERE userid = ?",
-        undef, $user_id);
+    my $password = $login_data->{password};
+    my $real_password_crypted = $user->cryptpassword;
 
     # Using the internal crypted password as the salt,
     # crypt the password the user entered.
     my $entered_password_crypted = bz_crypt($password, $real_password_crypted);
- 
-    return { failure => AUTH_LOGINFAILED }
-        if $entered_password_crypted ne $real_password_crypted;
+
+    if ($entered_password_crypted ne $real_password_crypted) {
+        # Record the login failure
+        $user->note_login_failure();
+
+        # Immediately check if we are locked out
+        if ($user->account_is_locked_out) {
+            return { failure => AUTH_LOCKOUT, user => $user,
+                     just_locked_out => 1 };
+        }
+
+        return { failure => AUTH_LOGINFAILED,
+                 failure_count => scalar(@{ $user->account_ip_login_failures }),
+               };
+    } 
 
     # The user's credentials are okay, so delete any outstanding
-    # password tokens they may have generated.
-    Bugzilla::Token::DeletePasswordTokens($user_id, "user_logged_in");
+    # password tokens or login failures they may have generated.
+    Bugzilla::Token::DeletePasswordTokens($user->id, "user_logged_in");
+    $user->clear_login_failures();
 
     # If their old password was using crypt() or some different hash
     # than we're using now, convert the stored password to using
     # whatever hashing system we're using now.
     my $current_algorithm = PASSWORD_DIGEST_ALGORITHM;
     if ($real_password_crypted !~ /{\Q$current_algorithm\E}$/) {
-        my $new_crypted = bz_crypt($password);
-        $dbh->do('UPDATE profiles SET cryptpassword = ? WHERE userid = ?',
-                 undef, $new_crypted, $user_id);
+        $user->set_password($password);
+        $user->update();
     }
 
     return $login_data;
diff --git a/Bugzilla/Config/Common.pm b/Bugzilla/Config/Common.pm
index 95866b032..6924761f3 100644
--- a/Bugzilla/Config/Common.pm
+++ b/Bugzilla/Config/Common.pm
@@ -34,6 +34,7 @@ package Bugzilla::Config::Common;
 
 use strict;
 
+use Email::Address;
 use Socket;
 
 use Bugzilla::Util;
@@ -50,7 +51,7 @@ use base qw(Exporter);
        check_user_verify_class
        check_mail_delivery_method check_notification check_utf8
        check_bug_status check_smtp_auth check_theschwartz_available
-       check_maxattachmentsize
+       check_maxattachmentsize check_email
 );
 
 # Checking functions for the various values
@@ -94,6 +95,14 @@ sub check_regexp {
     return $@;
 }
 
+sub check_email {
+    my ($value) = @_;
+    if ($value !~ $Email::Address::mailbox) {
+        return "must be a valid email address.";
+    }
+    return "";
+}
+
 sub check_sslbase {
     my $url = shift;
     if ($url ne '') {
diff --git a/Bugzilla/Config/Core.pm b/Bugzilla/Config/Core.pm
index 91426b30a..1bfebfa69 100644
--- a/Bugzilla/Config/Core.pm
+++ b/Bugzilla/Config/Core.pm
@@ -43,7 +43,8 @@ sub get_param_list {
   {
    name => 'maintainer',
    type => 't',
-   default => 'THE MAINTAINER HAS NOT YET BEEN SET'
+   default => 'please.set.the.maintainer.parameter@administration.parameters',
+   checker => \&check_email,
   },
 
   {
diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm
index 0be6d1efa..e052d2ecb 100644
--- a/Bugzilla/Constants.pm
+++ b/Bugzilla/Constants.pm
@@ -55,6 +55,7 @@ use File::Basename;
     AUTH_LOGINFAILED
     AUTH_DISABLED
     AUTH_NO_SUCH_USER
+    AUTH_LOCKOUT
 
     USER_PASSWORD_MIN_LENGTH
 
@@ -149,6 +150,8 @@ use File::Basename;
 
     MAX_TOKEN_AGE
     MAX_LOGINCOOKIE_AGE
+    MAX_LOGIN_ATTEMPTS
+    LOGIN_LOCKOUT_INTERVAL
 
     SAFE_PROTOCOLS
     LEGAL_CONTENT_TYPES
@@ -227,6 +230,7 @@ use constant AUTH_ERROR => 2;
 use constant AUTH_LOGINFAILED => 3;
 use constant AUTH_DISABLED => 4;
 use constant AUTH_NO_SUCH_USER  => 5;
+use constant AUTH_LOCKOUT => 6;
 
 # The minimum length a password must have.
 use constant USER_PASSWORD_MIN_LENGTH => 6;
@@ -373,6 +377,12 @@ use constant MAX_TOKEN_AGE => 3;
 # How many days a logincookie will remain valid if not used.
 use constant MAX_LOGINCOOKIE_AGE => 30;
 
+# Maximum failed logins to lock account for this IP
+use constant MAX_LOGIN_ATTEMPTS => 5;
+# If the maximum login attempts occur during this many minutes, the
+# account is locked.
+use constant LOGIN_LOCKOUT_INTERVAL => 30;
+
 # Protocols which are considered as safe.
 use constant SAFE_PROTOCOLS => ('afs', 'cid', 'ftp', 'gopher', 'http', 'https',
                                 'irc', 'mid', 'news', 'nntp', 'prospero', 'telnet',
diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm
index f34f05e2f..a2df26425 100644
--- a/Bugzilla/DB/Schema.pm
+++ b/Bugzilla/DB/Schema.pm
@@ -23,6 +23,7 @@
 #                 Lance Larsh <lance.larsh@oracle.com>
 #                 Dennis Melentyev <dennis.melentyev@infopulse.com.ua>
 #                 Akamai Technologies <bugzilla-dev@akamai.com>
+#                 Elliotte Martin <emartin@everythingsolved.com>
 
 package Bugzilla::DB::Schema;
 
@@ -982,6 +983,25 @@ use constant ABSTRACT_SCHEMA => {
         ],
     },
 
+    login_failure => {
+        FIELDS => [
+            user_id    => {TYPE => 'INT3', NOTNULL => 1,
+                           REFERENCES => {TABLE  => 'profiles',
+                                          COLUMN => 'userid',
+                                          DELETE => 'CASCADE'}},
+            login_time => {TYPE => 'DATETIME', NOTNULL => 1},
+            ip_addr    => {TYPE => 'varchar(40)', NOTNULL => 1},
+        ],
+        INDEXES => [
+            # We do lookups by every item in the table simultaneously, but 
+            # having an index with all three items would be the same size as
+            # the table. So instead we have an index on just the smallest item, 
+            # to speed lookups.
+            login_failure_user_id_idx => ['user_id'],
+        ],
+    },
+
+
     # "tokens" stores the tokens users receive when a password or email
     #     change is requested.  Tokens provide an extra measure of security
     #     for these changes.
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 3e6f3b6ba..e8ea2878e 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -65,6 +65,11 @@ use base qw(Bugzilla::Object Exporter);
 # Constants
 #####################################################################
 
+# Used as the IP for authentication failures for password-lockout purposes
+# when there is no IP (for example, if we're doing authentication from the
+# command line for some reason).
+use constant NO_IP => '255.255.255.255';
+
 use constant USER_MATCH_MULTIPLE => -1;
 use constant USER_MATCH_FAILED   => 0;
 use constant USER_MATCH_SUCCESS  => 1;
@@ -247,6 +252,15 @@ sub is_disabled { $_[0]->disabledtext ? 1 : 0; }
 sub showmybugslink { $_[0]->{showmybugslink}; }
 sub email_disabled { $_[0]->{disable_mail}; }
 sub email_enabled { !($_[0]->{disable_mail}); }
+sub cryptpassword {
+    my $self = shift;
+    # We don't store it because we never want it in the object (we
+    # don't want to accidentally dump even the hash somewhere).
+    my ($pw) = Bugzilla->dbh->selectrow_array(
+        'SELECT cryptpassword FROM profiles WHERE userid = ?',
+        undef, $self->id);
+    return $pw;
+}
 
 sub set_authorizer {
     my ($self, $authorizer) = @_;
@@ -1655,6 +1669,54 @@ sub create {
     return $user;
 }
 
+###########################
+# Account Lockout Methods #
+###########################
+
+sub account_is_locked_out {
+    my $self = shift;
+    my $login_failures = scalar @{ $self->account_ip_login_failures };
+    return $login_failures >= MAX_LOGIN_ATTEMPTS ? 1 : 0;
+}
+
+sub note_login_failure {
+    my $self = shift;
+    my $ip_addr = Bugzilla->cgi->remote_addr || NO_IP;
+    trick_taint($ip_addr);
+    Bugzilla->dbh->do("INSERT INTO login_failure (user_id, ip_addr, login_time)
+                       VALUES (?, ?, LOCALTIMESTAMP(0))",
+                      undef, $self->id, $ip_addr);
+    delete $self->{account_ip_login_failures};
+}
+
+sub clear_login_failures {
+    my $self = shift;
+    my $ip_addr = Bugzilla->cgi->remote_addr || NO_IP;
+    trick_taint($ip_addr);
+    Bugzilla->dbh->do(
+        'DELETE FROM login_failure WHERE user_id = ? AND ip_addr = ?',
+        undef, $self->id, $ip_addr);
+    delete $self->{account_ip_login_failures};
+}
+
+sub account_ip_login_failures {
+    my $self = shift;
+    my $dbh = Bugzilla->dbh;
+    my $time = $dbh->sql_interval(LOGIN_LOCKOUT_INTERVAL, 'MINUTE');
+    my $ip_addr = Bugzilla->cgi->remote_addr || NO_IP;
+    trick_taint($ip_addr);
+    $self->{account_ip_login_failures} ||= Bugzilla->dbh->selectall_arrayref(
+        "SELECT login_time, ip_addr, user_id FROM login_failure
+          WHERE user_id = ? AND login_time > LOCALTIMESTAMP(0) - $time
+                AND ip_addr = ?
+       ORDER BY login_time", {Slice => {}}, $self->id, $ip_addr);
+    return $self->{account_ip_login_failures};
+}
+
+###############
+# Subroutines #
+###############
+
 sub is_available_username {
     my ($username, $old_username) = @_;
 
@@ -1848,6 +1910,29 @@ groups.
 
 =back
 
+=head2 Account Lockout
+
+=over
+
+=item C<account_is_locked_out>
+
+Returns C<1> if the account has failed to log in too many times recently,
+and thus is locked out for a period of time. Returns C<0> otherwise.
+
+=item C<account_ip_login_failures>
+
+Returns an arrayref of hashrefs, that contains information about the recent
+times that this account has failed to log in from the current remote IP.
+The hashes contain C<ip_addr>, C<login_time>, and C<user_id>.
+
+=item C<note_login_failure>
+
+This notes that this account has failed to log in, and stores the fact
+in the database. The storing happens immediately, it does not wait for
+you to call C<update>.
+
+=back
+
 =head2 Other Methods
 
 =over