From 72cb2bc73e71f54c2223bb78af29fee888590b53 Mon Sep 17 00:00:00 2001 From: "mkanat%bugzilla.org" <> Date: Sun, 13 Dec 2009 20:46:24 +0000 Subject: 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 r=LpSolit, a=LpSolit --- Bugzilla/Auth.pm | 56 +++++++++++++++++++++++++++++- Bugzilla/Auth/Verify/DB.pm | 46 ++++++++++++++++--------- Bugzilla/Config/Common.pm | 11 +++++- Bugzilla/Config/Core.pm | 3 +- Bugzilla/Constants.pm | 10 ++++++ Bugzilla/DB/Schema.pm | 20 +++++++++++ Bugzilla/User.pm | 85 ++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 212 insertions(+), 19 deletions(-) (limited to 'Bugzilla') 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 element, which specifies +how many times the account has failed to log in within the lockout +period (see L). This is used to warn the user when +he is getting close to being locked out. + =head2 C This is an optional more-specific version of C. @@ -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. +=head2 C + +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). + +The hashref will also contain a C element, representing the +L whose account is locked out. + =head1 LOGIN TYPES The C 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 # Dennis Melentyev # Akamai Technologies +# Elliotte Martin 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 + +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 + +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, C, and C. + +=item C + +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. + +=back + =head2 Other Methods =over -- cgit v1.2.3-24-g4f1b