Bug 509100 - Implement lock out policy to protect against brute force password attacks
Implement lock out policy to protect against brute force password attacks
Status: CLOSED NEXTRELEASE
Product: Bugzilla
Classification: Community
Component: User Accounts (Show other bugs)
3.4
All All
medium Severity medium (vote)
: ---
: ---
Assigned To: Noura El hawary
: FutureFeature
Depends On: 537784
Blocks: RHBZ34UpgradeTracker
  Show dependency treegraph
 
Reported: 2009-07-01 07:13 EDT by Max Kanat-Alexander
Modified: 2009-11-21 07:29 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Enhancement
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-11-18 06:47:40 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
v1 for applying upstream brute password patch to 3.4 code branch (43.07 KB, patch)
2009-11-05 08:32 EST, Noura El hawary
dkl: review-
Details | Diff
v2 for applying upstream brute password patch to our 3.4 branch (49.98 KB, patch)
2009-11-16 08:36 EST, Noura El hawary
dkl: review-
Details | Diff
v3 brute password patch (49.98 KB, patch)
2009-11-17 06:33 EST, Noura El hawary
dkl: review-
Details | Diff
v4 of brute password patch (49.73 KB, patch)
2009-11-17 06:40 EST, Noura El hawary
dkl: review+
Details | Diff


External Trackers
Tracker ID Priority Status Summary Last Updated
Mozilla Foundation 355283 None None None Never

  None (edit)
Description Max Kanat-Alexander 2009-07-01 07:13:26 EDT
I log in to the Red Hat Bugzilla very rarely. So rarely, in fact, that EVERY TIME I log in, I have to reset my password. I suspect that this is the case for the majority of users in the community. Last time I had to do this, it almost just made me not want to file bugs or respond to questions anymore. Perhaps expiration should be limited to only @redhat.com accounts, or only people in significant security groups?

I'd also like to point out (as I pointed out when this was being implemented) that password expiration isn't adding any security. (See bug 459513 comment 2 and bug 459513 comment 9.)
Comment 1 David Lawrence 2009-07-30 15:41:10 EDT
(In reply to comment #0)
> I log in to the Red Hat Bugzilla very rarely. So rarely, in fact, that EVERY
> TIME I log in, I have to reset my password. I suspect that this is the case for
> the majority of users in the community. Last time I had to do this, it almost
> just made me not want to file bugs or respond to questions anymore. Perhaps
> expiration should be limited to only @redhat.com accounts, or only people in
> significant security groups?
> 
> I'd also like to point out (as I pointed out when this was being implemented)
> that password expiration isn't adding any security. (See bug 459513 comment 2
> and bug 459513 comment 9.)  

Max I understand your comments and you do make sense in what you say but the change was passed down from management in crisis mode and my hands were tied. They wanted it quick and wanted it to span all Bugzilla accounts. The new expiration policy is in place and had to be consistent for all Red Hat accounts internally and externally which meant Bugzilla as well. We have expanded the expiration time several times since then so it should not expire near as much as it did.

That being said, when we move over to 3.4 in the near future, I would like to integrate your locking code as well into our code base as I definitely feel that adds a better level of security than what we have now. So I am changing this bug to reflect the new feature for 3.4.

Should I use the patch already attached to bug 355284 at bugzilla.mozilla.org or is there something better in your repository that is ready to go for 3.4?

Thanks
Dave
Comment 2 Max Kanat-Alexander 2009-07-31 00:31:02 EDT
Okay, I understand that.

Yeah, once bug 366283 passes review, you should be able to backport it to 3.4, so I'd recommend using it then. It has the most current code available.
Comment 3 Noura El hawary 2009-10-28 07:38:49 EDT
The only bug I found in upstream bugzilla for the brute password issue was:
https://bugzilla.mozilla.org/show_bug.cgi?id=355283


bug #355284 , and bug #366283 mentioned in the previous 2 comments of this bug both opened non related bugs to the brute password problems.

So which one is the right upstream bug for this?

Thanks,
Noura
Comment 4 David Lawrence 2009-10-28 16:14:37 EDT
Noura upstream bug 355283 patch v7 is the correct patch although LpSolit made some suggested changes which maybe we should do as well. Once you are done we can submit your revised patch upstream as well. Bug 366283 was a typo error.

