Add "current password" field to "change password form"

nice idea, but you need then probably to have an extra "change password" field in case the user forgot the password and asked for a hash to be sent.

actually, for this to make any sense as a security meaasure you need to require the knowledge of the password when you change your mail address.

well, now we know what we want. The question is: Should this be in core? Could it be in a contrib module?

See the patch in http://drupal.org/node/86711 for a possible starting point.

The attached patch adds a "current password" field to the user's edit form which is required if the user wants to change any account information (this is mostly the code by kkaefer mentioned in #6). Concerning the issue of the reset password mechanism, I added a new node "user/%user/reset-password" which is merely a form with two password fields and a submit-button. I considered putting this into the "user_edit" form, but finally decided to create a new node because I thought otherwise it'd make the "user_edit" code too complicated.

has anyone worked on a module incorporating this ?

i am trying to build a module but i am very new to drupal .. so a bit confused .. wondering if someone could help me out ..

my train of thought:

have a node with a field for current password , and 2 fields for new password .. (enter pass and confirm pass)

compare the current password on the database to the current password entered ..
- if they are the same, replace current password in database by the new password
- if not, throw an error ..

this is assuming that the user is already logged in ... irrespective of being the admin or any user ..

on successfully changing the password, the user will be taken to a new page showing a message of the change being successful ...

can anyone help me out with this ????

Please don't recategorize issues like this! Start a new one for your proposed module.

This is about the proposed core change.

Related is this: http://drupal.org/node/138805 With that done, the problem with resetting forgotten password will be solvable by just not requiring old password in the "change required" state (see the other issue). I think that would play together nicely.

No longer applies.

Postponing this to D7.

just as a side note, while ur working on the user.module, u might wanna add a function where u could set an access right so u can choose if ppl can or cannot change their own password

might seem silly to some of u, but quite a few number of ppl are looking for this restriction

hi guys i have question regarding password field

problem is that on validation it resumes all the data in the field but clear password field and i want to resume it any idea please hellppp me

bjaspan's thoughts from the duplicate issue #251370: Require current password before changing it:

Standard security best practices dictate that we should require the user to provide their current password in the same form submission in which they specify a new password. This prevents "sneaker-net password change attacks."

If the user has forgotten their password and uses the one-time password link, we can skip requiring their old password because (a) they don't know it any more and (b) they just verified access to their email. HOWEVER, this implies that we have to require the current password before allowing a user to change their email address or else the sneaker-net attack is still possible.

There's at least one more duplicate #86711: Verify old password on password change.

Does anyone know if there's a contrib module for 6.x that does this yet? If not, I have one ready to go.

@Dave Reid;

I'm requiring something sort this flaw but I've had a search but can't see any existing modules. If you have something coded for this I would much appreciate being able to get a copy or it being released publicly, thanks!

@Dave Reid

I would need it for D6. can you just publish it as a module?

This is a patch for D6.12 from #86711: Verify old password on password change
I don't have a D7 for porting it, so leaving as "need work".

Please review

Patch for D7.

