this is a standard feature for most systems, e.g. windows nt [x] user must change password at next login until the user changes password, the user should not be allowed to take any normal database actions (view confidential bugs, change preferences, change bugs).

Comment on attachment 200074 [details] [diff] [review] erik's initial functional patch -- needs some boilerplate Ack!! I meant to attach this to bug 312945 - but it might be 90% of this bug's solution.

Actually, looking at our experience with bug 312945 and this bug, the way I would like to see this work is as follows.... 1) Add parameters password_age_limit (days), password_age_warn (days) and password_age_policy. 2) Permit the following policies.... a) No aging b) When the password has not been changed in limit-warn days, warn the user that the password must be changed. Permit the change to be done using the web interface. If the password actually expires, require a reset using a token. c) When the password has not been reset (using a token) in limit-warn days, warn the user that the password must be reset using a token. Do not restart the counter if the password is merely changed using the web interface. 3) Add a password_aging_since field to user profiles. 4) When a user logs in during the warning period or attempts to log in after the warnign period expires, provide a one-click request button for a password reset token.

I am fully against a password aging policy. Here are some of my comments from the Red Hat Bugzilla when they implemented one (which I found incredibly annoying): > We at Red Hat need to guarantee that the users > will set their password to a new unique value after a determined amount of > time. Yes, but why? This is a frequently-requested but rarely-examined idea. There's only two possible reasons: 1. To protect against somebody guessing their password given infinite time. In this case, a lockout policy works much better, since it throttles down guessing attempts to a rate such that they would take too long--possibly years or decades. 2. To prevent an attacker who secretly has the password of another user from continuing to log in. If an attacker already has a user's password, if that user is trusted, then largely everything dangerous that a user can do, they can do within a day--look at security bugs, open security bugs, or make dangerous administrative changes. Even if you changed your password every five minutes, if an attacker knew your password within those five minutes, he or she could do whatever dangerous activities he or she had intended, most likely. Also, if an attacker already has a user's password, you don't want to fix it silently, you just want to know that it's happened, and what he's done so that you can revert it. The solution to that is auditing. So in Bugzilla's case, there is no value and considerable inconvenience to requiring a user to change their password after a fixed period of time. In a system where brute force attacks are possible but take longer than the password-change period, password changes are a good idea. However, Bugzilla, with the lockout policy, is not such a system. Also, on most systems, people set their password-change period so long (say, three months) that almost any password could be guessed, brute-force, in that period of time.

