[drupal-devel] [feature] Request New Password Security
Dries
drupal-devel at drupal.org
Thu Mar 31 19:43:49 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
"So ... everyone happy?"
Not quite. The patch is an improvement but it uses tabs, incorrect
spacing, incorrect placement of brackets, and does not use
theme('placeholder').
If a new database column is required to implement this properly, please
add one. This patch won't get committed until it is implemented
properly.
I still vote to remove the configuration option.
We're getting there ...
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)
------------------------------------------------------------------------
March 31, 2005 - 13:46 : Dries
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.
------------------------------------------------------------------------
March 31, 2005 - 19:20 : Jose A Reyero
Attachment: http://drupal.org/files/issues/user_one_time_login_2.patch (12.77 KB)
Ok, I've started with chx's reviewed patch, and done most of Dries
suggestions
Main changes are:
-----------------------
- Rewording, variable names, path, etc... as pointed out by Dries. Now
it's user/reset. And yes, it was not clean-URL safe; it is now.
- Allowed also for admin account, I dont like it though, but seem to be
the only one :(
- Used also for user registration, but I also kept old style
login/password, think it can be some use and anyway, it's up to the
site admin to remove it from the welcome e-mail. However it is only for
'no admin approval', see below about this....
- Added back in the config option for time-out, though reworked
completely for usability. Default is '1 day' which I think should be
reasonable. However, there's no time out for firts time users. About
this, read below, please...
-----------------------
Using this for first time login has some problems when admin aproval
is required, as the hashed pass depends on 'changed' timestamp, and it
changes when the admin approves an account. Actually, there's no way to
know -or I didn't find it- when it is the first login for an approved
account. For no admin approval, I just used 'changed' == 'created' as a
criterium.
I think we should be separating this changed date and last login date
creating a new field in the user table, but that maybe for a future
patch. That would also increase security, as the Administrator's info
would be more accurate, and 'last login date' could be shown when user
logs in, which is also security+
Related with this I've experienced problems, as administrator, when
editing user accounts, then last login info for the users gets lost -is
reset-...
About the config option for time out, though I agree that enforcing
security can be good in general, this is one of this cases where more
security means less usability. And that decission should be left to
Site administrators -each site has its own security requirements-.
So I think the target here should be a 'reasonable' default, and then
some flexibility for site admins. IMHO this latest patch is quite well
balanced. So... everybody happy?
More information about the drupal-devel
mailing list