* Prevents changes to any of "Account information" fields without providing a current password. It's more secure way, as malicious user can block user or change the role without confirmation.
* Not require confirmations if it's an admin changing the data.
* Require confirmation if trying to change other admin's pass (need feedback on this. I think, it's wrong to not do it).
* Always require confirmation for changing current user's details.
* Patch supports generic "forgot password" scenario (previous aren't).

Simpletests for this patch will be ready when we'll come to consensus.

Waiting for your feedback. Thanks!

Reattaching the patch for testbot.

read this: http://lampsecurity.org/drupal-xss-password-reset

"Version: 6.x-dev » 7.x-dev"

this isn't going to appear in d6 at all then?!

new features/bugfixes first go in D7, and might get backported to D6 afterwards.

Hello again,

since Dave Reid didn't answer and my next Drupal project needs this functionality too,
I wrote a module by my own. It just uses hook_form_alter() and an other callback
function. all in all approx 55 lines of code and it works perfect.

if you wish, I will contribute this as a drupal 6.x module.

@dsms: http://drupal.org/project/password_change

@q0rban a-ha, thanks so much!

related to #590656: Harden one-time login links against vulnerability from disclosure of SQL backups, or SQL 'SELECT' injection

This is really a bug given how vulnerable it makes Drupal to session riding.

So - we should require current password to change password, e-mail, or add an OpenID association.

Adding tags

Subscribing. I noticed that #39636: Security: The "administer users" permission exposes user/1 was marked as duplicate but I'm not sure it is. I guess it's a duplicate only if code like this remains in the current patch:

+    $account_is_admin = user_access('administer users', $account);
+
+    // Always ask for confirmation from current user, or if trying to change password for admin or if you're not admin.
+    if ($GLOBALS['user']->uid == $uid || $account_is_admin || !$admin) {

That guarantees that "site admins" (including user 1) can't change each other's passwords without knowing them, but I'm not sure that user_access('administer users') is really the correct definition of a site admin? I'm also not sure we want to prevent that at all -- there is in general a use case for allowing highly-privileged users to edit any password on the site without having to know the original password.

I'd tentatively suggest that we remove the $account_is_admin stuff so as not to derail this issue with "what is an administrator" which is an impossible question to really answer.

Instead, how about requiring that you always have to type your own password when changing sensitive data on a user account (regardless of whether the account is yours or not)? This means that people with "administer users" will still be able to edit any account on the site, but if they accidentally leave themselves logged in to a public computer, the person who comes along next still will not be able to use that permission to go around hijacking every other account on the site.

>> that permission to go around hijacking every other account on the site.
It's exacly what they can do with global admin rights. They can edit other's admin password and log in it and then do what they want.

@David - that's a good suggestion - though it might make the code trickier. Lets try for a minimal solution for D7 - I'd consider this (liek parrword hash hardening which we already got in) pretty much a release blocker in terms of security.

I'm not actually sure it makes the code trickier? The most recent patch above has this:

user_check_password($form_state['values']['pass_current'], $account)

which we'd basically just change to this, no?

global $user;
user_check_password($form_state['values']['pass_current'], $user)

And then otherwise, we'd actually make the patch simpler, because we would not need to debate any kind of $account_is_admin stuff in the patch. And any discussion of whether "administer users" should actually let you edit every account vs. being more granular or whatever would not have to occur in this issue either, but rather would be left to #39636: Security: The "administer users" permission exposes user/1.

I still think this is critical for 7

Working on this with webchick in IRC, plus input from Bojan and yoroy on the text and UI.

Decided with webchick that while we could absolutely force the user to change their password after using a one-time link, that's a bit of a UX rathole and can more easily be a contrib add on. Here's the patch code that implements that:

@@ -1591,7 +1637,16 @@ function user_menu() {
  * Implements hook_init().
  */
 function user_init() {
+  global $user;
   drupal_add_css(drupal_get_path('module', 'user') . '/user.css');
+  if (isset($_SESSION['pass_reset_'. $user->uid])) {
+    $path = 'user/' . $user->uid . '/edit';
+    if (($_GET['q'] != $path) && variable_get('user_force_password_reset', TRUE)) {
+      // The user needs to reset their password.
+      drupal_set_message(t('You used your one-time login link. Please change your password.'), 'warning');
+      drupal_goto('user/' . $user->uid . '/edit');
+    }
+  }
 }

@pwolanin: I can easily just port http://drupal.org/project/password_change up to D7, so it is not a huge deal if this doesn't make it.

Here's the patch with the functionality - posting to see how many tests it breaks.

Also, function user_account_form_submit() doesn't exist.

@Dave Reid - I think this is long overdue for core.

I realized last night after further thinking that this patch is not yet robust protection. There are some users (e.g. Damien and webchick) who regularly use one-time links to avoid entering a password over http. The patch as is does nothing to protect them from the XSS attack that's the main concern.

So - the solution is to add a token to the session and initial redirect URL - thus if they navigate away from the user edit form, their URL will be missing the token, and the XSS attack will fail. However - they can still use the browser back at least to return to the URL that allows them to change. Coding that now.

With feedback from Heine - a couple minor changes to make the string more translatable, and the logic and comments a little clearer.

Plus add some tests of the user edit form.

I like this in principle.

• The current password field appears underneath the password fields. Not sure that is the best place
• If I edit another user's account "Your own current password", could simply be "Your own password", perhaps include the username there.
• There's no mention that I need to type my pwd to change the email address
• Current grouping suggest role changes and status changes also require my current password.

description field was missing due to typo in variable name.

re-ordered the fields and added a fieldset for visual grouping per Heine's suggestion.

Bojhan suggested the text "Your own current password", so leaving that alone for now. Patch plus annotated screenshots attached

Actually core UI patterns prohibit use age of indented fieldsets, although I agree the impression is given that role also requires your own current password, I doubt that is taken away by adding this fieldset. Other then that it looks good.

Fieldset removed - updated screenshot attached.

Looks good to me. If this still needs some more code reviews, feel welcome to put it back to needs review

huh? Why do I have to enter my password to change password or mail address of someone else (usually as site admin)?

@sun, probably because if you're admin (or just manager) and got XSS, this will prevent you from changing password for super user or other admins.

I agree we need this for the own account's password, but not for other accounts. When also used for other accounts, this protection becomes the start of a pattern to protect any security related action in the system. In the same way, one could perform an attack and add/edit text formats on behalf of a site admin, disable all filters, and afterwards post a content that executes PHP to change user passwords.

Per #15, I understand this as a measure against a different type of attacks - XSS should be taken care of in FAPI, or not? This should protect the password in the case you forgot to log out, and someone else uses that same computer. Which should be less probable for site admins (and is much more dangerous in that case already), but still protecting other privileged accounts (uid=1 even) makes some sense IMO.

Hm. I think I agree with sun in #55. But in any case, it sounds like this still needs more discussion.

Patch in #51 no longer applied so here it is rerolled.

It's a slippery slope but I hate to see nothing get in. The second patch attached, named 86299-current-pass-own-user-58.patch, removes the current password field from other users so people can test both.

I think if we are doing this it must include password for changing *any* users. see: http://seclists.org/fulldisclosure/2009/Mar/115 (the link to the original is broken)

I will check the 1st patch

Reposting after testing - this is the same patch as 86299-current-pass-58.patch (except rolled with git). I just want to be clear what I'm marking rtbc.

per catch - extremely minor changes to rename variable $not_my_account to $not_own_account and to fix one unclear code comment.

Also, add the ability for logged-in users to request a password reset per catch.

+++ modules/user/user.module
@@ -950,6 +951,33 @@ function user_account_form(&$form, &$form_state) {
+    // The user may only change their own password without their current
+    // password if they logged in via a one-time login link.
+    $not_own_account = ($user->uid != $account->uid);
+    if ($not_own_account || !$pass_reset) {

My point in #55 still stands. The disclosure you linked to even contains a template code snippet I can use to gain access to PHP filter, so I can change (all) other users' passwords via PHP, circumventing this own password protection -- or better yet, I can simplify things for me and just alter user roles and user permissions.

Do we want to add own password validations to all of those forms?

1 days to code freeze. Better review yourself.

The issue with password reset was if you get to tht form, realise you've forgotten your password, then the only way to reset it was to log out, then browse user/login, then click the password tab - which requires a knowledge of Drupal's inner workings we can't expect.

Those who do know, and browse to user/password manually, like me testing the patch, get a big fat access denied too.

The access change, link in current password description + showing which e-mail address to send it to is pretty standard across most sites, so seems like a good fix for that. Since this was otherwise RTBC for a while, marking back to RTBC for #61.

Here are 2 screen shots showing the changes.

cross-post. meant to leave at rtbc

@sun, that's still theoretically possible for a targeted attack, but adding this also prevents more casual hacking like admins leaving themselves logged in at public machines (one site I help with in spare time had prank posts from admins when they had friends staying over at their house for example, changing passwords would only be a couple of drinks away).

Everyone knows how to change someone's password and e-mail address, not everyone knows how to mess with text formats and write PHP.

Everyone knows how to change someone's password and e-mail address, not everyone knows how to mess with text formats and write PHP.

I agree. Not only that, but only certain users on certain sites are even able to grant themselves access to the PHP format. (Some sites remove the PHP module completely, for exactly this kind of reason.) Just because there are some cases where leaving yourself logged in on a public computer means all hope is lost doesn't mean we shouldn't protect everyone else.

From looking at the code (did not test the patch but seems like others did), I agree this is basically RTBC. A few minor nits - can be fixed in a followup if necessary, IMHO.

1. +      '#prefix' => '<p>',
   +      '#markup' =>  t('Password reset instructions will be mailed to %email.  You must log out to use the password reset link in the e-mail.', array('%email' => $user->mail)),
   +      '#suffix' => '</p>',
   

   Why have a separate prefix and suffix if we are already using #markup?

2. +    // Test that error message appears when attempting to change mail or passs
   +    // without the current password.
   

   Typo.

3. +      $current_pass_description = t('Enter your current password to change the %mail or %pass.  !request_new.', array('%mail' => $protected_values['mail'], '%pass' => $protected_values['pass'], '!request_new' => $request_new));
   

   This wording seems odd - and you can see from the screenshot that "E-mail address" and "Password" wind up capitalized, which also looks wrong. Maybe at least lowercase them and remove the italics? I'm also struggling with the fact that there is no flow between the first and second sentence, and that the word "password" is used twice to refer to two different ones....

   Maybe something like: "Enter your current password to change the fields below. If you've forgotten it, request a new one."

4. Ideally, there should also be a test for changing someone else's password - seems like the tests in this patch only test the case where you edit your own?
5. One situation where this feature will get very annoying is if you have to make multiple changes in a row (e.g., editing a bunch of different user accounts). An interesting/useful followup might be to store something in the session so we only ask for the password on this form if we didn't already do so in the past X minutes. Seems like that's definitely for a followup though.

re: comment #1 - The wrapping tags as prefix/suffix is used elsewhere in core such as update.manager.inc. The idea is to allow a form_alter to change the wrapper tags without needing to regex an already concatenated string.

re: comment #3 - I used uppercase/italic to exactly match the titles for the relevant form fields rather than having any ambiguity.

Added additional test code and fixed code comment in the test.

Minor nitpicks, also not holding up commit.

+++ modules/user/user.module
@@ -950,6 +951,33 @@ function user_account_form(&$form, &$form_state) {
+      $current_pass_description = t('Enter your current password to change the %mail or %pass.  !request_new.', array('%mail' => $protected_values['mail'], '%pass' => $protected_values['pass'], '!request_new' => $request_new));

There's an extra space before !request_new

+++ modules/user/user.pages.inc
@@ -36,6 +38,16 @@ function user_pass() {
+      '#markup' =>  t('Password reset instructions will be mailed to %email.  You must log out to use the password reset link in the e-mail.', array('%email' => $user->mail)),

Extra space before 'You' here.

Thanks - those are easy to fix. Patch just removes 2 spaces from the strings

I'm 100% with sun in #55. It seems crazy to require this for every user -- what's next? Entering a password on the module administration page. ;-)

I haven't tested the patch yet, so #55 might have been addressed.

I've to run right now, but I'll look into this more shortly.

@Dries - did you read the comments above - if you leave your computer open in a cafe - the person next to you grabs it and changes passwords for all the other admins on d.o? Or alternately, *I* leave my computer open in a cafe and your account password on d.o is altered. Or someone changes your e-mail address, requests a password rest link (allowing them to log in as you later) and changes the e-mail back?

This is obviously not complete protection against manual or XSS attacks, but it would mitigate the publicly available XSS script to change Drupal passwords, and would prevent the most obvious manual attacks.

Still, I'm really missing the point of why you see this as a problem to require for all users?

+1 for pwolanin arguments and point 5 in #67, though it seems more likely to get this functionality in some contrib module called "Better passwords security".

what's next? Entering a password on the module administration page.

Only if another issue requests it and a patch gets in. I don't think such a thing belongs in core, so I would vote it down, but it could exist in contrib.

I'm +1 on requiring your current password to change another user's email or password. A followup issue could implement a time-based assess cache (think sudo) so that you wouldn't have to enter your password more than once in X amount of time.

pwolanin in #73:

if you leave your computer open in a cafe

We have a in core about not babysitting broken code. To me this sounds like babysitting broken behavior. If one of the d.o admins leaves there laptop open in a cafe, someone might just as well decide, that Project module might better be turned off on drupal.org, or if someone with FTP access to the drupal.org codebase does likewise, that drupal.org should really run Joomla! and then delete the entire codebase. I think requiring admins to provide their password establishes a UX paradigm that you realilstically can't be consistent with throughout Drupal. And consistency in the UX is, to me, more important than that maybe-won security for careless admins.

- I really don't like the "Your own current password" field on other user pages. I don't understand why we protect those pages, but not all of the other administration pages? It feels random. If someone gets access to my account, there are many other things he/she can do.

- It also isn't necessary to protect all fields. We should only ask the current password when one wants to change the password or the e-mail address.

- I'm also tempted to lower the priority because this is not 'critical'. It is a weakness, but not a critical weakness.

Let's continue to discuss this some more.

@tstoeckler - it would be lovely if we had time to categorize all forms in Drupal and whether any given change has a security implication (or define a new 'secure form type'). But that's not going to happen this year.

This patch addresses a clear and well-defined and long-recognized (and recently publicized to hackers) vulnerability in Drupal core. UX can never trump all other concerns.

@Dries - did you try the patch or look at the screen shots?

With this patch your password is only needed to change the password and e-mail. Nothing else.

Also - without protecting other users credentials, I think there is no point to this patch. The only use case is a site run by user 1 with no other admins at all. Otherwise, once you have 2 admins, if admin #1 leaves their laptop open, the attacker can change the apssword of admin #2.

I looked at the screenshots and it is not clear that you only need to enter the password conditionally. In that case, we need to put in some extra UX work IMO.

@Dries - see #49, the second screenshot is a version where there was a nested fieldset suggested by Heine to visually group the items requiring the current password. Bojhan argued that the description is enough (it lists the fields), and that a nested fieldset violates the Drupal 7 UI guidelines, so I took it out.

We could try something else like a change in text color or I something else to make this connection evident?

Note that I attempted to work on a generic approach in #588550: Allow the user edit form to only ask for the current password when necessary (in a followup confirmation step) and will likely continue with that for D8.

For me, this patch really only makes sense if we limit its scope to securing the current user's own password/e-mail only. Any broader attempt is 100% equal to entirely questioning the security for form submissions throughout Drupal. Squashing some "own password" fields onto forms is a poor answer to the overall problem and doesn't feel like the right approach. If the goal is to prevent XSS attacks, then it would make much more sense to prevent submitting certain forms via XHR, or similar solutions. In other words: Applying this to more than the own user's password/e-mail is an attempt to fix the trigger, but not the cause.

"Also - without protecting other users credentials, I think there is no point to this patch. The only use case is a site run by user 1 with no other admins at all. Otherwise, once you have 2 admins, if admin #1 leaves their laptop open, the attacker can change the apssword of admin #2."

Hm? IMO there's certainly a point to this patch without prompting admins to re-enter their credentials. It would prevent the much more straight-forward (and common) "laptop open at a cafe" account hijacking attacks on "normal" user accounts. That's a big security win, in my book.

Protecting admins from other admins is also a good security goal to have, but if we want to go down that particular road, you have many more things you need to protect against, and then this starts to feel more like a one-off solution that barely scratches the surface. Exploring the full extent of how admins can screw with other admins is something that's probably best left to Paranoia module.

Or, "What sun said." ;)

@webchick - now I'm confused, since in our discussion before I rolled the patch, I laid out this functionality and you said you agreed (nay, insisted) that the logic for checking a password for changing some other user's pass/mail must be identical to the logic for changing your own.

Clearly this does not play the role of a full "paranoia" mode/module in terms of limiting admins or checking their credentials for all configuration changes, but I still think it's a useful hardening.

I think there is value in splitting this patch up. The first part (protecting own account) is a no-brainer. The second part is required too, but might need some more thought and UX work.

I IMed with pwolanin and the second part is required in the case someone gets access to a user account with 'administer users' permission. In that case, an attacker would be able to promote another user.

Either way, I'd love to get the first part in.

I'm starting to be convinced that protecting other user's password forms can be out of core. Here's Peter's patch for use case 1: protecting own account. I've removed the test for changing another user's password as well.

The patch in #88 looks good to me. Marking this RTBC.

If another users are not protected, you might as well not protect yourself, as there will be the U-turn loophole. (at least users with the permission to change other users should be protected, in addition to your own.)

this patch should only focus on account protection.

(and yes, i think sudo for drupal is a good thing.)

subscribing

Patch still applies with offset and Dries RTBCed it so I guess that means it's up to webchick for commit?

Cool, reviewed the patch and didn't find anything to complain about, and at least this stage has buy-in from the security team + both core committers + the majority of folks in this issue.

Committed to HEAD.

In Drupal 7.0-alpha2 release (22 Feb 2010), current password (current_pass in user_account_form) is accidently passed to user_save function, and saved in user data column as clear text.

Well, that's pretty serious. Good catch, clojel. Patch unsets the current pass fields in user_user_presave().

I applied the patch. Works!

Without the current password I can't change the email or password.
But I still can change the username.

In case I change the username and the emailadress and I don't supply the current password, the username get's changed, but the email stays unchanged.

Looks rtbc then. (I did not test/review, but based on #97 and the small size of the patch in #96)

We really need to kill user->data, see #347988: Move $user->data into own table and #721436: Remove magical fairy saving of cruft from user_save(). clojel, very nice find.

Edit: I've posted an alternative patch to remove the saving on the other issue.

Also, given all the other cruft removal is in user_save() itself, until that other patch gets in, this needs to be there too. I don't know why the user_picture stuff is split between _save() and pre_save() but that's not a good thing to emulate here.

OK, here it is moved into _save()

Looks fine as a band-aid

#347988: Move $user->data into own table is now postponed to D8.

Marking RTBC again given that the current patch addresses catch's concern and that this is truly critical and that we may not fix the automatic saving of $user->data stuff in D7.

Agreed. This is extremely bad and needs to be cleaned up by the next alpha, regardless of $user->data's ultimate fate. For once, I guess it's a feature that the upgrade path is so broken. :P~

Committed to HEAD.

Ahem. Committed to HEAD this time. :P (Thanks for the heads-up that this was missing, coltrane!)

One of my users complained about this, but we're not ready to move to D7, so I hacked together a backport for D6 with Pressflow. I figured I might as well drop it here too. If you can get it working, congratulations!

Any hope for this being backported (officially) to D6 core??
It's going to be years before some of us can realistically migrate to D7.

Thanks.

@nodecode: http://drupal.org/project/password_change