(In reply to comment #5) I agree with point #1. Throttling down the possible attempts per day is a good way to prevent brute force. You have a good point for item #2. My response would be that this is a method of containment. I agree that when an attacker has someone's password they can do lots of damage within a 5 minute window. However, a password change would possibly stop the issue and prevent continued exposure of sensitive data. For example, the attacker may be able to view all sensitive bugs in bugzilla at this point in time, but a password change would at least stop the attacker from continually viewing this bugs for years. It would be nice to know that it is happening. Perhaps we can add some more auditing controls such as detecting concurrent logins or suspicious IP source addresses. But, I also think that password aging is a good mitigating control to increase our overall security of the system.

I don't think any company/person who wants this ever considered scenarios laid out by mkanat in comment 5. And yet, companies have requirement for software they use to have password ageing and history policies features. We have been using a patch done by mkanat to NASA for password ageing and I put something together for history. Anyway, without any further do here is the patch

Comment on attachment 497588 [details] [diff] [review] Password ageing and history >=== modified file 'Bugzilla.pm' >- my ($class, $type) = @_; >+ my ($class, $type, $logout) = @_; $logout is an unused variable, so it doesn't make sense to pass it to login(). >=== modified file 'Bugzilla/Config/Auth.pm' >+ name => 'passwordresetperiod', >+ name => 'passwordhistorylength', Use underscores to separate words, per bug 155628. >+ name => 'password_reset_warn', Is this parameter really useful? Doesn't it make sense to *always* warn people that their account will be deactivated? >=== modified file 'Bugzilla/DB/Schema.pm' >+ password_history => { >+ pass_id => {TYPE => 'INT3', NOTNULL => 1}, How is this useful? If the goal is to uniquely identify the password, then it should be an auto-increment column. Else it can simply go away. My preference is to make it go away. >+ password => {TYPE => 'varchar(128)'}, The password cannot be NULL. >+ changed => {TYPE => 'DATETIME', NOTNULL => 1}, How is this useful? This information is not needed, right? Only the change time of the current password is, which is already stored in the profiles table. >=== modified file 'Bugzilla/Install/DB.pm' >+ _add_profiles_password_changed(); >+ > # 2009-03-31 LpSolit@gmail.com - Bug 478972 Please add DB changes chronologically. March 2009 is not the right place (even if it was when the patch was written). >+sub _add_profiles_password_changed { >+ # PRACA 4.0 Bug 311 - rmaia@everythingsolved.com Must go away. This information is useless to us. >+ if (!$dbh->bz_column_info('profiles', 'password_changed')) { >+ $dbh->bz_add_column('profiles', 'password_changed', >+ {TYPE => 'DATETIME'}); >+ $dbh->do("UPDATE profiles SET password_changed = NOW()"); >+ $dbh->bz_alter_column('profiles', 'password_changed', >+ {TYPE => 'DATETIME', NOTNULL => 1}); >+ } AFAIK, this is doable in a single line, by calling bz_add_column() directly with the right default. >=== modified file 'Bugzilla/User.pm' >+sub needs_password_change { >+ my $oneday = 60 * 60 * 24; >+ my $elapsed_days = (time() - str2time($self->password_changed))/$oneday; No need for $oneday. Everybody knows that a day has 86400 seconds. :) >+ if ($elapsed_days >= Bugzilla->params->{'passwordresetperiod'}) { >+ return 1; >+ } >+ return 0; Nit: $elapsed_days >= Bugzilla->params->{'passwordresetperiod'} ? 1 : 0; >=== modified file 'collectstats.pl' >--- collectstats.pl 2010-11-02 23:06:15 +0000 >+++ collectstats.pl 2010-12-14 17:26:23 +0000 >@@ -35,6 +35,8 @@ > > use List::Util qw(first); > use Cwd; >+use Date::Format; >+use Date::Parse; > > use Bugzilla; > use Bugzilla::Constants; >@@ -44,6 +46,7 @@ > use Bugzilla::User; > use Bugzilla::Product; > use Bugzilla::Field; >+use Bugzilla::Mailer; > > # Turn off output buffering (probably needed when displaying output feedback > # in the regenerate mode). >@@ -159,8 +162,67 @@ > # Uncomment the following line for performance testing. > #print "Total time taken " . delta_time($tstart, $tend) . "\n"; > >+send_password_notification_email(); > CollectSeriesData(); Please call send_password_notification_email() *after* CollectSeriesData(), in case there is something wrong with the MTA. It's more critical to not loose data than to delay notifications a bit. >+sub send_password_notification_email { >+ my $query = "SELECT userid, login_name, hidden_email, password_changed >+ FROM profiles >+ WHERE disabledtext = ''"; Several things: - What is hidden_email?? - Do not collect *all* active accounts. Only those which are close to the deadline. - Also, you shouldn't warn people with disable_mail = 1. >+ my $oneday = 60 * 60 * 24; Same remark as above. We all know about 86400. >+ my $elapsed_days = int ((time() - str2time($changed))/$oneday); >+ my $days_to_expire = $period - $elapsed_days; >+ >+ next if not ($days_to_expire < $warn_days); This should be part of the SQL query, else you can collect tens of thousands of user accounts which are not even affected by the deadline. >+ my $cryptedpassword = bz_crypt("i2l5a4k7e6t9s8u"); Simply set the encrypted password to '*', as we usually do. >+ $template->process('email/password-reset-notification.txt.tmpl', >+ { to => $recipient,}, Nit: useless trailing comma in the hash. >+ $template->process('email/password-change-notification.txt.tmpl', >+ { to => $recipient, >+ days => $days_to_expire,}, Nit: same here. Also, as I see it, you are going to get notifications *every day* till the deadline is reach. This means 7 notifications in a single week! This is a bit too much (imagine you are afk for a few days...). >=== modified file 'relogin.cgi' >- Bugzilla->login(LOGIN_OPTIONAL); >+ Bugzilla->login(LOGIN_OPTIONAL, 1); 2nd argument not used by login(). >=== modified file 'template/en/default/account/prefs/account.html.tmpl' >+ [% IF force_change_password %] >+ <input type="hidden" name="forced_change_password" value="1"> >+ [% END %] I don't get the logic here. Why passing this information via the template? userprefs.cgi already knows this information, if needed. >+ [% IF !force_change_password && user.authorizer.can_change_email && Param('allowemailchange')%] > <tr> > <th align="right">Your real name (optional, but encouraged):</th> > <td> > <input size="35" name="realname" value="[% realname FILTER html %]"> > </td> > </tr> >+ [% ELSE %] >+ <input type="hidden" name="realname" value="[% realname FILTER html %]"> >+ [% END %] This doesn't make sense to me. Let me change my real name if I want to. >=== modified file 'template/en/default/admin/params/auth.html.tmpl' >+ passwordhistorylength => "A number of passwords. Users will be required to not use" _ >+ " this number of old passwords. This" _ >+ " feature is disabled if the value is set to 0." } I think the last sentence should be reworded. It should simply say that Bugzilla won't check if the password has already been used in the past. >=== added file 'template/en/default/email/password-change-notification.txt.tmpl' >+Until then you can still access [% terms.Bugzilla %], but once your password has >+expired, your password will be reset. As I understand it, your account will be locked. "reset" makes me think I will get an email with a new password in it. >=== modified file 'template/en/default/global/tabs.html.tmpl' >+ [% IF !force_change_password %] This template is for several templates. You shouldn't hack it with specific code. Please leave this template alone. >=== modified file 'template/en/default/global/user-error.html.tmpl' >+ [% ELSIF error == "passwords_match_old" %] >+ [% title = "Passwords Matches Old Password" %] >+ The new password matches one of old passwords. You are not allowed to use >+ last [% passwordhistorylength FILTER html %] passwords used. I'm not sure the wording is correct (but english is not my mother-language). >+ [% ELSIF error == "password_expired" %] >+ [% title = "Password Expired" %] >+ Your password has expired and must be changed. You must log in >+ to Bugzilla's web interface to do this. (I would have to check when and where this message exactly appear.) >+ [% ELSIF error == "password_must_change" %] >+ [% title = "Password Must Change" %] >+ Your new password cannot be the same as the old one. How is this message different from passwords_match_old? >=== modified file 'token.cgi' >+ my $password_list = Bugzilla->dbh->selectall_arrayref( >+ "SELECT pass_id, password, changed FROM password_history ". >+ "WHERE userid=" . $userid . " ORDER BY pass_id ASC"); >+ >+ if ( $passwordhistorylength > 0 ) { >+ # Shift elements from front of the array (remove from oldest) >+ # to make it the same size with the password history length >+ while ( @$password_list >= $passwordhistorylength ) { >+ shift(@$password_list); >+ } Use LIMIT in the SQL query instead. >+ { 'passwordhistorylength' => $passwordhistorylength }); Use underscores, please. Thisiseasiertoread! :) >+ if ( $passwordhistorylength > 0 ) { Useless "> 0". Also, there is a lot of duplicated code between token.cgi and userprefs.cgi. Please refactor it, and use Token.pm or User.pm if needed.

pass_id is needed to get the passwords in order they were used. There is a problem with password_list array between functions set_password_history and password_history

Comment on attachment 498530 [details] [diff] [review] Password ageing and history v2.1 >=== modified file 'Bugzilla/Constants.pm' >+# Number of days before sending warning for password changing >+use constant PASSWORD_RESET_WARN_DAYS => 7; I think it should be 14 days. One week may be too short. >=== modified file 'Bugzilla/Install/DB.pm' >+ $dbh->bz_add_column('profiles', 'password_changed', >+ { TYPE => 'DATETIME', NOTNULL => 1},$dbh->selectrow_array('SELECT NOW()')); Store $dbh->selectrow_array('SELECT NOW()') in a variable, else this line is too long. Also, use LOCALTIMESTAMP(0) instead of NOW(). >+ $dbh->bz_add_column('profiles', 'password_changed', >+ { TYPE => 'DATETIME', NOTNULL => 1}, time()); It doesn't make sense to have two calls to bz_add_column(). Once the column is added, subsequent calls have no effect. >=== modified file 'Bugzilla/User.pm' >+sub set_password { >+ $self->{password_changed} = Bugzilla->dbh->selectrow_array('SELECT NOW()'); Same remark as above: use LOCALTIMESTAMP(0) instead of NOW(). >+sub set_password_history { >+ my ($self, $userid, $password_list, $cryptedpassword) = @_; You don't need to pass $userid. You can already get it with $self->id ($self is a User object here). Also, I think this subroutine should be called by $user->update() if the password changed. This subroutine doesn't need any argument. All it needs is the new password, which can already be provided by the user object. And the password list can get obtained by querying the DB directly. Also, it would make more sense to set pass_id as an auto-increment field, and don't worry about renumbering it. This would make things much easier and remove a lot of complexity. >+sub password_history { It should rather be named check_password_history. Also, you don't need to return the password list if you use my proposal above. >+ my $password_list = Bugzilla->dbh->selectall_arrayref( >+ "SELECT pass_id, password FROM password_history ". >+ "WHERE userid=" . $userid . Use placeholders instead of injecting the user ID directly into the SQL query. >+sub needs_password_change { >+ return 0 if not $self->id; >+ return 0 if not Bugzilla->params->{'password_reset_period'}; Nit: return 0 unless ($self->id && Bugzilla->params->{'password_reset_period'}). >+ return $elapsed_days >= Bugzilla->params->{'passwordresetperiod'} ? 1 : 0; Missing underscores. >=== modified file 'collectstats.pl' >+sub send_password_notification_email { >+ $warn_days = int ($reset_period / 2); Nit: no whitespace after nit. >+ my $query = "SELECT userid, login_name >+ FROM profiles >+ WHERE disabledtext = '' You forgot to exclude users with disable_mail = 1. >+ AND password_changed < DATE_SUB( CURDATE( ), INTERVAL ? DAY )"; This is not cross-DB compatible. See how dates are handled in the code. Also, your code will also catch user accounts which have already been disabled. >+ my ($userid, $recipient) = @$row; You don't care about user IDs, so don't collect them. Also, your code won't work if emailsuffix is not empty. Also, don't send emails every day, as I said in my previous review. Twice per week is enough. >+ my $cryptedpassword = bz_crypt('*'); Do not encrypt '*'. >+ Bugzilla->logout_user_by_id($userid); >+ MessageToMTA($message); Do not notify users whose account has already been locked. We don't want to spam them. >=== modified file 'template/en/default/admin/params/auth.html.tmpl' >+ password_reset_period => "A number of days. Users will be required to change" _ >+ " their passwords every time this number of days" _ >+ " elapses since their last password change. This" _ >+ " feature is disabled if the value is set to 0.", Remove "A number of days.". >+ password_history_length => "A number of passwords. $terms.Bugzilla will not allow" _ >+ " passwords used in the past. This" _ >+ " feature is disabled if the value is set to 0." } "The number of consecutive passwords which cannot use twice the same password" seems better to me. >=== added file 'template/en/default/email/password-change-notification.txt.tmpl' >+[% PROCESS "global/field-descs.none.tmpl" %] You don't need this template. Simply call variables.none.tmpl. >+To change your password now, go to: [%+ Param('urlbase') %]userprefs.cgi?tab=account Call [% urlbase %] instead of [% Param('urlbase') %]. > >=== added file 'template/en/default/email/password-reset-notification.txt.tmpl' >+[% PROCESS "global/field-descs.none.tmpl" %] Same comment here. >+To request a new password click "Forgot Password" link on [% terms.Bugzilla %] homepage >+or go to: [%+ Param('urlbase') %]?GoAheadAndLogIn=1#forgot I think we can forge the correct link directly. >=== modified file 'template/en/default/global/messages.html.tmpl' >+ [% ELSIF message_tag == "password_change_forced" %] Unused message. >=== modified file 'template/en/default/global/user-error.html.tmpl' >+ [% ELSIF error == "passwords_match_old" %] >+ [% title = "Passwords Matches Old Password" %] >+ The new password matches one of your previous passwords. You are not >+ allowed to use last [% password_history_length FILTER html %] passwords used. "... It cannot be the same as one of your last [% ... %] passwords". >+ [% ELSIF error == "password_expired" %] >+ [% title = "Password Expired" %] >+ Your password has expired and must be changed. You must log in >+ to Bugzilla's web interface to do this. How can you log in if your password expired? >+ [% ELSIF error == "password_matches_current" %] >+ [% title = "Password Must Change" %] >+ Your new password cannot be the same as current password. This error must go away, see below. >=== modified file 'token.cgi' >+ # Commit users old passwords including new one back-to DB >+ Bugzilla->user->set_password_history($userid,$password_history,$cryptedpassword); To avoid code duplication, you should refactor this code a bit and use a user object. Then $user->set_password() will do the job for you, including making sure that the password has not already been used in the past. >=== modified file 'userprefs.cgi' >+ # Check if password matches users old passwords and fetch password history >+ my $password_history = $user->password_history($user->id, $pwd1); This check should be part of $user->set_password(). > $dbh->do(q{UPDATE profiles >- SET cryptpassword = ? >+ SET cryptpassword = ?, >+ password_changed = NOW() > WHERE userid = ?}, > undef, ($cryptedpassword, $user->id)); You will have to refactor this code a bit to use a use object, as in token.cgi. Else you are duplicating your code again and again. >+ else { >+ ThrowUserError("password_matches_current"); No reason to throw an error if the user entered the same password as the current one. This simply means the user isn't updating his password, and nothing should be done here. I still haven't tested your patch (I hope you did), so more may come when reviewing future patches. But things are looking better. :)

Updated the patch but where comment says "Also, your code will also catch user accounts which have already been disabled." The code already handles disabled accounts by "WHERE disabledtext = ''", correct me if I am wrong

(In reply to comment #13) > The code already handles disabled accounts by "WHERE disabledtext = ''", > correct me if I am wrong There are two distinct bits per account: disabledtext, which disables the account and prevents the user to log in (but the user still gets emails); and disable_mail, which only prevents the user from getting emails (but the user can still log in). You have exclude both, here.

Per my discussion with mkanat on IRC, and his veto about implementing this feature in the core code of Bugzilla, you should implement it as an extension. If some hooks are needed, we can add them in separate bugs.

Comment on attachment 499001 [details] [diff] [review] Password aging and history v2.2 Per my previous comment.

For the record, I don't really even want to ship this extension with Bugzilla. I don't really think we should support or encourage security theater.

(In reply to comment #17) > For the record, I don't really even want to ship this extension with Bugzilla. > I don't really think we should support or encourage security theater. So are you saying this is a won't fix then?

(In reply to comment #18) > So are you saying this is a won't fix then? That would be my vote, for the Bugzilla Project. Somebody could certainly write an extension, though.

I have written an extension for this

Comment on attachment 517675 [details] Password Reset Extension Hi rojanu. :-) Extensions don't need review, and they don't normally become a part of Bugzilla itself. The process for publishing extensions is described in the documentation of Bugzilla::Extension itself, I believe.

OK, created a code.google project (http://code.google.com/p/bugzilla-password-reset) and updated Bugzilla::Extension documentation