Drupal XSS filtering cuts valid attributes, make it configurable / use a blacklist

Does this suggest that we should turn the logic around and instead of listing which attributes to skip - list which attributes we should protocol filter?

Like couldn't we just to protocol filtering if the attribute is one of:

• href
• src
• cite
• action
• longdesc
• xlink:href
• xml:base

From: https://wiki.whatwg.org/wiki/Sanitization_rules#Attributes_whose_value_i...

My list is incomplete... http://stackoverflow.com/questions/2725156/complete-list-of-html-tag-att...

This is extremely iffy -- HTML tag soup is still largely supported. So my recommendation would be: keep the current one but provide a setting where you can switch to a blacklist approach and then someone needs to write a little script to parse the attributes out of http://stackoverflow.com/a/2725168/308851 and use that as the blacklist. I do NOT want to ship the blacklist as default. But, it's just me. I am neither a maintainer nor a security team member :)

I was just going to open a new issue when I found this one. I discovered a similar problem when trying to create responsive images by a controller response. The problem is that the XSS filter crops any property containing a colon.
Example:
<source srcset="image.jpg 1x" media="all and (min-width: 560px) and (max-width: 850px)" type="image/jpeg"/>
becomes:
<source srcset="image.jpg 1x" media=" 850px)" type="image/jpeg"/>
and as a result the responsive images don't work as expected.

i thought that maybe we could add { and space to the list of characters in a protocol to say if it contains these then it is not a valid protocol but at the moment we're missing test coverage - see #1210798: In PHP 5.4+, html_entity_decode() doesn't decode invalid numeric entities - so I added { and [ but that still didn't fix the Angular case because it just calling UrlHelper::filterBadProtocol() mangles text.

Therefore after thinking about it I think we should just do the simple thing and add ng- and media to list of things we skip and be prepared to add to this list for the time being.

rerolled the patch

we should just do the simple thing and add ng- and media to list of things we skip and be prepared to add to this list for the time being.

@catch marked #2372127: Prevent Xss::filter from stripping a token at the beginning of an html attribute a duplicate of this issue, but then this needs real work (apart from fixing the broken reroll in #13) and won't be simple anymore...

As requested in https://www.drupal.org/node/2105841#comment-12016390, I'm posting an issue I've found with stripping of properties

I've got a field on my account, named bio. In this field, I'm manually adding the following:

<a href="https://www.jaypan.com/" typeof="schema:Organization>Jaypan</a>

However, the resulting HTML is as follows:

<a href="https://www.jaypan.com/" typeof="Organization>Jaypan</a>

So schema: is being stripped from the 'typeof' property.

I found that #69 in https://www.drupal.org/node/2544110 fixed the problem, even though one hunk of the patch failed.

I found that #69 in https://www.drupal.org/node/2544110 fixed the problem, even though one hunk of the patch failed.

Thanks for mentioning #2544110: XSS attribute filtering is inconsistent and strips valid attributes!

There are quite a few related issues here, all reporting problems that boil down to the Xss::attributes() method being too aggressive about colons, leading to mangled attributes:

(a) This issue.

Token-related:
(b) #2372127: Prevent Xss::filter from stripping a token at the beginning of an html attribute -- was reported a while back, and was already closed by @catch as a duplicate of this issue.
(c) #2944173: CKEditor mangles tokens in URLs, due to bug in Xss::attributes() -- was reported recently as a CKEditor bug, but it turned out to be due to Xss::attributes. It has a small test-only patch that illustrates the problem. EDIT: added note -- (c) is currently marked as Critical, because it leads to data loss merely by re-editing and saving a node with a token in an attribute in the body (such as href on an a tag), if the site uses CKEditor for editing that body.

Other valid attribute data being mangled:
(d) #2105841: Xss filter() mangles image captions and title/alt/data attributes
(e) #2544110: XSS attribute filtering is inconsistent and strips valid attributes
I believe that (d) and (e) are direct duplicates of each other (and just commented on the two of them to that effect). They are both asking for certain attributes containing colons to be left alone. Both have patches and tests. I have tested the latest patch in (e) and it doesn't solve (c); neither seems to be trying to address tokens.

Invalid attributes being mangled instead of stripped:
(f) #2552837: XSS::filter and filter_xss can create malformed attributes when you would expect them to be stripped
This issue is asking for certain invalid attributes to be stripped out instead of mangled, so it is a bit different from the others.

So... The problem seems to be fairly varied and widespread. I don't know if we want to try to solve it all here or in several interrelated issues or what?

I've made a test-only patch, with the test additions that were on the patch from #13 here, plus some new cases involving tokens. I ran the test locally, and it failed in the right way to illustrate the problems that I think this issue is trying to solve, assuming that it incorporates both the examples in the current issue summary, and the other issue(s) about tokens (one of which was already marked as a duplicate of this issue).

I don't have a good idea how to resolve this issue, but I don't think a configuration option or blacklist/whitelist will help, since these problematic attributes could be in any tag, in any attribute. Xss::attributes, in my opinion, just needs to be less aggressive and more careful about : characters in tokens.

And in case the scope is widened to include #2544110: XSS attribute filtering is inconsistent and strips valid attributes and/or #2105841: Xss filter() mangles image captions and title/alt/data attributes, here is a bigger test-only patch that includes examples from those issues as well (just basically copied from those two issues; I am not sure they are all valid...). And an interdiff from the previous test-only patch in #21.

@alexpott and/or @catch -- If you would like to make a decision about which (if any) of the issues mentioned in comment #20 you would like to consider as duplicates (i.e., part of this issue), I will volunteer to:

a) Update the issue summary here to define the scope and/or examples that should be covered.
b) Update the test-only patch to cover only those examples, as needed.
c) Mark the other issues as duplicates of this one.
d) Make a list of people who worked on those other issues that should be credited on this one.

Meanwhile, a few extra lines got into the end of that last patch. Here's a fix. Just deleted the lines like

+      [
+        'img tag with colon in data-caption attribute',
+        ['img'],
+      ],

My mistake: item (d) in comment #20 was an 8.0.x issue that was fixed, then it was moved to 7.x, but now there is a separate 7.x issue and so the 8.x one is closed, that's #2105841: Xss filter() mangles image captions and title/alt/data attributes. The others in #20 remain at large...

+1

I've described my case with Tailwind CSS here: #3252587: Extend the $skip_protocol_filtering list of attributes to use Tailwind CSS classes with prefix ":". Seems it's a bit related.

Closing as a duplicate of https://www.drupal.org/project/drupal/issues/3252587. From the description appears ":" was being stripped.

If you feel the issue is still there please reopen with the steps you are using.

Reopening to include the tests from this ticket.

Added reroll of patch #40 on Drupal 9.5.x., still needs work for #39.

Reading the last few comments I was about to change this to a task. Then I looked at the failing tests and it is failing on the tests being added here. So it appears that there are other changes to make as well, so leaving as a bug report.

Added issue summary template and tagging for an update.

Or does it make sense to move this work into #2544110: XSS attribute filtering is inconsistent and strips valid attributes and make this a duplicate of that?

@quietone personally I prefer to consolidate. There are a number of tickets open about updating the filter.

@quietone following up on #42

Closing out as work is being focused on #2544110: XSS attribute filtering is inconsistent and strips valid attributes which I believe covers this ticket. Will move over credit.

If anyone feel this is a separate ticket and wants to address the IS update please reopen.