user_pass() sets #default_value = $_GET[name] without checking the type.

catch → credited alexpott → .

catch → credited donquixote → .

Should this target 11.x, instead of 7.x, because of this?

This is present in Drupal 8 too in \Drupal\user\Form\UserPasswordForm::buildForm().

Re #6 I think this is no longer an issue in 10.x/11.x due to:

https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/HttpFo... e.g. a BadRequestException is already thrown by Symfony if the value retrieved in ::get() isn't a string.

Thanks for adding the test and working on this. Also thanks for the confirmation about 10.x/11.x.

The warning Warning: trim() expects parameter 1 to be string, array given in user_pass_validate() (line 69 of ***/modules/user/user.pages.inc). from the IS was fixed by #2975433: Notices/warnings from attempted exploitation of SA-CORE-2018-002 and SA-CORE-2018-004 → .

Therefore I think it should be sufficient to protect the #default_value and allow only scalar types. I am not sure if there is any notable benefit of the additional str_replace:

str_replace(array("\r", "\n"), '', (string) $_GET['name']);

What if there will be a newline in the string? Will that cause any problems? If not, I think we can simplify the patch to just check for is_scalar (without the str_replace call). Any thoughts? Otherwise the hardening looks good to me.

Yeah I'm not sure we need the replacement of the newline characters?

Along the lines of @alexpott's comment (which is now outdated for D10/11, IIUC) are there any other form fields we might want to add this to as well?

I'm thinking particularly of those that are typically exposed to anon users like the (rest of the) login form, perhaps search etc..?

I think that the answer to the question may be what @donquixote wrote in the private issue:

The '#default_value' is usually coming from a somewhat trusted source, where we know the type.
So I think the (unwritten) rule is that whoever sets #default_value is responsible for the value.

This case is special because the #default_value is set with a value that could be anything (anything that can come from $_GET, which means at least any string or array). This is why I think a "piecemeal" or one-instance fix is the correct thing to do here.

Currently the #value_callback, or automatic value callback like form_type_textfield_value() in D7, is responsible for sanitizing form data from $form_state['input'] before it goes into #value.
The #default_value is not sanitized automatically in this way, it is taken as-is and written into #value if there is no form input.

So I think the difference between this field and other inputs (that can be only manually filled) is that this one can be prefilled from URL and the value is written to the "mostly trusted" #default_value. But yes, looking at the fix from #2975433: Notices/warnings from attempted exploitation of SA-CORE-2018-002 and SA-CORE-2018-004 → , I am thinking, whether we even need this fix? Because the main reason was to sanitize $form_state['values']['name'] and that is already done by the is_scalar() right in the beginning of user_pass_validate(). On the other hand yes, you can alter the form and access the value sooner, than the default validate callback runs (but that is also questionable, if core does the sanitization, if we should sanitize it even in case contrib/custom code unsets or delays our validation).

Yeah, I think you're right that we've already addressed the problem described in the IS:

Warning: trim() expects parameter 1 to be string, array given in user_pass_validate() (line 69 of ***/modules/user/user.pages.inc).

It doesn't look like that could happen any more; core's checking is_scalar pretty early on in the form processing.

This was not the case when the (private) issue was originally filed.

I think we can close this as outdated, but thanks everyone that's worked on it.