Currently we get constant PHP trim warnings because of automated attack scripts trying the SA-CORE-2018-002 vulnerability. The warning is
Warning: trim() expects parameter 1 to be string, array given in Drupal\Core\Render\Element\Email::validateEmail() (line 73 of /var/www/drupal/core/lib/Drupal/Core/Render/Element/Email.php) #0 /var/www/drupal/core/includes/bootstrap.inc(582): _drupal_error_handler_real(2, 'trim() expects ...', '/opt/rh/httpd24...', 73, Array) #1 [internal function]: _drupal_error_handler(2, 'trim() expects ...', '/opt/rh/httpd24...', 73, Array) #2 /var/www/drupal/core/lib/Drupal/Core/Render/Element/Email.php(73): trim(Array) #3 [internal function]: Drupal\Core\Render\Element\Email::validateEmail(Array, Object(Drupal\Core\Form\FormState), Array) #4 /var/www/drupal/core/lib/Drupal/Core/Form/FormValidator.php(283): call_user_func_array(Array, Array) #5 /var/www/drupal/core/lib/Drupal/Core/Form/FormValidator.php(239): Drupal\Core\Form\FormValidator->doValidateForm(Array, Object(Drupal\Core\Form\FormState)) #6 /var/www/drupal/core/lib/Drupal/Core/Form/FormValidator.php(239): Drupal\Core\Form\FormValidator->doValidateForm(Array, Object(Drupal\Core\Form\FormState)) #7 /var/www/drupal/core/lib/Drupal/Core/Form/FormValidator.php(119): Drupal\Core\Form\FormValidator->doValidateForm(Array, Object(Drupal\Core\Form\FormState), 'user_register_f...') #8 /var/www/drupal/core/lib/Drupal/Core/Form/FormBuilder.php(571): Drupal\Core\Form\FormValidator->validateForm('user_register_f...', Array, Object(Drupal\Core\Form\FormState)) #9 /var/www/drupal/core/lib/Drupal/Core/Form/FormBuilder.php(314): Drupal\Core\Form\FormBuilder->processForm('user_register_f...', Array, Object(Drupal\Core\Form\FormState)) #10 /var/www/drupal/core/lib/Drupal/Core/Controller/FormController.php(74): Drupal\Core\Form\FormBuilder->buildForm(Object(Drupal\user\RegisterForm), Object(Drupal\Core\Form\FormState)) #11 [internal function]: Drupal\Core\Controller\FormController->getContentResult(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\RouteMatch)) #12 /var/www/drupal/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Array, Array) #13 /var/www/drupal/core/lib/Drupal/Core/Render/Renderer.php(582): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() #14 /var/www/drupal/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure)) #15 /var/www/drupal/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) #16 [internal function]: Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() #17 /var/www/drupal/vendor/symfony/http-kernel/HttpKernel.php(151): call_user_func_array(Object(Closure), Array) #18 /var/www/drupal/vendor/symfony/http-kernel/HttpKernel.php(68): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1) #19 /var/www/drupal/core/lib/Drupal/Core/StackMiddleware/Session.php(57): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #20 /var/www/drupal/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #21 /var/www/drupal/core/modules/page_cache/src/StackMiddleware/PageCache.php(99): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #22 /var/www/drupal/core/modules/page_cache/src/StackMiddleware/PageCache.php(78): Drupal\page_cache\StackMiddleware\PageCache->pass(Object(Symfony\Component\HttpFoundation\Request), 1, true) #23 /var/www/drupal/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #24 /var/www/drupal/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(50): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #25 /var/www/drupal/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #26 /var/www/drupal/core/lib/Drupal/Core/DrupalKernel.php(664): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #27 /var/www/drupal/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request)) #28 {main}.
The attack scripts are trying to pass in an array into the trim($element['#value']).
Attached is a patch for the Drupal\Core\Render\Element\Email class.
| Comment | File | Size | Author |
|---|---|---|---|
| #27 | interdiff-22-27.txt | 605 bytes | rassoni |
| #27 | 2973871-27.patch | 3.36 KB | rassoni |
| #26 | interdiff-22-26.txt | 2.36 KB | rassoni |
| #26 | 2973871-26.patch | 854 bytes | rassoni |
| #22 | reroll_diff_email-validateEmail-patch-only-2_2973871-22.txt | 2.6 KB | yogeshmpawar |
Comments
Comment #2
dkarso commentedComment #3
dkarso commentedComment #4
dkarso commentedComment #5
cilefen commentedCan you modify a test to cover this? It actually seems difficult because if you try passing an array in a test such as Drupal\Tests\system\Functional\Form\EmailTest, dom-crawler doesn't like that for the same reason. Perhaps there could be a better place for that test. Also, it seems it would be an improvement to block this kind of activity for any string type field.
Comment #6
dkarso commentedIt seems there was no unit test for the Email render element. There were unit tests for Password and Textfield. I added an extra unit test for the Email render element in the second patch. The test catches the array notice error without the first patch. With the email patch everything looks ok.
Regarding your improvement suggestion: Drupal\Core\Render\Element\Textfield::valueCallback() already checks if the input is a scalar.
Comment #8
dkarso commentedComment #9
cilefen commentedPlease add interdiffs when making changes. I have attached it. Could you please post the tests-only patch to prove it fails for the expected reasons, and not for t() being out of scope? Usually, in one comment you can upload first the tests-only patch, then the complete patch. The tests-only patch will fail but the full patch will pass. Thank you.
Comment #10
dkarso commentedTest only patch and patch only patch added.
Comment #12
dkarso commentedAs expected; the test-only patch catches the trim warning. The complete patch at #8 should do the trick.
Comment #13
dkarso commentedNew complete patch file added and new test only diff with interdiffs. I had the test in the wrong group and missed a space character.
Comment #14
eric_a commentedI wonder if this would be better fixed in the form element type value callback by casting to string...
I scanned the test. Does the error count matter? If not, FormState::hasAnyErrors() would benefit clarity?
The last test-only is green... It contains both a test and a fix. It's identical to email-validateEmail-patch-only-2.diff. (Also, the interdiffs look pretty broken... @dkarso: https://www.drupal.org/documentation/git/interdiff)
Comment #20
super_romeo commentedStill in 9.3.10.
Comment #21
larowlanComment #22
yogeshmpawarUpdated patch will reroll diff.
Comment #26
rassoni commented#22 patch applied successfully on d10. Fixed Test case failing .
Comment #27
rassoni commented#26missed file in patch.#22 patch applied successfully on d10. Fixed Test case failing .
Comment #28
smustgrave commentedRan the tests locally without the fix and they all pass without issue. So they will need to be updated.
Also the issue summary probably could be updated with steps to reproduce, proposed solution, etc.
Comment #30
smustgrave commented#28 should of been moved to new status.
Comment #31
smustgrave commentedwanted to bump 1 more time before closing.
Comment #33
smustgrave commentedSince there's been no follow up and summary is pretty incomplete (missing steps) going to close out. But it can always be re-open if happening in D11+, if so please update the summary using the standard template