[drupal-devel] [feature] Request New Password Security

chx drupal-devel at drupal.org
Thu Mar 31 05:56:01 UTC 2005 

Issue status update for http://drupal.org/node/18719

 Project:      Drupal
 Version:      cvs
 Component:    user.module
 Category:     feature requests
 Priority:     critical
 Assigned to:  Anonymous
 Reported by:  neofactor
 Updated by:   chx
 Status:       patch
 Attachment:   http://drupal.org/files/issues/user_one_time_login_0.patch (4.72 KB)

I cleaned up the patch a bit. I hope code style is OK. So may -- there
are no checks against the arguments? There is no need. user_load uses
%d for 'uid', so that's dealt with. Comparison by < and > means an
implicit cast to integer, so $timestamp is also dealt with
automatically.
I do not think timeout needs to be configured.
Please comment on not letting uid 1 using this one. I am not sure
whether this is necessary.


chx



Previous comments:
------------------------------------------------------------------------

March 11, 2005 - 03:52 : neofactor

Problem:
Any user can force another user's password to change... simply by
selecting "request new password" and putting in their username. The
user gets an email with the new.. but this feels like a violation to
the user... and a pain.
Solution?
If someone requests a new password... Don't blindly change it... send
an email that says...."Is this a real request authorized by you? Click
here to confirm otherwise disregard this message"
Please consider this critical for user by-in to Drupal as a secure
system.
I appreciate your consideration.
http://drupal.org/node/18689


------------------------------------------------------------------------

March 11, 2005 - 05:05 : neofactor