Dave
Comment 5 Max Kanat-Alexander 2009-10-28 20:40:52 EDT
I'll update the upstream patch.
Comment 6 Noura El hawary 2009-10-29 09:30:39 EDT
Thanks Dave & Max, I applied the patch to our 3.4 code and currently testing it

Noura
Comment 7 Noura El hawary 2009-11-05 08:32:01 EST
Created attachment 367609 [details]
v1 for applying upstream brute password patch to 3.4 code branch

Hey Dave,

I have removed our previous patches that expires bugzilla passwords as in the bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=486197
https://bugzilla.redhat.com/show_bug.cgi?id=459513

and applied the upstream patch to our code, tested it and it looks good. it is now applied to bz-web1 for your testing. Please take a look and let me know what you think.

Thanks,
Noura
Comment 8 David Lawrence 2009-11-09 17:32:50 EST
Comment on attachment 367609 [details]
v1 for applying upstream brute password patch to 3.4 code branch

Couple things:

When I was testing on bz-web1, I was entering bad passwords for my account one after anohter. After the 2nd attempt I got the warning properly. When I hit the 5th bad password, instead of getting another error screen, I got a blank screen instead. Then the next time I tried to login, I got the normal screen telling me my account is locked. Do you get the blank screen as well?

We do not yet have Bugzilla->cgi->remote_addr in our code so that may be the issue. 

Kevin and Schick are discussing with IS about whether we can remove the password expiration code or not. Schick thinks it is good to at least have a way to mass expire passwords if the need arises. So we may need to still make this work together. Stay tuned.

Dave
Comment 9 Noura El hawary 2009-11-10 07:34:06 EST
Hey Dave,

Yeah i get the blank screen as well when it is trying to retrieve the ip address to block it, so I guess as you mentioned it is because we don't have the Bugzilla->cgi->remote_addr in our code yet. I tried to look into upstream bugs for the code that has it and couldn't find it, is there a bug for it?

also as we remove the old code that changes the password from bugzilla we will need to remove the columns:

profiles.password_expire_days
profiles.password_last_changed

so shall we add $dbh->bz_drop_column($table, $column); for those columns to be executed when we run checksetup.pl for 3.4 upgrade?

Thanks,
Noura
Comment 10 Noura El hawary 2009-11-10 07:37:02 EST
also one more note , maybe we will need to apply the patch in the bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=527586

with the brute password patch

