Closed Bug 355283 Opened 18 years ago Closed 15 years ago

user login password can be brute forced because there is no lockout policy

Categories

(Bugzilla :: User Accounts, defect)

Product:
Bugzilla
Component:
User Accounts
Version:
2.23
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 3.6

People

(Reporter: timeless, Assigned: mkanat)

References

Details

(Whiteboard: [sg:moderate][wanted-bmo])

Attachments

(1 file, 8 obsolete files)

v8.1
(deleted), patch
LpSolit
: review+
Details | Diff | Splinter Review 
dveditz and i joked about guessing the password for a certain account. then it occured to me that we don't actually prevent brute forcing passwords.

it's true that eventually new accounts will be token based, but that doesn't protect old accounts. or dictionary attacks on normal accounts.

notes:
1. password policy should probably be optional (just so that people can turn it off if they're feeling silly) - but i don't care if it isn't optional
2. password policy should probably be configurable (no more than 5, 10, bad passwords before you need to use a token or contact an administrator).
3. some auditing should be retained (optional?)

i'm not really sure what the policy should be. obviously we don't want someone sending thousands of requests to bugzilla. and perhaps if bugzilla detects 5 (or 4) requests to multiple accounts in a short period, it should also notify an admin. just in case someone decides to try a small set of common passwords against a large group of users.
Group: webtools-security → bugzilla-security
Group: bugzilla-security → webtools-security
Group: webtools-security → bugzilla-security
Attached patch v1 (obsolete) (deleted) — — Splinter Review 
If the password check fails five times within one hour for a given user, the user's account will be locked and the user will be sent an email with a link to unlock their account.  The email includes the IP addresses of all the people who attempted those five logins.

A message will be displayed to the logging-in user that they have been
locked out and that they have been sent an email to unlock their account.
Assignee: user-accounts → elliotte_martin
Attachment #341219 - Flags: review?(mkanat)
Shouldn't you ban the IP address of the attacker(*) too, or at least report this IP address to admins? If I want to bother one user, all I have to do is to attack his account permanently, like every day, so that he always has to go read his emails to find the one with the token to unlock his account. Note that the IP address of the attacker could change every day if he uses DHCP, in which case my proposal wouldn't work very well.

(*) An attacker could be defined as someone trying to log in 5 times in a row, every time unsuccessfully and independently of the user account. This would let us detect those who randomly choose accounts to access.
Severity: major → normal
OS: Windows XP → All
Hardware: PC → All
For what its worth I've always been a fan of delays.  Indefinite lockout makes it possible to DoS a large set of users relatively easily.  

How about something like: delay = 4^(number of attempts in the last hour)

You would then have to wait for "delay" seconds before you could try to log in again.

DoS is still possible but its pretty tedious.  You can still combine this with an absolute lockout to prevent patient, low-intensity attacks.
Whiteboard: [sg:moderate]
Unless I miss something, I see nothing in the patch which prevents the user to log in successfully and use Bugzilla as usual. Why does he need to unlock his account using a token?
check_account_locked in Auth.pm prevents the user from logging in after 5 unsuccesfull attempts.
This method is only called if the error code is AUTH_LOGINFAILED.
You're right, if they know the correct password they don't need the token.
The idea with the token was to allow them to unlock the account and get back into bugzilla.  Then they could reset their password if they had forgotten it.
On second thought, I did miss something.  The code needs to be modified so that it checks to see if the account has been locked before allowing the user to login in.  I'll submit a new patch.
Attached patch v2 (obsolete) (deleted) — — Splinter Review 
Corrects hole in logic of first patch.  User can't login until the token is applied.
Attachment #341219 - Attachment is obsolete: true
Attachment #344236 - Flags: review?(LpSolit)
Attachment #341219 - Flags: review?(mkanat)
Attachment #344236 - Flags: review?(LpSolit) → review-
Comment on attachment 344236 [details] [diff] [review]
v2

>Index: token.cgi

>+  if ( ($action eq 'unlock_account') && $tokentype ne 'unlock' ) {
>+     Bugzilla::Token::Cancel($token, "wrong_token_for_unlocking_account");
>+    ThrowUserError("wrong_token_for_unlocking_account");
>+  }
> }

