Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.This was originally reported by alexpott to the Drupal Security Team, but is being made public since there is no actual vulnerability and it can be treated as a public bug.
There is no vulnerability since "on" attribute names are renamed which renders them harmless.
Problem/Motivation
Drupal\Component\Utility\Xss::filter has the following behaviour:
HEAD
BEFORE: <IMG SRC= onmouseover="alert('xxs')"
AFTER: <IMG nmouseover="alert('xxs')">
With patch
BEFORE: <IMG SRC= onmouseover="alert('xxs')"
AFTER: <IMG>
You can see the bug by running the test code below using "drush scr xss.php.txt"
<?php
$strings = [
'<IMG SRC= onmouseover="alert(\'xxs\')"',
'<IMG onmouseover="alert(\'xxs\')"',
'<img src="http://example.com/foo.jpg" title="Example: title" alt="Example: alt">',
];
foreach ($strings as $original) {
$string = filter_xss($original, array('img'));
print "BEFORE: $original\nAFTER: $string\n\n";
}
Proposed resolution
Fix the logic so malformed attributes are stripped
Remaining tasks
review patch, backport.
User interface changes
n/a
API changes
n/a
Data model changes
n/a
| Comment | File | Size | Author |
|---|---|---|---|
| #31 | 2552837-31.patch | 3.72 KB | smustgrave |
| #31 | interdiff-28-31.txt | 743 bytes | smustgrave |
| #31 | 2552837-31-tests-only.patch | 1.79 KB | smustgrave |
Comments
Comment #2
pwolanin commentedComment #7
pwolanin commentedComment #8
alexpottDiscussed with @catch, @xjm, @effulgentsia, @cilefen, and @Cottser. We agreed that this is a major bug because it is unexpected behaviour from a key part of the way that Drupal secures itself against XSS attack.
Comment #9
alexpottCreditting myself since I reported it to security.drupal.org and I think provided the patch and test.
Comment #10
pwolanin commented@alexpott - yes, looks like you wrote the original fix, and I added some tests and moved it over here.
Comment #11
pwolanin commentedHere's a re-roll for 8.3.x
Comment #13
Munavijayalakshmi commentedRerolled the patch.
Comment #17
jhodgdonSee also #2635632-20: Drupal XSS filtering cuts valid attributes, make it configurable / use a blacklist, where I have tried to summarize all the mangled attribute issues and ask whether we should solve them together or apart.
Comment #18
alexpott@jhodgdon this is a straight-up bug fix which sets it apart from those other issues.
I think the test fail in #13 is unrelated setting back to needs review.
Comment #27
amber himes matzTriaged as part of Bug Smash. Sounds like this is a separate issue from the other XSS filter issues (some of which will be merged), so keeping open.
The patch has a test, but I think it also needs a failing test to prove the bug?
I think this will probably need both a 8.9 and 9.5 patch.
At the very least, it needs a re-roll to 9.5.x and a code review.
Comment #28
smustgrave commentedHiding the previous patches.
Rerolled #13 after verifying the test fails without the fix.
Only thing I didn't include was the break in the case2. Thought all switch cases needed to have a break?
Uploading a tests-only patch
Comment #31
smustgrave commentedFixed the test case. It was invalid now with this fix.
Comment #33
smustgrave commentedTrying to keep to the front of the issue queue. Don’t want to get lost in the shuffle
Comment #34
cilefen commentedI think this needs extra security scrutiny.
Comment #35
lendudeThis change seems to make us lose the test coverage for the onmouseover => nmouseover functionality (quick scan didn't reveal any other coverage for this, but could very well have missed something).
Are we ok with that? (my first guess would be 'no', but didn't look to deeply into what is doing this renaming)
Comment #36
smustgrave commentedSo this is removing the onmouseover from the src of the image.
There's so many tickets trying to update this filter but they have each a unique case. So really they should happen in an order
Comment #38
lendude@smustgrave ah yes, I misread, it is indeed good that the old test case is removed
Not sure how to get 'Needs security review' to happen, so moving to RTBC, which might help get that done if indeed needed.
Comment #39
alexpottI wrote this patch sometime back in 2015 :) and it has had tests ever since. I think we should trust our test coverage of the Xss class and we can see that this results in better and more correct HTML. I've reviewed the code again. I've not looked at it for years and I still believe that this is the correct fix.
The removal of the
break;is neither here nor there. It's the last statement in a switch - the break does nothing.Comment #43
catchThe change here looks good -
just completely removing the attribute instead of leaving an invalid one.edit: via @alexpott for correctness: "no longer creates invalid attributes and now filters following attribute/values the same as if src had had a value"
Untagging for security review because we already have extensive test coverage and this only affects the one test case that was validating the bug (bug better than XSS though of course).
Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!