I added some code to prevent the admin account from being reset...
Please add as a patch.
if ($account->uid == 1 {
unset($account);
form_set_error('name', t('Sorry. The username %name is not allowed to
be changed.', array('%name' => '<em>'. $edit['name'] .'</em>')));
}
// Just above this code on line 911:
if ($account) {
$from = variable_get('site_mail', ini_get('sendmail_from'));
$pass = user_password();


------------------------------------------------------------------------

March 11, 2005 - 05:08 : killes at www.drop.org

Please only set to "patch" if you are actually submitting one. The
solution you proposed is rather a workaround and will not be acceptable
for core.


------------------------------------------------------------------------

March 11, 2005 - 09:21 : Deno

I was in charge of a web site with >10000 paying customers for some
time, and the "customers care" was constantly troubled with pasword
change requests, until we switched to a system that worked in the
following way:
1) "I forgot the password" function accepts either users email, or
users login name, generates a random phrase, and adds an entry to
"pasword_change_request" table. This entry consists of:
- User ID (corresponding to email or login)
- random phrase
- datestamp
User ID is unique key, and the entries in this table older than
MAX_TIME are regularly purged by a cron job.
2) System sends an email with a link to "change forgotten password"
page with this random phrase as argument to the user. That would be
something like:
https://drupal.org/user/cfp/d438rwiuodw894732ehdw
3) When user follows the link, "cfp" page checks if the random phrase
exists in the table, allows the user to immediately change the
password, and sends another email with "your password was changed on
RIGHT_NOW ... " note to the user in question. 
- Password change automatically triggers deleting of the corresponding
entry in pasword_change_request table.
- Password change automatically logs-in the user as well. 
This way, users weren't bothered by accidental password changes, and
they also understood the system better than the one with automatically
generated passwords.
For additional security, we also built some brakes that assured that
the number of attempts to change the password, such as:
- "An email with instructions was sent to you X minutes ago. In case
you don't receive it within N hours, please contact the site
administrator"
- "According to our logs, you have visited the CFP page more than NMAX
times within last 24 hours. For security reasons, you will be denied
access to this page for the next 24 hours. Please contact the site
administrator."
hope this helps...


------------------------------------------------------------------------

March 30, 2005 - 21:01 : Jose A Reyero

Attachment: http://drupal.org/files/issues/user_one_time_login.patch (6.52 KB)

This patch for -cvs- addresses this issue.
It generates an *only one use* URL to login into user account,  instead
of a new password. Then the user clicks on the URL, logs in, and may
change his old password if desired.
It is a only one use URL, and has some additional security measures,
like a configurable time-out -can be confgured in admin/user/settings-
and also, if somebody uses it or password is changed in the meanwhile,
previously generated login URLs of this type won't work.
About how it works, it creates a new url with:
- user id,
- timestamp
- a hash of current timestamp, changed date for account, and the old
password hashed
With this patch, this wont work for admin accounts, which I think is
better, you never know...
It has a number of features like:
- User can be ignoring these emails if somebody else is requesting
access, a new password request wont change current password. However,
the user will be warned when seeing the emails
- It reveals no sensible information about the user account in the URL
itself
- the generated hash is not predictable at all if you dont know the old
password, as it depends on user's old password
- there's a configurable timeout
- The URL's are only one use, as they depend on two timestamps, one of
which will be changing in case this login is used -or in case user logs
in for other means
- Any 'new password' request is independent of others, so no one can be
interfering with one user asking for his password back.
Also doesn't require any new db field nor keeping track of requested
passwords.


------------------------------------------------------------------------

March 30, 2005 - 21:41 : chx

Very nice patch. Please consider for 4.6.


------------------------------------------------------------------------

March 30, 2005 - 23:16 : drumm

Couldn't those nested if statements be made into one with a bit cleaner
indentation? It looks like the page which the user arrives at after
getting the new login information is their user page, not a page to set
the password. The "Time out for password recovery e-mail" configuration
is very unfriendly. It should be a drop down select of the most used
human readable durations. Or we could just decide on one timeout and
leave that hard coded in; who actually needs to use that setting? The
url is quite long, what could be done to make it shorter?


------------------------------------------------------------------------

March 31, 2005 - 00:50 : Jose A Reyero

/Couldn't those nested if statements be made into one with a bit cleaner
indentation?/
Maybe, but I had to split that if conditions in two parts because the
$account var didn't get the value before the rest of the conditions
-latest two- where evaluated. Someone can help with PHP here?
/It looks like the page which the user arrives at after getting the new
login information is their user page, not a page to set the password. 
The "Time out for password recovery e-mail" configuration is very
unfriendly. It should be a drop down select of the most used human
readable durations.  Or we could just decide on one timeout and leave
that hard coded in; who actually needs to use that setting?/
Agreed, will be polishing this a little bit, just wating for some more
suggestions...
For the duration, I'll change to 'hours', ok? Just don't like to
restrict options using unneccessary dropdowns, it's only intended for
administrators anyway. Maybe I add also a '0' for unlimited.
Hardcoding it, would need anyway to be mentioned in the module help, so
it's about the same overhead, both for coding and for the admin
interface, and harcoding in two places in code is not good anyway. And
I like having options, am I the only one?
/ The url is quite long, what could be done to make it shorter?/
Well, the url has to be long, it has neccessary params and an MD5 hash,
which yes, could be cut someway, but that adding inneccessary code and
also reducing security -though half an MD5 hash may be secure enough-
but I see no point in that.


------------------------------------------------------------------------

March 31, 2005 - 02:18 : FactoryJoe at civicspacelabs.org

"Or we could just decide on one timeout and leave that hard coded in;
who actually needs to use that setting?
"
+1 for making this *not* configurable! C'mon, Drupal should have
standards about security -- and if best practices say that, say, 8
hours, is a good amount of time for that URL to be active, then do
that. Cut out the confusion and extra brain juice it takes to decide...
"Gee, is 4 hours better than 5?"
Additionally, there's a simple way to make this workflow better for the
user. If you visit an outdated change password URL, you should be told,
"Sorry, but that link you followed to change your password has expired.
If you would like to reset your password, please fill out this form:
[form] If you did not request to change your password, please notify
[administrator's name w/ PM link]".
This way, if you missed your change password deadline, you can request
your password again right from the timeout page. If you didn't request
the change, it will be obvious that someone else was trying to change
it, and then you can contact the administrator and find out who it was
that was messin' with your account.

More information about the drupal-devel mailing list