Xss filter() mangles image captions and title/alt/data attributes

Excellent find, and awesome bug report: all the info is here, explained very clearly. Thank you! :)

Whew, I fear this is going to be mighty difficult to fix. The cause is deep in the filter system: the filter_html uses Xss::filter(), which is responsible for stripping out evil protocols. If you type something (anything) before "http:", such as "You should see http://drupal.org", then ""You should see http:" is considered the protocol! Patch attached to fix this.

Tested with this content, and with the "restrict images to this site" filter off:

<p>Simple</p>
<img data-align="center" data-caption="http://drupal.org" src="https://drupal.org/sites/all/modules/drupalorg/drupalorg/images/d8.svg" />
<p>Anything preceding the URL is interpreted as the protocol</p>
<img data-align="center" data-caption="See http://drupal.org" src="https://drupal.org/sites/all/modules/drupalorg/drupalorg/images/d8.svg" />
<p>And when HTML is embedded in a data- attribute, that too is interpreted as the protocol</p>
<img data-align="center" data-caption="This is a &lt;a href=&quot;http://drupal.org&quot;&gt;quick&lt;/a&gt; test.." src="https://drupal.org/sites/all/modules/drupalorg/drupalorg/images/d8.svg" />

(Note that you have to copy/paste the above in CKEditor's "Source" mode, without exiting source mode; the current CKEditor build will mangle HTML stored in data- attributes. That's fixed in newer versions of CKEditor.)

Before:

After:

.

Of course. But before I do that, I want sign-off on the approach.

Some test cases with the patch applied and using Basic HTML:

The most recent patch only works when the scheme is preceded by a space. There are other valid captions where the scheme is not preceded by a space (see example 1). Rather than checking everything between space and ':', the logic should follow RFC 3986's definition of a scheme as "Scheme names consist of a sequence of characters beginning with a letter and followed by any combination of letters, digits, plus ("+"), period ("."), or hyphen ("-")"

However, I'm not sure how we can fix examples 2 and 3 while still using Url::stripDangerousProtocols.

Needs work for example 1, although we might have to rethink the approach to accommodate examples 2 and 3 too.

Thanks for the review — much appreciated!

