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

Jose A Reyero drupal-devel at drupal.org
Tue Apr 5 00:20:25 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:   Jose A Reyero
 Status:       patch
 Attachment:   http://drupal.org/files/issues/user_one_time_login_3.patch (20.26 KB)

Ok, lets try to make people happy, which will be good for my karma, I
hope so ;-)
So, changes:
- Added 'lastlogin' field to user table. Now there's one timestamp for
when an account is edited and another one for login. Of course, I've
also taken care of the user's admin page.
- Some small detail like help text for e-mail fields and that.
- Now it can be used also for accounts with admin approval.
- And yes, removed config option, 1 day time out for password recovery,
no time out for first login.
By the way, I've also patched db files for PostgreSQL, please someone
review it, I don't know too much about it.
The default e-mails may need better wording. Specially the one for
password recovery which just says 'You have requested a new password',
but I dont want users get too worried in case someone else has
requested that password and the old one that said something like 'This
is your new login information..' which may suggest it has been sent by
the admin maybe updating accounts..... Anyway, every site admin can
configure that email...
As an idea for the future or for someone else - I've had enough for now
with logging in 100 times to try this-, now that we can know when it is
the first login for an user, a nice welcome message or welcome screen
would be great :-) Or if someone wants to go for better security *nix
style, some message telling the user when last logged in wouldn't be
bad...
So... ?


Jose A Reyero



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.


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

March 31, 2005 - 06: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 - 07: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 - 08: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 - 11: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 - 12: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 - 18: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?


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

March 31, 2005 - 20:43 : Dries

"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 ...


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

April 2, 2005 - 19:45 : factoryjoe

I’m going to have to stick by my earlier assertion on this one, and,
and, as with most usability issues, none of us are “correct” until
we test it 50 times and see whether anyone actually uses the option.
Just like with my folksonomy review [1], more options is *not always
better* nor does it always help people *get things done*!
To further expand this point, Jose says [2]:

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.

"
But this is a misuse of the term “usability”. What you’re talking
about is *functionality*—since usability has more to do with focusing
single-mindedly on the task at hand and reducing distraction than it
does with providing extraneous options that might be useful for a
fraction of all Drupal admins.
To speak to Ber’s point [3], I cannot imagine a situation where, if I
want to change my password because I’ve forgotten it, that it will
take more than a few hours to get the email and fulfill the process.
And even still, if it does — for whatever reason — take me a week
to get around to resetting my password, then starting the process over
again isn’t all that time consuming. Plus, I have the added sense of
security that Drupal doesn’t let things like password resetting links
stay active forever!
So again, I agree that we need a reasonable default—24 hours is very
generous for the singular task that we’re trying to serve here, which
is, to my knowledge, to make the process for resetting your password
easier and more secure.
Lastly, about the patch itself: I noticed a couple <em> tags strewn
around in lines 38, 43, 54 and 72 (as well as a <strong> on 54). Until
we have a style guide for marking up usernames, should we really be
using HTML like this? It seems to be that it would be better to use a
span with a class of… say… “username” and let the themer
determine whether the username should be bold, italicized, red, all
caps and so on.*/
[1] http://drupal.org/node/19697#comment-25599
[2] http://drupal.org/node/18719#comment-25480
[3] http://drupal.org/node/18719#comment-25463


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

April 2, 2005 - 21:29 : factoryjoe

One other quick question. How does this patch interact with distributed
authentication? If I'm logged in with a remote account and ask to reset
my password, what happens?


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

April 3, 2005 - 10:22 : Dries

factoryjoe: you can't reset your password when you are logged in.


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

April 3, 2005 - 14:03 : Jose A Reyero

About usability I've written this piece [4], so I really prefer to
discuss it on the forums...
That patch, I'll be cleaning it up a little bit, adding that last login
field, and maybe go for that 'reasonable default', which anyway is not
worth so much discussion....
[4] http://drupal.org/node/19918

More information about the drupal-devel mailing list