Plain text passwords can be accidentally dumped to the database by code that doesn't intend to do that

As I posted in #2418119-124: REST user updates bypass tightened user account change validation I think it has to be that way:

I do not think it's possible to drop it. When I consider doing a multi-step workflow for updating the user account where the last step saves the input, while the first step needs password verification, it would loose the password during serialization in step 2 and fail validation afterwards.

Furthermore, the password would be already in serialized form state, so is it really an issue if it is an the user object also? But probably more important is that it's skipped by the symfony serialization system what's already covered generally for PasswordItem by #2338559: Never serialize password fields by default.

Was this intended to be a critical issue? I don't think it should be...

I think it depends on what the code paths are to resulting in a clear text password being serialized to the database/cache/kvstore. #1 implies it could be a pretty common code path, which does seem critical to me. I don't know that the title as written is necessarily the required solution, but having some protection against clear text passwords in the database seems necessary.

Adding my related comment from #2418119-127: REST user updates bypass tightened user account change validation:

Looking details up, #124 and #126 is not correct in the following:
>Furthermore, the password would be already in serialized form state,...
> the password is around in $form_state values and so on as well, which are serialized too

The password is not in form state as part of the form values - as those are not part of its "cached" version. However, it becomes part of the new user object when changing or creating a user until it's saved (it get's converted to the hash in presave). Thus it is already serialized as part of the user object if it ends up in form state storage for multi-steps or ajax requests on the profile form. So having the plain text password in there sometimes is not something new.

I think we could solve the problem for updating / setting the pass in a multi-step form workflow by hashing the password when it's serialized, and keeping the hash only. For the existing password, we could just skip it during serialization and force people using a multi-step workflow to explicitly skip existing password validation in sub-sequent steps if it validated fine before? Or we serialize an internal flag that the changed password got validated.

#1 implies it could be a pretty common code path, which does seem critical to me.

That could need some verification, but checking the code flow I think most ajax requests do not store the user object in form storage as the skip form-level submit (e.g. add more, file upload). Of course there might be other ajax request which do not skip form-level submit or run the entity building theirself, what would trigger the issue just as doing a multi-step form. So it doesn't look like a common scenario to me, but it certainly can happen.

I agree that now that we can we should not let the password be stored on the user object ever, but always only the hash.

Is that a problem in D7 as well? I think it might be if you make the registration form a multi-step form ...

Yes, I can't imagine that the situation is any worse than it was in 7.x. So if there's a real security issue here, then that would be affected too?

For the existing password, we *need* the plain text password. The API doesn't allow to compare two hashes, because new hashes always have a different seed. check() requires the plain text password.

For a new password, it's tricky too. Right now, the API works so that on preSave(), we check if we have a different value and if yes, hash it. We could change it so that setPassword() would hash, but those methods on entities are essentially just there to improve DX a bit. Form value assignments or anything that accesses or sets content entity values in a generic way uses the entity field/typed data API.

Also, don't forget that, as discussed in #2418119: REST user updates bypass tightened user account change validation, there are actually valid use cases for modules to have access to the plain text password, for example when third party systems need to be updated, some people there confirmed that they are doing things like that.

Suggestion: Add a line in User::postSave() to empty the existing password field. Then it is only set between building the entity object, validation and saving, if anything serializes the entity between that, or sets the existing password in a different context, then that's their problem.

Was this intended to be a critical issue?

I think have plaintext passwords stored on an object that is statically cached and therefore available to all code - it needs to be thought about. At the very least, we should be removing the existing password on save.

I have tested and atm we are never serialising the user object with plaintext passwords atm.

So based on #8, at the moment, this isn't an actual security bug, so removing that tag. Which means it's also not in the way of an upgrade path, so removing that tag too. Security improvements can be either critical or not depending on various factors, so not changing priority yet.

We should prevent plain text password from ever possibly ending up in a database dumps. If the user objects get serialized with the existing password or in the middle of a password set it will have plain text passwords - yes this does not happen in core - but as people have said all it will take is for someone to implement a multistep form and we might have this issue.