And you're right, I didn't consider the (http://example.com) scenario, which of course must also work :/ Thanks for the pointers!

So I'm now testing with this HTML:

<p>Simple</p>
<img data-align="center" data-caption="http://drupal.org" src="/sites/default/files/inline-images/dark.png" />
<p>Anything preceding the URL is interpreted as the protocol</p>
<img data-align="center" data-caption="See http://drupal.org" src="/sites/default/files/inline-images/dark.png" />
<p>And when HTML is embedded in a data- attribute, that too is interpreted as the protocol</p>
<img data-align="center" data-caption="This is a &lt;a href=&quot;http://drupal.org&quot;&gt;quick&lt;/a&gt; test.." src="/sites/default/files/inline-images/dark.png" />
<p>#6.1:</p>
<img data-align="center" data-caption="(http://drupal.org)" src="/sites/default/files/inline-images/dark.png" />
<p>#6.2a:</p>
<img data-align="center" data-caption="Source:reuters" src="/sites/default/files/inline-images/dark.png" />
<p>#6.2b:</p>
<img data-align="center" data-caption="Source: reuters" src="/sites/default/files/inline-images/dark.png" />

For a moment, I thought it would've been possible to first run the filter_caption filter and then run the filter_html filter, but that won't work, because it'd require the <figure> and <figcaption> tags to be whitelisted. We don't want that.

The solution that I found is super simple and elegant: we simply leverage the FilterInterface::prepare() phase that filters may implement! By URL-encoding the value of the data-caption attribute, no protocol stripping happens anymore. The data-caption attribute is intended to contain (HTML-encoded) HTML, therefore filtering should be applied. (This already is the case.) And it is then that filtering that is responsible for stripping dangerous protocols.

This patch no longer touches the Xss or UrlHelper classes at all anymore, so the "needs security review" tag is not even necessary anymore. However, thanks to the test coverage added by #2099741: Protect WYSIWYG Editors from XSS Without Destroying User Data (Drupal\editor\Tests\EditorXssFilter\StandardTest), we can also be certain that we did not enable any new security holes! Hence I'm definitely able to remove the "needs security review" part.

I took the patch in #8 for quick test-drive, and there seems to still be an issue. To reproduce:

Creating/editing a node and adding a caption:

Viewing the result on node view:

Editing the node again:

How do you keep coming up with these edge cases? :D Amazing!

Now testing with:

<p>Simple</p>
<img data-align="center" data-caption="http://drupal.org" src="/sites/default/files/inline-images/dark.png" />
<p>Anything preceding the URL is interpreted as the protocol</p>
<img data-align="center" data-caption="See http://drupal.org" src="/sites/default/files/inline-images/dark.png" />
<p>And when HTML is embedded in a data- attribute, that too is interpreted as the protocol</p>
<img data-align="center" data-caption="This is a &lt;a href=&quot;http://drupal.org&quot;&gt;quick&lt;/a&gt; test.." src="/sites/default/files/inline-images/dark.png" />
<p>#6.1:</p>
<img data-align="center" data-caption="(http://drupal.org)" src="/sites/default/files/inline-images/dark.png" />
<p>#6.2a:</p>
<img data-align="center" data-caption="Source:reuters" src="/sites/default/files/inline-images/dark.png" />
<p>#6.2b:</p>
<img data-align="center" data-caption="Source: reuters" src="/sites/default/files/inline-images/dark.png" />
<p>#7:</p>
<img data-align="center" data-caption="Interesting (Scope resolution operator ::)" src="/sites/default/files/inline-images/dark.png" />

Upon further (and deep — spent my entire Saturday on this) investigation, it turns out that the solution in #8 indeed solves it for filtering (i.e. output for the end user), but it's broken because of #2099741: Protect WYSIWYG Editors from XSS Without Destroying User Data, which introduced EditorXssFilterInterface to ensure safety of authors when using WYSIWYG editors. That also uses
Xss::filter()… and therefore we end up having exactly the same problems as before #8, but only for the author, not for the end user!

If you'd paste the sample data, save it, edit it again, you'd notice that exactly the same things are broken as before #8, but only for authors!

In other words: the solution in #8 is not a complete solution. And since we have to fix it in Xss::filter() anyway, we might as well get rid of #8 entirely — but keep the added test coverage, of course.

By very carefully adapting UrlHelper::stripDangerousProtocols(), I've been able to find a solution, that:

1. still has all of the existing security test coverage still passing tests: \Drupal\editor\Tests\EditorXssFilter\StandardTest, \Drupal\Tests\Component\Utility\XssTest, \Drupal\filter\Tests\FilterUnitTest and \Drupal\Tests\Component\Utility\UrlHelperTest.
2. also fixes the known security flaws we had since PHP 5.4 due to an API change in PHP — see #1210798: In PHP 5.4+, html_entity_decode() doesn't decode invalid numeric entities (but that issue might still need to be resolved, it could have more symptoms than only the two test cases that were disabled because of it)
3. also ensures that if there are *multiple* protocols in a single attribute value (e.g. in the case of the encoded HTML for the data-caption attribute), they are all stripped. Note that we in fact committed \Drupal\editor\Tests\EditorXssFilter\StandardTest with a test case that shows that this is broken in HEAD — the mitigating factor is that you'd need the <META> tag to be allowed to use it.
4. does not support the Source:Wikipedia use case, because it's impossible for Drupal to know whether Source: is an actual protocol, now or in the future. That will be a minor WTF, but we must always err on the side of safety.

Sorry for stupid question, but I don't understand a few XSS protection filter. E.g. this test:

$data[] = array('<IMG SRC="jav&#x09;ascript:alert(\'XSS\');">', '<IMG src="jav' . "\t" . 'alert(&#039;XSS&#039;);">');

Why that string needs to be "protected"? I test (only with Chrome) and it does not do anything harmful.

There's a link just above that line of code that explains it. Please remember that XSSes don't need to work in every browser to be dangerous, if they only work in some browsers, that's already enough.

IMO that link say the contrary: it is used to protect against XSS, not to do an XSS attack. Because I don't trust 100% on any type of wiki, I tried to reproduce it in any browser, but no popup was trigger, so I raised the question.

Well, in any case: what you're talking about is completely off-topic for this issue. Please open a new issue if you want to remove that.

Here you are #2240365: Remove misunderstanding XSS tests.

+        // If the colon is followed by whitespace, then this cannot be a URI,
+        // per RFC3986, section 3 (Syntax Components).
+        // Example: the alt attribute of an image might contain the value
+        // "Source: Wikipedia".
+        if (preg_match('/\s/', $uri[$colonpos + 1])) {
+          break;
+        }

^ javascript: alert(123) works pretty well. Virtually every browser will automatically url-encode spaces in embedded URLs.

Formal validation is pretty useless in an XSS filter, precisely because browsers have very lenient parsers.

+        // First: ignore (but retain) leading whitespace. (There could be a
+        // leading space that would cause the URI scheme to be considered
+        // disallowed, which does not make sense.)
+        $parts = preg_split('/\s+/', $protocol);
+        $actual_protocol = array_pop($parts);

^ \s is possibly local specific, I'm not sure we want to go there.

+        $whitespace_prefix = substr($protocol, 0, strrpos($protocol, $actual_protocol));

^ This looks like a really slow version of substr($protocol, 0, -strlen($actual_protocol))

+        // If $protocol is empty after stripping leading whitespace, we cannot
+        // be dealing with an URI, and therefore also not an URI scheme.
+        // Example: the alt attribute of an image might contain the value
+        // "Scope resolution operator ::".
+        if (empty($protocol)) {
+          break;
+        }

^ "javascript\t:alert(123)" works pretty well as an XSS (at least in Chrome), so this assumption is just wrong with the split on \s.

         // If a colon is preceded by a slash, question mark or hash, it cannot
-        // possibly be part of the URL scheme. This must be a relative URL, which
+        // possibly be part of the URI scheme. This must be a relative URL, which
         // inherits the (safe) protocol of the base document.
-        if (preg_match('![/?#]!', $protocol)) {
+        if (in_array($protocol[0], array('/', '?', '#'))) {
           break;
         }

^ The new version is missing a lot of relative URLs, that do not need to start with either /, ? or #.

(For example: about?test:toto is a valid URL.)

+          $offset = $colonpos + 1 - strlen($protocol);

^ From the look of it, there is a off-by-one error here. You are stripping the protocol *and* the colon.

stripDangerousProtocols comes from the original KSES port ( https://api.drupal.org/api/drupal/modules%21filter.module/function/filte... ) and as such it should not be changed at all unless we have very very good reasons and a lot of expertise. I do not think it possible to enhance the KSES code but feel free to prove me wrong.

Closed #2327341: Xss::attributes() corrupts data attributes that contain colons or JSON-encoded data as a duplicate.

Also related: #2231709: Filter system clean-up: stop applying UrlHelper::stripDangerousProtocols()'s logic to *every* attribute value.

Can we instead think on which attributes are JS capable? I know it's dangerous too but perhaps it's less dangerous than trying this.

That could be a solution. Its clear that our attribute needs are a bit more complex then they where almost 9 years ago when the port of KSES was added. It is still doing its job exceedingly well, its just the requirements of complex attributes and more advanced browsers seems to have pushed us to require something a little more surgical.

So what we can do here; we can write a harmless attribute list based on https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes ; data-* surely belongs there. And be quite careful with it.

Damien, please review this -- and please noone else set to RTBC even if you like it.

@chx: See also #952964: String stripped from title and alt attribute if contains a colon which is pretty much doing the same thing, and was the solution I was heading towards as well.

This implies FilterHtml::getHTMLRestrictions() must also be updated.

Maybe we should continue this "new" direction either on #952964: String stripped from title and alt attribute if contains a colon or #2327341: Xss::attributes() corrupts data attributes that contain colons or JSON-encoded data?

Just mark everything as a duplicate and keep this one going?

I'm fine with this approach. Only one question: do we want this list of harmless attributes to be configurable or not? #952964: String stripped from title and alt attribute if contains a colon made it configurable.

(Especially considering Web Components are upcoming, I think we must allow for additional attributes to be considered harmless. And that list could vary per site and potentially even per text format.)

You put the wrong attribute there and you are hosed. Do we dare? I am already very very anxious to touch this code at all but to hand it over to users? Eek :)

Yes, we should make it configurable.

#27: I'm fine with deferring configurability to a follow-up if you want :) data-* attributes are the immediate need/problem/use case, everything else could be dismissed as speculation :)

I'm ok to making it configurable without an exposed UI in a follow-up.

I closed #2346989: [PP-1] 'Restrict images to this site' leads to incorrect HTML with language attribute as another duplicate of this.

(About one month later...)

I reviewed the approach from #23 and it sounds very reasonable to me.

The whitelist approach is recommended by several sources (for example the nice write-up by Jakob Kallin and Irene Lobo Valbuena at excess-xss.com). A few years back, a "common" list of sanitization rules was posted on the whatwg wiki. It has a list of "acceptable attributes" and a list of "attributes whose value is a URI", we probably want to consider in our whitelist only the difference between those two (+ everything that starts with data-), and run anything else through the filter.

That would be a first start. The other approach would be to just leverage an externally maintained filter? HTML purifier looks extremely relevant.

Another duplicate was opened: #2383205: Xss::filter removes valid HTML attributes . But one that has nice tests :)

Looks like we want to go with #23. What's missing in that case, are tests.

We want these tests that I wrote (in #10) for the caption filter in particular:

--- a/core/modules/filter/lib/Drupal/filter/Tests/FilterUnitTest.php
+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterUnitTest.php

But also tests for the XSS filtering changes themselves in #23. webflo's patch in #2383205 does just that.

1. +++ b/core/tests/Drupal/Tests/Component/Utility/XssTest.php
   @@ -486,6 +486,41 @@ public function testQuestionSign() {
   +   * Check that strings in html attributes are are correctly processed.
   

   s/html/HTML/

   Missing @covers.

   And finally, missing the test coverage I wrote for the caption filter, see my previous comment.

2. +++ b/core/tests/Drupal/Tests/Component/Utility/XssTest.php
   @@ -486,6 +486,41 @@ public function testQuestionSign() {
   +    if ($allowed_tags === NULL) {
   +      $value = Xss::filter($value);
   +    }
   

   This is never used. Let's delete it?

3. +++ b/core/tests/Drupal/Tests/Component/Utility/XssTest.php
   @@ -486,6 +486,41 @@ public function testQuestionSign() {
   +        'Image tag with alt and title attribute',
   

   s/alt and title/data-/

Addressed Wim's comments in #36. I've added tests from #10. Although, I've tried to fix the errors but there are still some errors in the filterCaption tests. Maybe someone can help me on how to fix it.

Fixed a few test failures.

• FilterCaption does not have a prepare() step, just process(), so removed calls to the FilterBase::prepare() since we are not calling those for FilterHtml either.
• Swapped String::decodeEntities() and String::check_plain for their non-deprecated counterparts.
• Fixed incorrect capitalisation of Xss in a use-statement.
• The tests were originally written in #10, before the special treatment of "data-"-attributes introduced in #22. I have updated the expected values in the three remaining failing tests accordingly.

Thank you very much for making this patch green again, @mr.baileys!

1. +++ b/core/modules/filter/src/Tests/FilterUnitTest.php
   @@ -241,7 +238,7 @@ function testCaptionFilter() {
        $input = '<img data-caption="This is an &lt;a href=&quot;javascript:alert(&quot;foo&quot;)&gt;evil&lt;/a&gt; test…" src="llama.jpg" />';
        $expected = '<figure><img src="llama.jpg" /><figcaption>This is an <a>evil</a> test…</figcaption></figure>';
        $this->assertIdentical($expected, $test_with_html_filter($input));
   

   This unchanged test code still ensures that no XSS can make it to the site's output.

2. +++ b/core/modules/filter/src/Tests/FilterUnitTest.php
   @@ -241,7 +238,7 @@ function testCaptionFilter() {
   -    $expected_xss_filtered = '<img data-caption="This is an &lt;a href=&quot;alert(&quot;foo&quot;)&gt;evil&lt;/a&gt; test…" src="llama.jpg" />';
   +    $expected_xss_filtered = '<img data-caption="This is an &lt;a href=&quot;javascript:alert(&quot;foo&quot;)&gt;evil&lt;/a&gt; test…" src="llama.jpg" />';
        $this->assertIdentical($expected_xss_filtered, $test_editor_xss_filter($input));
   

   This on the other hand… it now actually verifies that an XSS does make it all the way to the WYSIWYG editor. Which means that an XSS is possible in theory. CKEditor protects against this automatically, but another editor may not.

   A mitigating factor is that the editing user would actually have to click this link within the editor (which CKEditor also doesn't let you do) — he couldn't be automatically attacked. And note that this never makes it to the end user, see point 1.

   So that makes this less than ideal, but still better than what I proposed in #10 — see the comments by chx and Damien Tournoud.

   AFAICT it is literally impossible to not have this problem when going with the approach recommended by chx & Damien, since it uses a simple whitelist of tags. My code in #10 used to do what you could call "speculative URL parsing".

   The only way I see to fix this problem is to add a editorXssFilter() method to FilterInterface, to allow filters to perform additional XSS filtering for editors. That way, the FilterCaption filter could apply XSS filtering to the data-caption attribute's value.

3. +++ b/core/modules/filter/src/Plugin/Filter/FilterCaption.php
   @@ -9,7 +9,6 @@
   -use Drupal\Component\Utility\String;
   
   @@ -40,12 +39,12 @@ public function process($text, $langcode) {
   -        $caption = String::checkPlain($node->getAttribute('data-caption'));
   +        $caption = SafeMarkup::checkPlain($node->getAttribute('data-caption'));
   ...
   -        $caption = String::decodeEntities($caption);
   +        $caption = Html::decodeEntities($caption);
   

   These are deprecated, but changing them here is actually not necessary. It actually is distracting, because it suggests it's part of the solution for this issue.

Talked to @Wim Leers in irc, assigning to myself and will try to upload a new patch later today.

After talking to Wim Leers on irc, we decided it might be better to perform XSS filtering on all data-*-attributes globally rather than relying on filters implementing editorXssFilter() individually. Attached patch implements this global data-* attribute filtering.

These are deprecated, but changing them here is actually not necessary. It actually is distracting, because it suggests it's part of the solution for this issue.

You're right, I've changed them back.

1. +++ b/core/modules/editor/src/EditorXssFilter/Standard.php
   @@ -85,7 +89,30 @@ public static function filterXss($html, FilterFormatInterface $format, FilterFor
   +    // Since data-*-attributes can contain encoded markup that could be decoded
   

   s/data-*-/data-/, like everywhere else.

   s/markup/HTML markup/, to clarify, because "markup" in general does not need to be filtered, only HTML markup.

   Maybe also add an @see \Drupal\filter\Plugin\Filter\FilterCaption?

2. +++ b/core/modules/editor/src/EditorXssFilter/Standard.php
   @@ -89,7 +89,7 @@ public static function filterXss($html, FilterFormatInterface $format, FilterFor
   -    $output = Xss::filter($html, $blacklisted_tags);
   +    $output = parent::filter($html, $blacklisted_tags);
   

   Hah :)

3. +++ b/core/modules/editor/src/EditorXssFilter/Standard.php
   @@ -85,7 +89,30 @@ public static function filterXss($html, FilterFormatInterface $format, FilterFor
   +        $value = String::decodeEntities($node->value);
   +        $value = Xss::filterAdmin($value);
   +        $node->value = SafeMarkup::checkPlain($value);
   

   Add a comment for these 3 lines:

   // Decode, filter, encode.
   

4. +++ b/core/modules/filter/src/Plugin/Filter/FilterCaption.php
   @@ -39,12 +40,12 @@ public function process($text, $langcode) {
   -        $caption = SafeMarkup::checkPlain($node->getAttribute('data-caption'));
   +        $caption = String::checkPlain($node->getAttribute('data-caption'));
   ...
   -        $caption = Html::decodeEntities($caption);
   +        $caption = String::decodeEntities($caption);
   

   Let's omit these distracting, unnecessary changes.

5. +++ b/core/modules/filter/src/Tests/FilterUnitTest.php
   @@ -199,8 +199,11 @@ function testCaptionFilter() {
   +      $filtered_html_format = FilterFormat::create(array(
   

   s/array(…)/[…]/

6. +++ b/core/modules/filter/src/Tests/FilterUnitTest.php
   @@ -199,8 +199,11 @@ function testCaptionFilter() {
   +        'format' => 'filtered_html',
   +        'name' => 'Filtered HTML',
   

   Please use random names instead; this creates unnecessary assumptions.

   In fact, why not just omit this completely?

Thanks for the review, new patch attached.

Maybe also add an @see \Drupal\filter\Plugin\Filter\FilterCaption?

Not sure about this, since FilterCaption is just one of the potentially many filters for which we globally sanitize the data-attributes?

Straight reroll.

Small typo, trying again. Sorry.

+++ b/core/modules/editor/src/EditorXssFilter/Standard.php
@@ -85,7 +89,31 @@ public static function filterXss($html, FilterFormatInterface $format, FilterFor
+    // Since data-attributes can contain encoded HTML markup that could be
+    // decoded and interpreted by editors, we need to apply XSS filtering to
+    // their contents.
+    return Standard::filterXssDataAttributes($output);

This means we need to expand the test coverage in \Drupal\Tests\editor\Unit\EditorXssFilter\StandardTest.

After that, this is RTBC.

I started adding tests to \Drupal\Tests\editor\Unit\EditorXssFilter\StandardTest specifically for the attributing filtering.

There is one failing test because DOMAttr->value automatically decodes attribute values, while we want the original unescaped value.

Forgot to attach the interdiff to previous patch.

Going to review this.

Discussed with @pfrenssen; this is a critical (and upgrade path blocking because of the disclosed vulnerability).

As discussed with @xjm this is critical because it is an actual XSS issue which is easily exploitable.

This issue can be quite confusing to figure out. Here's what I've learned from experimenting with the patch, and talking to @mr.baileys and @Wim Leers:

• The image captions are saved in the data-caption attribute. It is possible to use HTML in captions, for example if you want to output a link. If you create a link using the button in the CKEditor interface this will be single escaped. If you want to show any HTML entities then these will need to be double escaped. Double escaped captions are inherently safe and should never be filtered.
• If the user manually enters any HTML code in the caption this will be interpreted by CKEditor. For example, if I enter <strong>my text</strong> and submit the form this will become bold text. If I enter <script>alert('xss');</script> then this will be executed as well, causing the XSS vulnerability. I suspect that this is a bug in CKEditor. Even if it would be intentional to allow editors to embed scripts in their captions it doesn't make much sense to execute the scripts when they are being edited since any executed scripts no longer have their code present in the editor and will be lost on a subsequent form submit.
• The only way we can prevent the XSS issue on our side is by stripping the dangerous code. Technically this is data loss (the <script> tags will be gone), but this is of course preferable over having an actual XSS issue.
• If you use the Full HTML text format then the XSS will still be present, even if the patch is applied. This is as designed, since the Full HTML format does allow to embed scripts, for example to display external assets.
• The patch disables filtering of bad protocols (eg. javascript:alert('xss')) in selected "harmless" attributes (data attributes + title and alt attributes). These attributes are fully text based and will never have their contents executed as javascript code. This made me wonder about the data-caption attribute. It turns out fine, since its contents are fully XSS-filtered by FilterCaption::process(). A potential attack with a bad protocol could be data-caption="<a href="javascript:alert(1);">test</a>" but if this is decoded and XSS-filtered the javascript: part is filtered correctly since it now is part of an href which is not on the whitelist of "harmless" attributes.

1. +++ b/core/modules/editor/src/EditorXssFilter/Standard.php
   @@ -7,7 +7,11 @@
   +use Drupal\Component\Utility\SafeMarkup;
    use Drupal\Component\Utility\Xss;
   +use Drupal\Component\Utility\Html;
   

   Order use statements alphabetically.

2. +++ b/core/modules/editor/src/EditorXssFilter/Standard.php
   @@ -7,7 +7,11 @@
   +use Drupal\Component\Utility\String;
   +use Drupal\Component\Utility\Unicode;
   

   These two namespaces are unused.

3. +++ b/core/modules/editor/src/EditorXssFilter/Standard.php
   @@ -85,7 +89,31 @@ public static function filterXss($html, FilterFormatInterface $format, FilterFor
   -    return static::filter($html, $blacklisted_tags);
   +    $output = parent::filter($html, $blacklisted_tags);
   

   I don't understand why it was changed from static:: to parent::. This has a different meaning when this class is extended. It would mean that if the filter() method is overridden in the called class then it would not be called in this case, but instead its parent would be called. I think this should be put back to static::.

4. +++ b/core/modules/editor/src/EditorXssFilter/Standard.php
   @@ -85,7 +89,31 @@ public static function filterXss($html, FilterFormatInterface $format, FilterFor
   +    // Since data-attributes can contain encoded HTML markup that could be
   +    // decoded and interpreted by editors, we need to apply XSS filtering to
   +    // their contents.
   +    return Standard::filterXssDataAttributes($output);
   

   Same here, it should be static::filterXssDataAttributes(), so that in case this class is overridden the method of the called class is used, and not specifically always the one from the Standard class.

5. +++ b/core/modules/editor/src/EditorXssFilter/Standard.php
   @@ -85,7 +89,31 @@ public static function filterXss($html, FilterFormatInterface $format, FilterFor
   +        // Decode, filter, encode. There is no need to explicitly decode
   +        // $node->value, since DOMAttr holds the decoded value.
   

   We are not decoding in this part, only filtering and encoding. As is already said $node->value is already decoded.

6. +++ b/core/modules/editor/tests/src/Unit/EditorXssFilter/StandardTest.php
   @@ -512,6 +512,12 @@ public function providerTestFilterXss() {
   +    // Test XSS filtering on data-attributes.
   +    // @see \Drupal\editor\EditorXssFilter::filterXssDataAttributes()
   +    $data[] = array('<img src="butterfly.jpg" data-caption="&lt;script&gt;alert();&lt;/script&gt;" />', '<img src="butterfly.jpg" data-caption="alert();" />');
   +    $data[] = array('<img src="butterfly.jpg" data-caption="&amp;lt;script&amp;gt;alert();&amp;lt;/script&amp;gt;" />', '<img src="butterfly.jpg" data-caption="&amp;lt;script&amp;gt;alert();&amp;lt;/script&amp;gt;" />');
   +    $data[] = array('<img src="butterfly.jpg" data-caption="&lt;EMBED SRC=&quot;http://ha.ckers.org/xss.swf&quot; AllowScriptAccess=&quot;always&quot;&gt;&lt;/EMBED&gt;" />', '<img src="butterfly.jpg" data-caption="" />');
   

   Maybe explain why it is needed to filter single escaped values but not double escaped values.

7. +++ b/core/modules/filter/src/Tests/FilterUnitTest.php
   @@ -8,6 +8,8 @@
    use Drupal\Component\Utility\Html;
   +use Drupal\filter\Entity\FilterFormat;
   +use Drupal\editor\EditorXssFilter\Standard;
    use Drupal\Component\Utility\SafeMarkup;
   

   Alphabetical ordering of use statements, my favourite pet coding standard :)

8. +++ b/core/modules/filter/src/Tests/FilterUnitTest.php
   @@ -176,6 +178,69 @@ function testCaptionFilter() {
   +    // So far we've tested that the caption filter works correctly. But we also
   +    // want to make sure that it works well in tandem with the "Limit allowed
   +    // HTML tags" filter, which is typically used with.
   

   "which is typically used with" is not a complete sentence. There should be an "it" in there somewhere.

9. +++ b/core/modules/filter/src/Tests/FilterUnitTest.php
   @@ -176,6 +178,69 @@ function testCaptionFilter() {
   +    $test_with_html_filter = function($input) use ($filter, $html_filter) {
   

   Anonymous functions should have a space between "function" and the opening parenthesis.

10. +++ b/core/modules/filter/src/Tests/FilterUnitTest.php
    @@ -176,6 +178,69 @@ function testCaptionFilter() {
    +    // All the tricky cases encountered at https://drupal.org/node/2105841.
    +    // A plain URL preceded by text.
    +    $input = '<img data-caption="See http://drupal.org" src="llama.jpg" />';
    +    $expected = '<figure><img src="llama.jpg" /><figcaption>See http://drupal.org</figcaption></figure>';
    +    $this->assertIdentical($expected, $test_with_html_filter($input));
    +    $this->assertIdentical($input, $test_editor_xss_filter($input));
    +    // An anchor.
    +    $input = '<img data-caption="This is a &lt;a href=&quot;http://drupal.org&quot;&gt;quick&lt;/a&gt; test…" src="llama.jpg" />';
    +    $expected = '<figure><img src="llama.jpg" /><figcaption>This is a <a href="http://drupal.org">quick</a> test…</figcaption></figure>';
    +    $this->assertIdentical($expected, $test_with_html_filter($input));
    +    $this->assertIdentical($input, $test_editor_xss_filter($input));
    +    // A plain URL surrounded by parentheses.
    +    $input = '<img data-caption="(http://drupal.org)" src="llama.jpg" />';
    +    $expected = '<figure><img src="llama.jpg" /><figcaption>(http://drupal.org)</figcaption></figure>';
    +    $this->assertIdentical($expected, $test_with_html_filter($input));
    +    $this->assertIdentical($input, $test_editor_xss_filter($input));
    

    ...

    This is very dense and hard to read, can we separate each case with an empty line?

11. +++ b/core/modules/filter/src/Tests/FilterUnitTest.php
    @@ -176,6 +178,69 @@ function testCaptionFilter() {
    +    // A source being credited, with a typo.
    +    $input = '<img data-caption="Source:Wikipedia" src="llama.jpg" />';
    +    $expected = '<figure><img src="llama.jpg" /><figcaption>Source:Wikipedia</figcaption></figure>';
    +    $this->assertIdentical($expected, $test_with_html_filter($input));
    +    $expected_xss_filtered = '<img data-caption="Source:Wikipedia" src="llama.jpg" />';
    +    $this->assertIdentical($expected_xss_filtered, $test_editor_xss_filter($input));
    

    What's the typo? The missing space after the double colon?

    Also it seems that $input and $expected_xss_filtered are identical, so $input can be used in the final assert.

Thanks for the review @pfrenssen, will address your feedback asap.

For the strangeness on the CKEditor side WRT encoding/decoding, we already have #2460145: Image captions incorrectly deal with plain-text HTML tags for that. Specifically see #2460145-3: Image captions incorrectly deal with plain-text HTML tags, which shows that it's a bug in CKEditor itself.

• 1, 2: Fixed
• 3, 4: Changed back to static. These were changed since tests were failing with static earlier, but I can no longer reproduce those test failures to figure out what exactly was happening (I remember it had something to do with the switch between tag white-listing and black-listing, and the incorrect value being used).
• 5: Re-worded the comment.
• 6: Added some additional comments to explain the test cases.
• 7: Fixed.
• 8: Fixed.
• 9: Fixed.
• 10: Newlines added between individual test-cases.
• 11: Reworded the comment, but the "typo" is indeed the missing space, since protocol-filtering used to consider "Source:" as a protocol, invoking the bad protocol filtering and mangling the caption.

Thanks a lot Ivo! Just some minor cleanup left and this is good to go.

1. +++ b/core/modules/editor/src/EditorXssFilter/Standard.php
   @@ -85,7 +87,33 @@ public static function filterXss($html, FilterFormatInterface $format, FilterFor
   +  /**
   +   * Applies a very permissive XSS/HTML filter to data-attributes.
   +   */
   +  protected static function filterXssDataAttributes($html) {
   

   Document the parameter and the return value.

2. +++ b/core/modules/filter/src/Tests/FilterUnitTest.php
   @@ -176,6 +178,75 @@ function testCaptionFilter() {
   +    $input = '<img data-caption="Source:Wikipedia" src="llama.jpg" />';
   +    $expected = '<figure><img src="llama.jpg" /><figcaption>Source:Wikipedia</figcaption></figure>';
   +    $this->assertIdentical($expected, $test_with_html_filter($input));
   +    $expected_xss_filtered = '<img data-caption="Source:Wikipedia" src="llama.jpg" />';
   +    $this->assertIdentical($expected_xss_filtered, $test_editor_xss_filter($input));
   

   $expected_xss_filtered is identical to $input, so we can get rid of $expected_xss_filtered and use $input in the final assert.

Addressed the comments from #69.

Looking good now, thanks a lot!

+++ b/core/lib/Drupal/Component/Utility/Xss.php
@@ -203,6 +204,10 @@ protected static function attributes($attributes) {
+            $harmless = substr($attribute_name, 0, 5) === 'data-' || in_array($attribute_name, array(
+              'title',
+              'alt',
+            ));

Let's add a comment as to why this is harmless.

Added a comment. I've also renamed the $harmless-variable to $skip_protocol_filtering, since that better describes what it does.

I have read though the interdiff which looks fine. Since we claim Drupal 8 to be HTML5 ready I would expect the @see link to point out at html5 doc not html4, but I could not find a proper one.
RTBC+1

Very good, I like the variable rename too. Thanks!

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Afaics @Damien Tournoud has done a security review and agreed that the current whitelist approach is the correct one to take. Committed 2febbff and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Component/Utility/Xss.php b/core/lib/Drupal/Component/Utility/Xss.php
index 5f8e981..9081fae 100644
--- a/core/lib/Drupal/Component/Utility/Xss.php
+++ b/core/lib/Drupal/Component/Utility/Xss.php
@@ -207,14 +207,12 @@ protected static function attributes($attributes) {
 
             // Values for attributes of type URI should be filtered for
             // potentially malicious protocols (for example, an href-attribute
-            // starting with "javascript:").
-            // However, for some non-URI attributes, performing this filtering
-            // actually causes valid and safe data to be mangled. To prevent
-            // this, we whitelist a number of non-URI attributes for which
-            // protocol filtering can be safely by-passed.
-            //
-            // @See \Drupal\Component\Utility\UrlHelper::filterBadProtocol()
-            // @See http://www.w3.org/TR/html4/index/attributes.html
+            // starting with "javascript:"). However, for some non-URI
+            // attributes performing this filtering causes valid and safe data
+            // to be mangled. We prevent this by skipping protocol filtering on
+            // such attributes.
+            // @see \Drupal\Component\Utility\UrlHelper::filterBadProtocol()
+            // @see http://www.w3.org/TR/html4/index/attributes.html
             $skip_protocol_filtering = substr($attribute_name, 0, 5) === 'data-' || in_array($attribute_name, array(
               'title',
               'alt',

I reworked the comment to be shorter and less repetitive.

Will this be backported to D7?

Amazing work on this very, very tricky issue, @mr.baileys! Thank you so much for sticking with it for one and a half years! (Your first comment on this issue was on November 6, 2013…)

Attached patch is a partial backport of the changes to filter_xss code in the D8 patch. The part filtering and encoding data attributes values has not been backported as this may break existing sites.

Updated patch to fix the comment.

Fix codestyle issue

Tagging to indicate this is a backport.

Up in #26 through #29, there was talk about having a follow-up to make the attribute whitelist configurable. Adding that here: #2544110: XSS attribute filtering is inconsistent and strips valid attributes

This is newly relevant based on #2501697: Remove SafeMarkup::set in rdf_preprocess_comment(), where we discovered that the XSS filter mangles valid RDF attribute name/value combinations, like rel="schema:author" and typeof="foaf:image".

Removing the upgrade path tag.

Expanded #85 to backport filterXssDataAttributes() as well so it could be used by D7 contrib projects, such as CKEditor. If this isn't needed then #85 should be good to go.

I found #85 to be a straight backport of #75 and didn't find any issues after a review or manual testing. This issue affects the D7 version of the Entity Embed module. #2550669: Backport field formatter display capabilities

Marking (#90) as RTBC, looks like a pretty straight-forward backport of the current D8 filtering.

#90 looks like it won't work on PHP 5.2?

Seems like #85 is the most important part to backport, but the extra function in #90 could be useful also (although I haven't read up on the details).

In the meantime, just changing the issue title, since it's pretty scary-looking and confusing otherwise :)

To address #93, I set up an environment with PHP 5.2.17 and ran the filter tests, with #90 applied, which all passed without any warnings.

I don't see how since namespaces weren't introduced until PHP 5.3. And if I try this at http://sandbox.onlinephpfunctions.com using PHP 5.2 I get a PHP warning due to the slash in "\DOMXPath" (Unexpected character in input: '\').

Also if we're going to introduce this new function then I think we need more documentation on how and when to use it. It is not used at all in Drupal 7 core, and it's not really clear from the current documentation what you would use it for.

+          // @see filter_xss_bad_protocol()
+          // @see http://www.w3.org/TR/html4/index/attributes.html

(minor) These should be part of a sentence since they're inline code comments rather than PHPDoc, e.g. See filter_xss_bad_protocol() and http://www.w3.org/TR/html4/index/attributes.html.

Otherwise it looked good to me.

Also demoting the priority here, since the part of this issue that affects Drupal 7 is not a critical bug. Would still be very nice to fix it though.

An updated version of #90 to address #93 and #95.

Tried it out & it worked great for me in D-7.41.

Ran the testing suite. 1 test failed -- "HTML scheme clearing evasion -- spaces and metacharacters before scheme." I don't think that's related to this fix, though.

I'm not sure how filter_xss_data_attributes() is really needed, though since it is currently only used for the tests.
As I understand it we're supposed to run data through the filters that are set up (Full, Filtered, Plain, etc.), so there'd be no need to call filter_xss_data_attributes() separately.

Otherwise, I'd say that its good to go.

aDarkling witch patch did you use?
And if it is #90 dit you try it with PHP 5.2?

Sorry, no.
I used #97 on PHP Version 5.4.38.
Just (manually) re-applied the patch to D-7.42. Still working.

This patch is already being used by (at least) the 2,424 sites that are using the Entity Embed module.

RTBC + 1, it looks good to me.

Marking for commit.

I thought about this issue a little more and I'm not sure the data- part of it is safe in terms of backwards compatibility.

The problem is that data- attributes are by definition arbitrary, so it's certainly possible for a URL to be stored in them intentionally (which is later moved into an actual href attribute by JavaScript). And I've definitely seen modules store URLs in data- attributes before.

For a simple example, if your JavaScript code does $('.some-link').attr('href', $('.some-source').attr('data-url')) that's safe against XSS currently as long as the source has been run through filter_xss(), but not safe with this patch. So this patch would potentially open up a security hole in existing code in that case.

Seems like we might need some kind of whitelist here, where a module can declare that a particular data- attribute shouldn't undergo this filtering? (This could be as simple as a single binary flag that is applied sitewide, e.g. a list of whitelisted data- attributes which modules could set via variable_set() and which filter_xss() then checks... as long as we're willing to assume that modules only use this on attributes that they can reasonably believe they themselves "control" and that other JavaScript code out there won't be using also.)

Some other minor issues I would have fixed on commit if not for the above:

+    // Check that strings in HTML attributes are are correctly processed.

"are are"

+ * Contrib modules which allow rich text fields to be edited using client-side
+ * WYSIWYG editors must apply XSS filtering to the contents of data-attributes
+ * since they can contain encoded HTML markup that could be decoded and
+ * interpreted by editors.

Should be "Contributed modules". And maybe this could use a specific explanation that contrib modules are expected to call this function themselves in those cases, and how to call it? (e.g., filter_xss_data_attributes(filter_xss($html))... specifically that this function by itself is not a substitute for regular XSS-filtering)

Swap media tags to the right one.

Re-roll of the patch from #97 which is the latest D7 patch in this issue.

Please create a Drupal 7 issue, so we can finally close this against Drupal 8. This was committed in April 2015, almost two years ago!

In 8.2.x, I'm still seeing this issue. I'm posting here in case someone can point me at where I'm going wrong.

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.

@Jaypan,

Could you repost your typeof issue in #2635632: Drupal XSS filtering cuts valid attributes, make it configurable / use a blacklist?

Done. Thanks.

@Wim Leers I've created an issue #2859006 which I just closed as a duplicate of this issue. If that would be the appropriate 7.x issue, then it's available for that.

I think that this issue is probably a duplicate of #2544110: XSS attribute filtering is inconsistent and strips valid attributes (or vice versa)? I just commented there as well and added this one as related there.

See 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.

In case we are going to mark this issue as a duplicate of #2635632: Drupal XSS filtering cuts valid attributes, make it configurable / use a blacklist, I just made a test-only patch on comment #22 there that incorporates (I think) the test cases here.

Oh, I see. This was an 8.x issue and was apparently fixed. Someone finally created the 7.x version of it in #123. This one should be marked Fixed / 8.0.x. It was closed several years ago and reopened around comment #83 for 7.x, but now our procedure is to have separate 8.x and 7.x issues. Hence... fixed.

Adding a link to #2859006: Image alternative text loses text preceding colon upon leaving plain-text editor or upon saving node as mentioned in #123... also made it "related"