Plain text user password leakage

@hgoto Hashing the value of the submitted password at the earliest possible time is important because it lessens the window of opportunity for the value to get accidentally exposed when writing serialized data to a log file or the watchdog error table. With this change, we can at least prevent having an unhashed password as part of the created user entity before user_save is called.

I did notice a concerning but unused set of code that implies the sending of passwords via email in plain-text, but I'm guessing that this is quite old and was just not removed when the support for sending the password in the welcome email was removed.

// Add plain text password into user account to generate mail tokens.
$account->password = $pass;

Thanks for working on this.

Re #15: Yes, Drupal core removed the support for sending passwords in registration emails here: #660302: Migration path for changed user email tokens; don't complicate translation of default messages → . However, there are some contrib modules still using this, so we need to be careful and not break these. For example:

LoginToboggan (https://git.drupalcode.org/project/logintoboggan, "provide a user:password token that can be used to send a plain text password as part of registration process.")

Registration Password Token ( https://www.drupal.org/project/rpt → )

... (I have not done a full search for such modules) ...

-----

I think the one important point in the parent issue was to remove the plaintext password from the user object, so that it cannot be accidentally serialized/written to the database. If we change it in the form_state, then we are probably going to break multistep forms, some contrib modules, etc., which I think is not something we can do in this D7 phase.

Also, the MR is making changes to user_register_submit(), but what about user_profile_form_submit()?

Third thing - we are still saving the plaintext value to the $account object in user_register_submit(), see:

  // Add plain text password into user account to generate mail tokens.
  $account->password = $pass;

Other than that, the MR is also missing additional tests, which we will need before a commit, so given these points, I am changing the status back to Needs Work. Thanks!

@akalata Can you explain the status change with regards to my questions raised in #16? Also why have you removed the "Needs tests" tag. Thanks!

@poker10 I hadn't refreshed the page to see your comment and the additional tag. reverted.

@poker10 I'm a bit confused by your feedback regarding the plain-text password at `$account->password = $pass;`, which I called out in #15. First you say that it's necessary to support contrib module tokens, but then you mention it as your "third thing" which makes it sound like something that needs to be changed?

If we don't want these changes in `$form_state`, for the reason that you've mentioned above, is there a recommended alternative? Seems like we would need to add a try/catch to every function that accepts $account or $form_state, and clear the password value before throwing any exceptions:

• entity_form_submit_build_entity()
• user_save()
• entity_uri()
• _user_mail_notify()
• user_login_submit()
• form_state_values_clean()

Given the level of disruption THAT might cause, is this worth even attempting?

@akalata By my comment about the "third thing" I meant that the MR is hashing the password to $form_state['values']['pass'], but the password is still saved to $account->password in a plaintext. So I was not sure about the benefit of the solution, if we still keep the plaintext password on another place. I will try to take a look at this, what other possible solutions we have here. Thanks!