Thanks,
Noura
Comment 11 Max Kanat-Alexander 2009-11-10 11:46:46 EST
remote_addr is part of CGI.pm.
Comment 12 David Lawrence 2009-11-11 17:50:34 EST
(In reply to comment #10)
> also one more note , maybe we will need to apply the patch in the bug:
> https://bugzilla.mozilla.org/show_bug.cgi?id=527586
> 
> with the brute password patch
> 
> Thanks,
> Noura  

Yes please apply the above patch to our code. We currently have a hack in Bugzilla.pm in init_page() that does similar that will need to be removed as well since we will now use the upstream method.

But file that as a separate bug report and make this bug depend on it.

Dave
Comment 13 David Lawrence 2009-11-11 17:58:18 EST
(In reply to comment #9)
> Hey Dave,
> 
> Yeah i get the blank screen as well when it is trying to retrieve the ip
> address to block it, so I guess as you mentioned it is because we don't have
> the Bugzilla->cgi->remote_addr in our code yet. I tried to look into upstream
> bugs for the code that has it and couldn't find it, is there a bug for it?
> 

Using the patch from comment # 12 you would just use remote_ip() in Bugzilla::Util instead.

> also as we remove the old code that changes the password from bugzilla we will
> need to remove the columns:
> 
> profiles.password_expire_days
> profiles.password_last_changed
> 
> so shall we add $dbh->bz_drop_column($table, $column); for those columns to be
> executed when we run checksetup.pl for 3.4 upgrade?
> 

Yes it would go in Bugzilla/Install/DB.pm since we didn't do this as a plugin originally. If it had have been does as a plugin then it would have gone in extensions/redhat_fields/code/install-update_db.pl

> Thanks,
> Noura
Comment 14 David Lawrence 2009-11-11 17:59:24 EST
Please look into how much work would be needed to keep the password expiration in along with code but don't do it yet. I just want to give Kevin an idea on how much effort it would take.

Dave
Comment 15 Noura El hawary 2009-11-12 08:01:24 EST
Hey Dave,

I think it will take from 2 to 3 days of work to merge our password expiration code with upstream brute password patch.

Cheers,
Noura
Comment 16 David Lawrence 2009-11-12 12:20:06 EST
Still some other leftover code:

Can't locate object method "set_password_expire_days" via package "Bugzilla::User" at /var/www/html/bugzilla/editusers.cgi line 266.
Comment 17 Noura El hawary 2009-11-16 08:36:02 EST
Created attachment 369694 [details]
v2 for applying upstream brute password patch to our 3.4 branch

Hey Dave,

This is a modified version of the previous patch that removes our password expiry code and replaces it with brute password upstream patch, this patch includes the following modifications:

1- the blank screen problem that we used to get at the user lockout time has been fixed, the problem was from missing to add the new template template/en/default/email/lockout.txt.tmpl in the patch

2- added code to remove the profiles table columns related to password expiry

3- removed still existing code of password expiry in editusers.cgi and in some documentation text

4- This patch must be applied after applying the patch in bug #537784, as $cgi->remote_addr has been replaced with remote_ip()

I have tested the patch on my localhost and seems to be working perfectly, applied to bz-web1 at the moment but untested because bz-web1 is down.

Cheers,
Noura
Comment 18 David Lawrence 2009-11-16 15:34:37 EST
Comment on attachment 369694 [details]
v2 for applying upstream brute password patch to our 3.4 branch

You missed a section referencing password_last_changed in token.cgi lines 223-231.

Also I think we should make LOGIN_LOCKOUT_INTERVAL and MAX_LOGIN_ATTEMPTS configurable from editparams.cgi. Some people may want to have different shorter/longer time and more/less number of retries.

Max, do you think we should email a token the locked out user allowing them to click on a link to be able to unlock right away? This would help legitimate users get running again sooner if they somehow messed up their passwords too many times? Malicious users would not see the email and continue to be locked out.

Dave
Comment 19 Max Kanat-Alexander 2009-11-16 16:06:46 EST
(In reply to comment #18)
> Max, do you think we should email a token the locked out user allowing them to
> click on a link to be able to unlock right away? This would help legitimate
> users get running again sooner if they somehow messed up their passwords too
> many times? Malicious users would not see the email and continue to be locked
> out.

  Original versions of the patch actually implemented this, yeah. Originally we removed it because it was too easy to spam somebody's account with lockout notices, but now that we restrict per-IP, it could be a reasonable thing to do, but it might also be very confusing to the user who didn't lock himself out (that is, it wasn't his IP that did the lockout). What do you think?
Comment 20 Noura El hawary 2009-11-17 04:49:32 EST
(In reply to comment #19)
> (In reply to comment #18)
> > Max, do you think we should email a token the locked out user allowing them to
> > click on a link to be able to unlock right away? This would help legitimate
> > users get running again sooner if they somehow messed up their passwords too
> > many times? Malicious users would not see the email and continue to be locked
> > out.
> 
>   Original versions of the patch actually implemented this, yeah. Originally we
> removed it because it was too easy to spam somebody's account with lockout
> notices, but now that we restrict per-IP, it could be a reasonable thing to do,
> but it might also be very confusing to the user who didn't lock himself out
> (that is, it wasn't his IP that did the lockout). What do you think?  

I agree that it would be confusing to the user, as we already got few users sending us emails asking why they got notifications about people trying to change their password and that they don't understand what is happening, I say as we are taking care of it for them then it should be okay, besides we "the bugzilla maintainer" get emails about all lockouts.
Comment 21 Noura El hawary 2009-11-17 06:33:01 EST
Created attachment 369863 [details]
v3 brute password patch

Please ignore this patch Dave
Comment 22 Noura El hawary 2009-11-17 06:40:02 EST
Created attachment 369865 [details]
v4 of brute password patch

Hey Dave,

Thanks for your review, attached is a new patch with:

1- removed missing part in token.cgi related to password expiry code.
2- converted the constants LOGIN_LOCKOUT_INTERVAL, MAX_LOGIN_ATTEMPTS to parameters in editpatams.cgi , pointing to the constants as the default values.

Please review and let me know what you think.

Thanks,
Noura
Comment 23 David Lawrence 2009-11-17 15:22:38 EST
Comment on attachment 369863 [details]
v3 brute password patch

I am unable to get your patch to apply cleanly. Please create it from an updated svn tree if you can.

patching file Bugzilla/Token.pm
Hunk #1 FAILED at 126.
Hunk #3 FAILED at 283.
2 out of 4 hunks FAILED -- saving rejects to file Bugzilla/Token.pm.rej
patching file Bugzilla/Util.pm
Hunk #1 FAILED at 35.
Hunk #2 FAILED at 52.
Hunk #3 succeeded at 315 with fuzz 1 (offset 10 lines).
Hunk #5 succeeded at 471 (offset 10 lines).
Hunk #7 succeeded at 991 (offset 10 lines).
2 out of 7 hunks FAILED -- saving rejects to file Bugzilla/Util.pm.rej
Comment 24 David Lawrence 2009-11-17 15:24:58 EST
(In reply to comment #23)
> (From update of attachment 369863 [details])
> I am unable to get your patch to apply cleanly. Please create it from an
> updated svn tree if you can.
> 
> patching file Bugzilla/Token.pm
> Hunk #1 FAILED at 126.
> Hunk #3 FAILED at 283.
> 2 out of 4 hunks FAILED -- saving rejects to file Bugzilla/Token.pm.rej
> patching file Bugzilla/Util.pm
> Hunk #1 FAILED at 35.
> Hunk #2 FAILED at 52.
> Hunk #3 succeeded at 315 with fuzz 1 (offset 10 lines).
> Hunk #5 succeeded at 471 (offset 10 lines).
> Hunk #7 succeeded at 991 (offset 10 lines).
> 2 out of 7 hunks FAILED -- saving rejects to file Bugzilla/Util.pm.rej  

Nevermind. Would help if I applied the right patch :) My fault.
Comment 25 David Lawrence 2009-11-17 16:09:24 EST
Comment on attachment 369865 [details]
v4 of brute password patch

>Index: Bugzilla/Config/Auth.pm
>+   default => Bugzilla::Constants::LOGIN_LOCKOUT_INTERVAL,
>+  },
>+
>+  {
>+   name => 'maxloginattempts',
>+   type => 't',
>+   default => Bugzilla::Constants::MAX_LOGIN_ATTEMPTS,
>+  });

Nit: I prefer underscores in the names when possible.

login_lockout_interval
max_login_attempts


>Index: template/en/default/admin/params/auth.html.tmpl
>+  loginlockoutinterval => "This defines the time that the account with the maximum login " _
>+                          "attempts will be locked for.",

"This defines the time that the account with the maximum login " _
"attempts will be locked out for in minutes."

>+  maxloginattempts => "This defines the maximum failed logins attempts to lock account using " _
>+                      "its IP address.",}

"This defines the maximum number of failed login attempts before locking account from " _
"it's IP address."

Otherwise works well and looks fine. You can update the above before checkin.

Dave
Comment 26 Noura El hawary 2009-11-18 06:47:40 EST
Thanks for the review Dave, I made the changes you suggested and committed to svn.

Noura
Comment 27 David Lawrence 2009-11-18 10:45:28 EST
If you get some free time please work on a version of this patch that applies cleanly to the upstream Bugzilla code. And then attach the new updated patch to the attached upstream bug report asking for review. We would like to get this accepted upstream as well.

Thanks
Dave
Comment 28 Noura El hawary 2009-11-19 06:27:02 EST
sure Dave, will do that.
Comment 29 Max Kanat-Alexander 2009-11-19 17:07:26 EST
Note that currently, upstream, we don't want additional parameters for this--we're trying our best to cut down on parameters.
Comment 30 Noura El hawary 2009-11-21 07:29:39 EST
(In reply to comment #29)
> Note that currently, upstream, we don't want additional parameters for
> this--we're trying our best to cut down on parameters.  

In this case, I don't really need to submit our patch upstream then, caze only thing extra that has is the adding of the parameters, and removal of our password expiry which upstream doesn't have.

Note You need to log in before you can comment on or make changes to this bug.