I don't know what else to do here.

Yes, it would be nice if we could only store a hash, but again, the password service simply doesn't allow for that.

If we wanted to mess with the password system you could always make an unsalted hash of the user input and use that as the input to the password comparison routine.

Basically the same as the way the D6 to D7 handled the extra md5 over the plain text I'm not sure that's practical at this point, however.

What happened to the earlier idea of preventing serialize if the raw password is present?

What happened to the earlier idea of preventing serialize if the raw password is present?

That would actually be pretty nice - I think.

Simple change to one word in a comment in Berder's user-reset-existing-password-2463113-10.patch from above. (minify -> minimize).

UPDATE: Patch failed because I don't have testing environment quite correct yet. My apologies. Oh. And hi, everybody. :)

Discussed this on a triage call with @alexpott, @webchick, and @xjm. On the one hand, babysitting broken contrib code isn't normally a critical priority issue: there's lots of ways that contrib can do something stupid and thereby introduce a security bug. However, what makes this issue special is:

1. Compromising user passwords is a really, really bad security bug that deserves extra special consideration.
2. #6 claims that D8 HEAD is no worse in this regard than D7, but I don't think that's true: in D7, a contrib module would need to explicitly put a password into a $form_state key that gets serialized, which puts the vulnerability clearly into that module's responsibility, whereas in D8 HEAD, it would only need to implement a multistep form protected by needing to enter the existing password, and then forget to clear that out between steps, which makes the responsibility for the vulnerability shared between the module (for setting up the conditions) and core (for serializing a plain text password as a default behavior under those conditions).

So, we decided that it really is a critical bug that needs fixing. However, the solution need not be the one that's in the current issue title (e.g., #15 is a different approach), so retitling to focus on the critical portion of the bug.

diff --git a/modules/user/user.pages.inc b/modules/user/user.pages.inc
index f21bd13..b8fab21 100644
--- a/modules/user/user.pages.inc
+++ b/modules/user/user.pages.inc
@@ -316,6 +316,8 @@ function user_profile_form_submit($form, &$form_state) {
   $account_unchanged = clone $account;
 
   entity_form_submit_build_entity('user', $account, $form, $form_state);
+  var_dump($form_state['user']);
+  exit;
 
   // Populate $edit with the properties of $account, which have been edited on
   // this form by taking over all values, which appear in the form values too.

Filling out the user edit form shows that pass and current_pass is part of the user object. And if you're doing multistep forms in 7.x, you will do something similar, otherwise the values in there are lost.

So, I still don't get how this is worse in 8.x?

Yeah, I think as well that this is the same is in d7: If you are doing a multi-step form including e.g. the changed password or new password the user object is going to end up in the form state, including the plain password.

What happened to the earlier idea of preventing serialize if the raw password is present?

That would actually be pretty nice - I think.

That would break your multi-step form then - not so nice, I think. ;)

The only way I see this possibly working is by
- fixing user password field to hash new passwords immediately when they are set
- improve checking existing password to work with a hash. Not sure whether I'm missing something right now, but we should be able to hash the existing password when setting using the same hash as the current password. Then, checking later on is as simple as checking that the hashes are equal. Thats needs some API changes for make Password service allow us that though.

Okay thinking about this some more and reading comments from @fago and @Berdir - I agree multi-step forms in D7 have the problem. Given that I'm downgrading this to major.

The patch attached moves the unset of the existing password property to the field item's preSave method so all password fields have this.

Removing "Triaged D8 critical" tag, as this is no longer critical. Re: #20

Re-roll plus added @var comment.

RTBC, looks great to me.

ugh, bot fail? reposting patch

re-posting patch to see if ti's still valid.

Needed a reroll since the patch didn't apply anymore.

Marking this RTBC again as it applies nicely, and the only thing that changed was some of the chaff (spacing and following code) around the chunk in PasswordItem.php

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

Filed a followup... #2654904: Password fields should hash on set rather than save

[deleted - posted in wrong thread, sorry]