Please use the standard indentation (4 spaces). 2 spaces was used years ago.


>+sub unlock_account {

>+    trick_taint($token);

The token has already been detainted. If that wasn't the case, this means its type wouldn't have been checked yet, which would be a huge bug.

   
>+    my ($userid) = $dbh->selectrow_array('SELECT userid FROM tokens
>+                                           WHERE token = ?', undef, $token);

Use Bugzilla::Token::GetTokenData($token) to get this information.


>+    $dbh->do('DELETE FROM login_activity WHERE user_id = ?', undef, $userid );

Nit: extra whitespace after $userid.


>+    $dbh->do('DELETE FROM tokens WHERE token = ?', undef, $token);

Use delete_token($token) to delete tokens.


>+    my $vars = {'message' => "account_unlocked"};

$vars is already defined. Look for "our $vars".



>Index: Bugzilla/Auth.pm

>+    my $username = $login_info->{username};

This will only be useful if $login_info = $self->{_verifier}->check_credentials($login_info) fails, so you should move this line into the |if ($self->{_info_getter}->{successful}->requires_verification)| block, because if requires_verification() returns false, you cannot get any AUTH_LOGINFAILED error later.


>     if ($login_info->{failure}) {
>+        $login_info->{username} = $username;
>         return $self->_handle_login_result($login_info, $type);
>     }

Adding $login_info->{username} = $username here is useless as you cannot come here with AUTH_LOGINFAILED as error. Only the previous one makes sense.


>+        #make sure account is not locked
>+        my $attempts = $dbh->selectrow_array( q{SELECT COUNT(*)
>+                                                FROM login_activity 
>+                                                WHERE user_id = ?},
>+                                             undef, $user->id);

IMO, this check should be done at the end of login(), right before $user->set_authorizer($self), and call _handle_login_result() with a new AUTH_LOCKED error code if $attempts >= MAX_LOGIN_ATTEMPTS (>= rather than == in case the attacker manages to generate a race condition). I think $user->set_authorizer($self) should only be called if *ALL* tests are OK.


>+                                        
>+        ThrowUserError("account_locked") if ($attempts == MAX_LOGIN_ATTEMPTS);

Remove the 20 whitespaces or so of the first line above and remove this test as commented above (it should be in login()).



>Index: Bugzilla/Token.pm

>+sub IssueTokenForLockedAccounts {
>+    my ($user_id, $login_activity) = @_;

Pass the user object, not the user ID.


>+    my $dbh = Bugzilla->dbh;

$dbh is unused.


>+    my ($token, $token_ts) = _create_token($user_id, 'unlock', 'account_locked');

Is there no more interesting data than 'account_locked' to store in the DB? Each token has a creation time stored in the DB already. If you replace 'account_locked' by the IP of the attacker, you no longer need the login_activity DB table AFAIK.


>+    my $user = new Bugzilla::User($user_id);

Pass the user object to the subroutine.


>+    $vars->{'emailaddress'} = $user->email;

Useless. The user object is accessible from templates directly. Also, what happens if you cannot access your old email address and you are requesting a new one? The email with the token with be sent to the unaccessible old email address (this happens e.g. when you leave a company and your email address is no longer available). If you didn't change your email address one time, you are lost.


>+    $vars->{'max_token_age'} = MAX_TOKEN_AGE;

Constants are also directly accessible from templates.


>+    $vars->{'token_ts'} = $token_ts;

Don't pass token_ts to templates, see bug 452519.


>+    $vars->{'max_logins'} = MAX_LOGIN_ATTEMPTS;

Same remark about constants.


>+    $template->process("account/password/lockedout-password.txt.tmpl", 
>+                        $vars, \$message)
>+      || ThrowTemplateError($template->error());

Please reset Bugzilla->template_inner().


>+=item C<IssueTokenForLockedAccounts($user_id, $ip_addrs)>
>+
>+ Description: Sends a token per email to the given user. This token
>+              can be used to reset the password if the user account has
>+              been locked from too many failed logins.

Huh?? You don't reset the password.


>+ Params:      $user_id  - User ID of the user requesting a new password.

Same remark.



>Index: Bugzilla/User.pm

>+sub check_account_locked {

>+    $dbh->do(qq{DELETE FROM login_activity
>+                WHERE user_id = ? AND login_time < NOW() - $time},

Please write NOW - login_time > $time. This looks clearer to me.


>+    if ($attempts == MAX_LOGIN_ATTEMPTS) {

Could we write >= here just for safety?


>+    my $cgi = Bugzilla->cgi;

Nit: please define $cgi at the top of the method.


>+    $dbh->do("INSERT INTO login_activity (user_id, ip_addr, login_time) 
>+              VALUES (?, ?, now())", undef, ($self->id, $ip_addr));

now() -> NOW().


>+        #send email for unlocking the account
>+        Bugzilla::Token::IssueTokenForLockedAccounts(
>+            $self->id, $login_activity);

The lifetime of a token is 3 days. If the user doesn't unlock his account within 3 days, the token is automatically deleted and there is no way to get a new token. Your account is locked forever. This makes me think that admins must have a way to 1) know if an account is locked or not in editusers.cgi UI, and 2) unlock accounts if necessary (e.g. because the old email address is no longer available or the token's lifetime expired or the user accidentally lost his email (or got caught e.g. as spam)).



>Index: Bugzilla/DB/Schema.pm

>+    login_activity => {

As I said, this table is useless as the token already has all the needed information.



>Index: template/en/default/global/messages.html.tmpl

>+ [% ELSIF message_tag == "account_unlocked" %]
>+    [% title = "Account Unlocked" %]
>+    [% url = "index.cgi?GoAheadAndLogIn=1" %]
>+    [% link = "Log in again." %]

Are url and link magical variables or what? What are they suppose to do??



>Index: template/en/default/global/user-error.html.tmpl

>+  [% ELSIF error == "account_locked" %]
>+    [% title = "Account Locked" %]
>+    Your account has been locked as you have exceeded the maximum number of

"you or someone else" would be more appropriate. If we were sure the user was responsible for all these attempts, we wouldn't have such a policy.



>+++ template/en/default/account/password/lockedout-password.txt.tmpl	2008-09-27 16:36:17.718750000 -0700

>+[% expiration_ts = token_ts + (max_token_age * 86400) %]

Don't do this. We now use FILTER time(), which means you have to use the same way as for bug 452519, see my patch there.


>+Your [%+ terms.Bugzilla %] account was locked, as the maximum number ([%+ max_logins +%]) of 

Remove these useless ++ in the directive. They are useless.


>+[% FOREACH login = login_activity %]
>+[% login.ip_addr FILTER html %] at [% login.login_time FILTER html %]
>+
>+[% END %]

Remove the useless empty line and fix the indentation (the indentation will also appear in the email, which is nicer IMO)


>+If you are not the person who made this request, or if you do nothing, the

Nobody requested anything here. Also, if you do nothing, you are in huge trouble, unless you relax the account lock after 3 days, which would make sense. An attacker won't do a lot of harm if he can only try 5 passwords every 3 days. So the account should be automatically unlocked after 3 days.


>+request will lapse after [%+ max_token_age +%] days (at precisely 

Remove ++ and write [% constants.MAX_TOKEN_AGE %] instead.


>+[%+ time2str("%H:%M on the %o of %B, %Y", expiration_ts) -%]).

Same as above, we now use FILTER time(), see bug 452519.
(In reply to comment #12)
Thanks for you review.  I fixed the major hole you pointed out earlier and submitted it for your review thinking that was all that needed to be done.  This code was accepted by Max for the Mozilla branch, so I went ahead and submitted it upstream.  You're right that the new table is not necessary if you only want to store 1 IP address.  It was Max's idea to use the new table to store 5 IP addresses.  Unfortunately, I don't have the time now to do a major re-design of this.  Feel free to take this over if you want.
(In reply to comment #13)
> submitted it upstream.  You're right that the new table is not necessary if you
> only want to store 1 IP address.  It was Max's idea to use the new table to
> store 5 IP addresses.

Each token contains 1 IP address. With all 5 tokens, you have the 5 IP addresses you want. You still don't need the extra table.
Assignee: elliotte_martin → user-accounts
Whiteboard: [sg:moderate] → [sg:moderate][wanted-bmo]
Comment on attachment 344236 [details] [diff] [review]
v2

>+    Bugzilla:Token::IssueTokenForLockedAccounts ($user_id, $ipaddrs, login_times);

Should be Bugzilla::Token, and please get rid of that extra space.
I'd like to nominate this bug for the next security release, as it has a wip patch, and bmo already has a patch like this.
Flags: blocking3.2.2?
Honestly, this is too much on the level of enhancement to probably make any point release.
Flags: blocking3.2.2? → blocking3.2.2-
Target Milestone: --- → Bugzilla 4.0
Assignee: user-accounts → mkanat
Status: NEW → ASSIGNED
Attached patch v3 (obsolete) (deleted) — — Splinter Review 
Okay, here's a major polishing-up of this patch.

I realized it only makes sense for the DB login method, since that's the only time that we're actually guaranteed to have a User object when login fails. Other login types can do their own lockout controls.

We do need the separate table, that's how we check how many login attempts there have been. We don't use tokens for that. We also want to know the IP of each separate attempt, not just the IP of the very last attempt.
Attachment #344236 - Attachment is obsolete: true
Attachment #359385 - Flags: review?(LpSolit)
To be sure: is this really a 4.0-only security patch? No backport planned for branches?
(In reply to comment #19)
> To be sure: is this really a 4.0-only security patch? No backport planned for
> branches?

I definitely want it on the branch, and I know the Mozilla Security Group does, too.
Flags: blocking3.2.3?
(In reply to comment #20)
> I definitely want it on the branch, and I know the Mozilla Security Group does,
> too.

Woah, the Mozilla Security Group cannot decide for us here; sorry. ;)
... and just because bmo has a custom patch for this already deployed doesn't mean this bug is any less important, as there are lots of Bugzilla installations that could benefit from this patch.
(In reply to comment #19)
> To be sure: is this really a 4.0-only security patch? No backport planned for
> branches?

  Yes. It's way too much of an enhancement to be backported, and it's not really a secret security issue--this is in the security group to keep track of it, not because it's secret. It will be a security enhancement, like how randomizing cookies was a security enhancement in 2.22.
Flags: blocking3.2.3? → blocking3.2.3-
why not in 3.4?  Did we already freeze or something?
(In reply to comment #24)
> why not in 3.4?  Did we already freeze or something?

  No, I want it for 3.4, that's what "4.0" means at the moment still.
Target Milestone: Bugzilla 4.0 → Bugzilla 3.4
Ah, ok.  Sounds good to me then. :)
Please find a way to track security bugs that doesn't involve hiding them unnecessarily.  The core-security group uses the whiteboard field, for example.
Group: bugzilla-security
Severity: normal → major
Flags: blocking3.4?
Flags: blocking3.4? → blocking3.4+
Attachment #359385 - Flags: review?(LpSolit) → review?(bugzilla)
Attachment #359385 - Flags: review?(bugzilla) → review?(LpSolit)
Comment on attachment 359385 [details] [diff] [review]
v3

The patch no longer applies cleanly due to a change in format_time().
Attached patch v4 (obsolete) (deleted) — — Splinter Review 
Okay, this one applies. I also fixed various bugs that I found in the previous patch.
Attachment #359385 - Attachment is obsolete: true
Attachment #373560 - Flags: review?(LpSolit)
Attachment #359385 - Flags: review?(LpSolit)
Attachment #373560 - Flags: review?(LpSolit) → review-
Comment on attachment 373560 [details] [diff] [review]
v4

>Index: token.cgi

>+sub unlock_account {

>+    $dbh->bz_start_transaction();

$dbh is not defined in this subroutine.



>Index: Bugzilla/Token.pm

>+# Generates a random token, adds it to the tokens table, and sends it
>+# to the user with instructions for using it to change their password.

This comment is wrong (copy/paste error).


>+sub IssueAccountUnlockToken {
>+    my ($user, $unlock_at) = @_;
>+    my $dbh = Bugzilla->dbh;

$dbh is never used here.


>+    my ($token) = _create_token($user->id, 'unlock');

Write: my $token = ... (without parens) else wantarray in _create_token() will return an array which triggers extra code for nothing, as all you want is the token only.


>+    my $template = Bugzilla->template_inner(
>+        $user->settings->{'lang'}->{'value'});

Nit: this could be on a single line, as we do everywhere else in this file.


>+    $template->process("account/email/lockout.txt.tmpl", $vars, \$message)
>+        || ThrowTemplateError($template->error());

You have to add Bugzilla->template_inner("") right after it, else all subsequent templates will be displayed in the other user's language instead of yours.



>Index: Bugzilla/User.pm

>+sub account_is_locked_out {

This method and all other ones have no POD.


>+sub unlock_account {

>+        "DELETE FROM login_failure 
>+          WHERE user_id = ? AND login_time > LOCALTIMESTAMP(0) - $time",

Why not deleting all entries for this user? As you are deleting some entries here, and if your goal if to have some history of failures, then this goal broken already as some data is deleted when unlocking your account.



>Index: Bugzilla/Util.pm

format_time() executes a regexp on $date, but if $date is an object, it shouldn't try to execute the regexp.



>Index: template/en/default/account/email/lockout.txt.tmpl

>+[%+ FOREACH login = to_user.recent_login_failures %]
>+  [% login.ip_addr %] at [% login.login_time FILTER time %][% "\n" %]
>+[% END %]

Why writing [% "\n" %]? I think writing [%+ login.ip_addr %] would do it in a cleaner way.



>Index: template/en/default/global/user-error.html.tmpl

>+  [% ELSIF error == "account_locked" %]

Please move this error in alphabetical order, i.e. right after "account_exists".


>+  [% ELSIF error == "wrong_token_for_unlocking_account" %]

Move this error right after "wrong_token_for_creating_account".


I didn't apply and test your patch yet, but it looks good. I will test your updated patch.
(In reply to comment #30)
> >Index: Bugzilla/User.pm
> >+sub account_is_locked_out {
> 
> This method and all other ones have no POD.

  I know. I honestly didn't think they were important enough to document.

> Why not deleting all entries for this user? As you are deleting some entries
> here, and if your goal if to have some history of failures, then this goal
> broken already as some data is deleted when unlocking your account.

  I'd like to generally keep most of the data. This only removes a small bit of the data, in an unusual circumstance. I'm not going to implement some whole auditing system for Bugzilla in this patch, but this at least gives us a bit of it.

> Why writing [% "\n" %]? I think writing [%+ login.ip_addr %] would do it in a
> cleaner way.

  Because the \n way works and the + way doesn't work--the + way creates too much whitespace, or not enough whitespace--I don't remember.
Attached patch v5 (obsolete) (deleted) — — Splinter Review 
Okay, this fixes everything that I didn't respond to.
Attachment #373560 - Attachment is obsolete: true
Attachment #374978 - Flags: review?(LpSolit)
Attached patch v6 - Lockout based on IP+Account (obsolete) (deleted) — — Splinter Review 
Okay, so we had a discussion about this patch on IRC. We had various proposals, and here are the results:

1) Lock out by just account.
  Problem: It's too easy for people to DoS your account by doing intentional bad logins.

2) Lock out by just IP
  Problem: In corporate situations, there are often lots of people accessing a remote Bugzilla from one IP. If, collectively, enough people fail to log in in a given time period, everybody gets locked out.

So we went with:

3) Lockout by IP+Account

This isn't as secure as lockout by just IP, but it doesn't have the dangers that pure-IP lockout does, and it doesn't allow some remote user to DoS your account like pure-account lockout does.

The only problem with this is that it allows (a) a botnet to attack a given account (b) a single IP to continuously try different accounts (though only a small number of passwords at a time for each account). However, attackers can *currently* do that, without this patch, in an even worse way, so this patch is still an overall security win.
Attachment #374978 - Attachment is obsolete: true
Attachment #375116 - Flags: review?(LpSolit)
Attachment #374978 - Flags: review?(LpSolit)
Oh, also, the patch notifies the maintainer every time there's a lockout, so if there are consistent lockouts from a given IP, the maintainer can theoretically ban that IP.
This is just getting too invasive to take for 3.4.
Flags: blocking3.4+ → blocking3.4-
Target Milestone: Bugzilla 3.4 → Bugzilla 3.6
Honestly, I'm starting to wonder if "the user account can be DoS'ed" is even something we should be worrying about. Are we just solving problems that we don't know exist yet? I find it much more convenient to be able to immediately unlock my account. Also, login failures don't continue to rack up after a lockout, so it's not like they can endlessly extend the lockout period. They can only start doing a lockout again after I unlock the account.
FWIW, probably a better solution to all of this is to require a CAPTCHA after too many login failures from either a particular IP or on an individual account. That's a bit more complex to implement, though.
(In reply to comment #35)
> This is just getting too invasive to take for 3.4.

Uh, I want this for 3.4. See comments 19-26.
(In reply to comment #38)
> (In reply to comment #35)
> > This is just getting too invasive to take for 3.4.
> 
> Uh, I want this for 3.4. See comments 19-26.

3.4 is already frozen, and this is still an enhancement request that didn't yet have a patch when we froze.  We can backport it to 3.4 for b.m.o (which should basically mean "apply the patch" since it hasn't diverged that far yet) and share the patch if anyone else wants to apply it themselves.
Max, before I start reviewing this patch, is this still what we want? I think it is.
(In reply to comment #40)
> Max, before I start reviewing this patch, is this still what we want? I think
> it is.

  Yeah, I think it is. It's at least better than nothing.
Comment on attachment 375116 [details] [diff] [review]
v6 - Lockout based on IP+Account

bitrot in Util::format_time() due to bug 491630. Should be easy to fix.
Attachment #375116 - Flags: review?(LpSolit) → review-
Attached patch v7 (obsolete) (deleted) — — Splinter Review 
Okay, fixed the bitrot. Tested it, and it still works.
Attachment #375116 - Attachment is obsolete: true
Attachment #395130 - Flags: review?(LpSolit)
Comment on attachment 395130 [details] [diff] [review]
v7

>Index: Bugzilla/Auth.pm

>+        my $dt = datetime_from($determiner->{login_time}, $user->timezone);

Why the timezone of the user? Both the maintainer and the user will get the same timezone, despite the maintainer is maybe in a different timezone. Also, it would make total sense to check whether the maintainer is a Bugzilla user or not, so that you can use his timezone.


>+                locked_user => $user,

>+The hashref will also contain a C<user> element

Nit: Shouldn't it be a C<locked_user> element?



>Index: Bugzilla/User.pm

>+sub note_login_failure {
>+    my $ip_addr = Bugzilla->cgi->remote_addr || NO_IP;

I would prefer that you pass Bugzilla->cgi->remote_addr to the method, so that this method doesn't depend on CGI. Also, you should clear old unsuccessful attempts when the user enters the correct password from this IP, IMO. Data will grow forever in this table, and I don't really see a reason for that.


>+sub account_ip_login_failures {
>+    my $ip_addr = Bugzilla->cgi->remote_addr || NO_IP;

Same here.



>Index: Bugzilla/Config/Core.pm

>+   default => 'THE MAINTAINER HAS NOT YET BEEN SET',
>+   checker => \&check_email,

This fails when you reset the parameter.



>Index: template/en/default/global/user-error.html.tmpl

>+    ([% constants.LOGIN_LOCKOUT_INTERVAL %] minutes from now), as you have

This is only true when you lock out your account. Any new attempt won't update the unlock time, and so "30 minutes from now" is no longer true. Just remove it. This way you also fix the test below, see 008filter.t:

#   Failed test '(en/default) template/en/default/global/user-error.html.tmpl has unfiltered directives:
#   83: constants.LOGIN_LOCKOUT_INTERVAL
# --ERROR'
#   at t/008filter.t line 132.

Nit: Also, do we want to warn the user in the "wrong password" error message that the next time he enters the wrong password, his account will be locked for 30 minutes, when he is closed to the max # of attempts?
Attachment #395130 - Flags: review?(LpSolit) → review-
(In reply to comment #44)
> >+                locked_user => $user,
> 
> >+The hashref will also contain a C<user> element
> 
> Nit: Shouldn't it be a C<locked_user> element?

  No, that's what we pass to the template, not what the authorizer returns.

> I would prefer that you pass Bugzilla->cgi->remote_addr to the method, so that
> this method doesn't depend on CGI. Also, you should clear old unsuccessful
> attempts when the user enters the correct password from this IP, IMO. Data will
> grow forever in this table, and I don't really see a reason for that.

  The only valid way in any web application running under Perl to get the IP address of the end user is $cgi->remote_addr, which can be overridden by local installations that use proxies. We use remote_addr everywhere, so there's no reason to pass it to the function as an argument. If in the future there's some need, we can refactor it pretty easily.


  Everything else is getting fixed.
>   The only valid way in any web application running under Perl to get the IP
> address of the end user is $cgi->remote_addr,

  To be more exact, the valid way is $ENV{'REMOTE_ADDR'} and using $cgi->remote_addr allows admins to override it for proxy situations (something we may even be providing a parameter for in the future).
Attached patch v8 (obsolete) (deleted) — — Splinter Review 
Here, this takes all your suggestions, including warning the user when he's about to be locked out.
Attachment #395130 - Attachment is obsolete: true
Attachment #409005 - Flags: review?(LpSolit)
Hey Max,

I had a look at your patch, and tested it , it is looking good, only a little error I found in Bugzilla/User.pm as the following:

+sub clear_login_failures {
+    my $self = shift;
+    my $ip_addr = Bugzilla->cgi_remote_addr || NO_IP;

should be Bugzilla->cgi->remote_addr

Thanks,
Noura
Attached patch v8.1 (deleted) — — Splinter Review 
Thanks for catching that, Noura! :-)
Attachment #409005 - Attachment is obsolete: true
Attachment #410432 - Flags: review?(LpSolit)
Attachment #409005 - Flags: review?(LpSolit)
Blocks: 472217
Comment on attachment 410432 [details] [diff] [review]
v8.1

The Util.pm part of this patch has been checked in as part of the Bugzilla::Comment patch. So the Util.pm part won't apply, but that's not a problem, because it's already been applied.
Hi Max, 

Do you also need to change the code in Bugzilla/User.pm which is:
Bugzilla->cgi->remote_addr to be remote_ip() ? for the changes done to Util.pm as you mentioned in your previous comment.

Thanks,
Noura
(In reply to comment #51)
> Do you also need to change the code in Bugzilla/User.pm which is:
> Bugzilla->cgi->remote_addr to be remote_ip() ? for the changes done to Util.pm
> as you mentioned in your previous comment.

  No, because the patch that implements remote_ip (bug 527586) has not been checked in yet, because it is waiting on blockers.
No longer blocks: 472217
Comment on attachment 410432 [details] [diff] [review]
v8.1

>Index: Bugzilla/Auth.pm

>+        my $unlock_at = $dt->add(minutes => LOGIN_LOCKOUT_INTERVAL);

Nit: $unlock_at is just a reference to $dt itself, so is pretty useless (you could rename the $dt variable as $unlock_at, if you want a sensible variable name).


>+            # We're sending to the maintainer, who isn't a Bugzilla account,

Nit: who knows? Maybe the maintainer *is* a regular user. But I guess the logic is fine here, only the comment is not accurate.


>+        ThrowUserError('account_locked', 
>+            { ip_addr => $attempts->[0]->{ip_addr}, unlock_at => $unlock_at });

Nit: you could eventually write $determiner instead of $attemps->[0].


>+=head2 C<AUTH_LOCKOUT>
>+
>+The user's account is locked out after having failed to log in too many
>+times with a certain period of time (as specified by
>+L<Bugzilla::Constants/LOGIN_LOCKOUT_INTERVAL>).

s/with/within/ ?



>Index: Bugzilla/User.pm

>+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);
>+}

Shouldn't $self->{account_ip_login_failures} be nuked as well? Also, this method has no POD.


>+sub account_ip_login_failures {

>+    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(

Nit: wouldn't it be better to set $time and $ip_addr only when $self->{account_ip_login_failures} is undefined as you don't need them otherwise?


>+=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>.

I don't understand the last sentence at all. How could login attempts have anything to do with updating a user object? Nuke this sentence completely.



>Index: template/en/default/email/lockout.txt.tmpl

>+The IP address [% attempts.0.ip_addr %] failed too many login attempts (
>+[%- constants.MAX_LOGIN_ATTEMPTS +%]) for

The admin probably knows how many attempts are needed before an account is locked. No need to specify it here.



>Index: template/en/default/global/user-error.html.tmpl

>+    [%# People get two login attempts before being warned about
>+      # being locked out.
>+      #%]
>+    [% IF remaining <= (constants.MAX_LOGIN_ATTEMPTS - 2) %]

I would rather warn the user when there are only 2 attempts left. Setting MAX_LOGIN_ATTEMPTS to 10, you would get warnings about you having only 8 attempts left. Not only this is a bit too far from being critical, but this also gives attackers a good hint about this account being valid (despite the idea is to display the same error message for invalid accounts and for invalid passwords for this exact reason). So I would much prefer
 [% IF remaining <= 2 %].


r=LpSolit, with as many comments addressed as possible.
Attachment #410432 - Flags: review?(LpSolit) → review+
Note that old failures can stay in the DB forever if the user never attempts to log in from some IPs in the future. Shouldn't they be removed after a (few) month or so?
Flags: approval+
Keywords: relnote
Okay, I fixed most of the comments on checkin.

Checking in Bugzilla/Auth.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth.pm,v  <--  Auth.pm
new revision: 1.22; previous revision: 1.21
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.123; previous revision: 1.122
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.200; previous revision: 1.199
done
Checking in Bugzilla/Auth/Verify/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/DB.pm,v  <--  DB.pm
new revision: 1.11; previous revision: 1.10
done
Checking in Bugzilla/Config/Common.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Common.pm,v  <--  Common.pm
new revision: 1.30; previous revision: 1.29
done
Checking in Bugzilla/Config/Core.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Core.pm,v  <--  Core.pm
new revision: 1.12; previous revision: 1.11
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.126; previous revision: 1.125
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/email/lockout.txt.tmpl,v
done
Checking in template/en/default/email/lockout.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/email/lockout.txt.tmpl,v  <--  lockout.txt.tmpl
initial revision: 1.1
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.292; previous revision: 1.291
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Added to the release notes in bug 547466.
Keywords: relnote
Blocks: 707594
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: