Contrib.social

XSS::filter and filter_xss can create malformed attributes when you would expect them to be stripped

Open on Drupal.org →
Created on 17 August 2015, about 9 years ago
Updated 8 March 2023, over 1 year ago

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(&#039;xxs&#039;)">

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

🐛 Bug report
Status

Fixed

Version

9.5

Component
Base  →

Last updated 32 minutes ago

Created by

🇺🇸United States pwolanin

Live updates comments and jobs are added and updated live.
  • Triaged core major

    There is consensus among core maintainers that this is a major issue. Only core committers should add this tag.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment over 1 year ago →
    🇳🇱Netherlands Lendude Amsterdam

    @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 over 1 year ago →
    🇬🇧United Kingdom alexpott 🇪🇺🌍

    I 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 over 1 year ago →
    System Message
    • catch → committed 0217ee0a on 10.0.x 
      Issue #2552837 by smustgrave, pwolanin, alexpott: XSS::filter and...
  • Comment over 1 year ago →
    System Message
    • catch → committed 4421948a on 10.1.x 
      Issue #2552837 by smustgrave, pwolanin, alexpott: XSS::filter and...
  • Comment over 1 year ago →
    System Message
    • catch → committed c18e6eda on 9.5.x 
      Issue #2552837 by smustgrave, pwolanin, alexpott: XSS::filter and...
  • Status changed to Fixed over 1 year ago
  • Comment over 1 year ago →
    🇬🇧United Kingdom catch

    The change here looks good - just completely removing the attribute instead of leaving an invalid one. 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!

  • Comment over 1 year ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024