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

Dries drupal-devel at drupal.org
Thu Mar 31 11:46:38 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:   Dries
 Status:       patch

IMO, that setting has nothing to do with a site being "low level".  If
you request a new password, you probably want to login ASAP.  I vote
against a configuration option.
Either way, this patch needs work:
1. We don't glue words together (eg. $hashedpass, $currenttime,
$loginurl).  Similarly, I suggest to change 'user/newpassword' to
'user/reset' or 'user/request-password'.
2. The patch exposes technical details to the user; terminology like
"one time login URL" is likely to confuse many users.
3. The login URL is not clean URL safe, I think.
4. Please rename the variable $pass1 in user_pass_rehash().
5. The code comments are often inconsistent.  Some end with a
punctuation, some don't.  Some span two lines, others don't.
On a related note, can't we reuse this system for the initial password?
 That is, instead of mailing a password, we mail an URL and force the
user to enter a password.


Dries



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

March 11, 2005 - 04: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 - 06: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 - 06: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 - 10: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 - 22: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 - 22:41 : chx

Very nice patch. Please consider for 4.6.


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

March 31, 2005 - 00: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 - 01: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 - 03: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.


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

March 31, 2005 - 07:55 : chx

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.


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

March 31, 2005 - 08:12 : chx

Attachment: http://drupal.org/files/issues/user_one_time_login_1.patch (6.21 KB)

Implemented Chris' idea, so if you use a one time URl which is expired,
you are sent to user/password with a message to ask for another. I
changed a few messages and deleted the "not for admin" feature. Do we
really want one gazillion support requests "I locked myself out of my
site, user/password tells me not for admin, what now"?


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

March 31, 2005 - 09:24 : chx

One more point on uid 1. Yes, uid 1 can reset his pwd over the database.
But then we would need to lock out administer users privileged users.
Then administer nodes, 'cos of XSS. Then administer comments... then
administer filters... OK, this is pointless.
If someone can eavesdrop on your emails, everything is lost, I think.
This is not something Drupal shall address. Maybe we could employ a
security question -- secret answer pair.


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

March 31, 2005 - 12:52 : Bèr Kessels

Eventhough I am a less-settings advocate, I think this must remain a
setting :).
Why?
Sites on Drupal differ a lot, and have a very differnet userbase.
I run very low level sites, where it sometimes takes days before
someone will read his/her mail. But I also know sites (drupal.org)
where days would certainly be too long. 
Sorry Chris et al, I +1 on configurable duration.
(hell, we really need some about:config alike screen in drupal)

More information about the drupal-devel mailing list