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, over 9 years ago
Updated 8 March 2023, almost 2 years 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 about 6 hours 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 almost 2 years 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 almost 2 years 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 almost 2 years ago →
    System Message
    • catch → committed 0217ee0a on 10.0.x 
      Issue #2552837 by smustgrave, pwolanin, alexpott: XSS::filter and...
  • Comment almost 2 years ago →
    System Message
    • catch → committed 4421948a on 10.1.x 
      Issue #2552837 by smustgrave, pwolanin, alexpott: XSS::filter and...
  • Comment almost 2 years ago →
    System Message
    • catch → committed c18e6eda on 9.5.x 
      Issue #2552837 by smustgrave, pwolanin, alexpott: XSS::filter and...
  • Status changed to Fixed almost 2 years ago
  • Comment almost 2 years 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 almost 2 years ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024