Problem/Motivation

Xss::filter() calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by refactoring the code.
  • If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.

Remaining tasks

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
  2. Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.

Manual testing steps (for XSS and double escaping)

Not necessary, we are only changing documentation.

Do these steps both with HEAD and with the patch applied:

  1. Clean install of Drupal 8.
  2. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
  3. If there is any user or calling code input in the string, submit
    alert('XSS');

    and ensure that it is sanitized.

User interface changes

N/A

API changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Issue summary: View changes

Working on this at NHDevDays 2.

star-szr’s picture

Status: Active » Needs review
FileSize
676 bytes

Status: Needs review » Needs work

The last submitted patch, 2: document-2501403-2.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
683 bytes

Real time feedback from @cwells, new patch.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Sure, this is fair

xjm’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Component/Utility/Xss.php
@@ -79,6 +79,8 @@ public static function filter($string, $html_tags = array('a', 'em', 'strong', '
+    // Calling SafeMarkup::set() is acceptable here because all known XSS
+    // vectors will have been filtered out by now.

So, technically, if someone passes <script> in the $html_tags when they call this, then the XSS has been filtered. I think this is actually more like #2273925-256: Ensure #markup is XSS escaped in Renderer::doRender(). Also, I think we should emphasize why calling SafeMarkup::set() is correct rather than just acceptable.

Also, this is minor, but the regex is still doing some of the sanitization, so "by now" is slightly misleading I think.

So something like:

Strip any tags that are not in the whitelist, then mark the text as safe for output. All other known XSS vectors have been filtered out by this point, so it is acceptable to call SafeMarkup::set() on the resultant string.

Or something along those lines. Try to improve it from that probably. :)

xjm’s picture

Or something like:

...All other known XSS vectors have been filtered out by this point and any HTML tags remaining will have been deliberately allowed, so it is acceptable to call SafeMarkup::set() on the resultant string.

star-szr’s picture

Status: Needs work » Needs review
FileSize
837 bytes
964 bytes

Thanks @xjm, I added some more to your suggestion in #7.

Status: Needs review » Needs work

The last submitted patch, 8: document-2501403-8.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
875 bytes
1.13 KB

Running out of brain steam ;)

pwolanin’s picture

Looks ok to me, though we should make it harder for people to allow stupid things like SCRIPT, IFRAME, or OBJECT in the UI

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Perfect, very thorough.

@pwolanin Maybe you could make a follow up but I believe we should let people shoot themselves in the foot sometimes and not mollycoddle them.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Great, thanks @joelpittet and @pwolanin!

The use is internal to Drupal's sanitization APIs, and we've had a member of the sec team check over this patch, so I think this is good as one of the "Document" issues. We could possibly use a followup about Xss more generally for #11.

This issue only changes documentation, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. It's also part of resolving a critical issue. Committed and pushed to 8.0.x.

  • xjm committed 0834487 on 8.0.x
    Issue #2501403 by Cottser, xjm, pwolanin, joelpittet: Document...
David_Rothstein’s picture

Looks ok to me, though we should make it harder for people to allow stupid things like SCRIPT, IFRAME, or OBJECT in the UI
....
@pwolanin Maybe you could make a follow up but I believe we should let people shoot themselves in the foot sometimes and not mollycoddle them.

See #275811: Warn about potentially insecure filter configurations

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.