Commit 89b4f966 authored by catch's avatar catch
Browse files

Issue #2364647 by chx, alexpott: Fixed [sechole] Remove blacklist mode from Filter:XSS.

parent 55b85580
...@@ -14,18 +14,6 @@ ...@@ -14,18 +14,6 @@
*/ */
class Xss { class Xss {
/**
* Indicates that XSS filtering must be applied in whitelist mode: only
* specified HTML tags are allowed.
*/
const FILTER_MODE_WHITELIST = TRUE;
/**
* Indicates that XSS filtering must be applied in blacklist mode: only
* specified HTML tags are disallowed.
*/
const FILTER_MODE_BLACKLIST = FALSE;
/** /**
* The list of html tags allowed by filterAdmin(). * The list of html tags allowed by filterAdmin().
* *
...@@ -55,10 +43,6 @@ class Xss { ...@@ -55,10 +43,6 @@ class Xss {
* can cause an XSS attack. * can cause an XSS attack.
* @param array $html_tags * @param array $html_tags
* An array of HTML tags. * An array of HTML tags.
* @param bool $mode
* (optional) Defaults to FILTER_MODE_WHITELIST ($html_tags is used as a
* whitelist of allowed tags), but can also be set to FILTER_MODE_BLACKLIST
* ($html_tags is used as a blacklist of disallowed tags).
* *
* @return string * @return string
* An XSS safe version of $string, or an empty string if $string is not * An XSS safe version of $string, or an empty string if $string is not
...@@ -69,7 +53,7 @@ class Xss { ...@@ -69,7 +53,7 @@ class Xss {
* *
* @ingroup sanitization * @ingroup sanitization
*/ */
public static function filter($string, $html_tags = array('a', 'em', 'strong', 'cite', 'blockquote', 'code', 'ul', 'ol', 'li', 'dl', 'dt', 'dd'), $mode = Xss::FILTER_MODE_WHITELIST) { public static function filter($string, $html_tags = array('a', 'em', 'strong', 'cite', 'blockquote', 'code', 'ul', 'ol', 'li', 'dl', 'dt', 'dd')) {
// Only operate on valid UTF-8 strings. This is necessary to prevent cross // Only operate on valid UTF-8 strings. This is necessary to prevent cross
// site scripting issues on Internet Explorer 6. // site scripting issues on Internet Explorer 6.
if (!Unicode::validateUtf8($string)) { if (!Unicode::validateUtf8($string)) {
...@@ -90,8 +74,10 @@ public static function filter($string, $html_tags = array('a', 'em', 'strong', ' ...@@ -90,8 +74,10 @@ public static function filter($string, $html_tags = array('a', 'em', 'strong', '
// Named entities. // Named entities.
$string = preg_replace('/&([A-Za-z][A-Za-z0-9]*;)/', '&\1', $string); $string = preg_replace('/&([A-Za-z][A-Za-z0-9]*;)/', '&\1', $string);
$html_tags = array_flip($html_tags); $html_tags = array_flip($html_tags);
$splitter = function ($matches) use ($html_tags, $mode) { // Late static binding does not work inside anonymous functions.
return static::split($matches[1], $html_tags, $mode); $class = get_called_class();
$splitter = function ($matches) use ($html_tags, $class) {
return $class::split($matches[1], $html_tags, $class);
}; };
return SafeMarkup::set(preg_replace_callback('% return SafeMarkup::set(preg_replace_callback('%
( (
...@@ -134,15 +120,16 @@ public static function filterAdmin($string) { ...@@ -134,15 +120,16 @@ public static function filterAdmin($string) {
* @param array $html_tags * @param array $html_tags
* An array where the keys are the allowed tags and the values are not * An array where the keys are the allowed tags and the values are not
* used. * used.
* @param bool $split_mode * @param string $class
* Whether $html_tags is a list of allowed (if FILTER_MODE_WHITELIST) or * The called class. This method is called from an anonymous function which
* disallowed (if FILTER_MODE_BLACKLIST) HTML tags. * breaks late static binding. See https://bugs.php.net/bug.php?id=66622 for
* more information.
* *
* @return string * @return string
* If the element isn't allowed, an empty string. Otherwise, the cleaned up * If the element isn't allowed, an empty string. Otherwise, the cleaned up
* version of the HTML element. * version of the HTML element.
*/ */
protected static function split($string, $html_tags, $split_mode) { protected static function split($string, $html_tags, $class) {
if (substr($string, 0, 1) != '<') { if (substr($string, 0, 1) != '<') {
// We matched a lone ">" character. // We matched a lone ">" character.
return '&gt;'; return '&gt;';
...@@ -167,11 +154,7 @@ protected static function split($string, $html_tags, $split_mode) { ...@@ -167,11 +154,7 @@ protected static function split($string, $html_tags, $split_mode) {
} }
// When in whitelist mode, an element is disallowed when not listed. // When in whitelist mode, an element is disallowed when not listed.
if ($split_mode === static::FILTER_MODE_WHITELIST && !isset($html_tags[strtolower($elem)])) { if ($class::needsRemoval($html_tags, $elem)) {
return '';
}
// When in blacklist mode, an element is disallowed when listed.
elseif ($split_mode === static::FILTER_MODE_BLACKLIST && isset($html_tags[strtolower($elem)])) {
return ''; return '';
} }
...@@ -188,7 +171,7 @@ protected static function split($string, $html_tags, $split_mode) { ...@@ -188,7 +171,7 @@ protected static function split($string, $html_tags, $split_mode) {
$xhtml_slash = $count ? ' /' : ''; $xhtml_slash = $count ? ' /' : '';
// Clean up attributes. // Clean up attributes.
$attr2 = implode(' ', static::attributes($attrlist)); $attr2 = implode(' ', $class::attributes($attrlist));
$attr2 = preg_replace('/[<>]/', '', $attr2); $attr2 = preg_replace('/[<>]/', '', $attr2);
$attr2 = strlen($attr2) ? ' ' . $attr2 : ''; $attr2 = strlen($attr2) ? ' ' . $attr2 : '';
...@@ -303,4 +286,19 @@ protected static function attributes($attributes) { ...@@ -303,4 +286,19 @@ protected static function attributes($attributes) {
return $attributes_array; return $attributes_array;
} }
/**
* Whether this element needs to be removed altogether.
*
* @param $html_tags
* The list of HTML tags.
* @param $elem
* The name of the HTML element.
*
* @return bool
* TRUE if this element needs to be removed.
*/
protected static function needsRemoval($html_tags, $elem) {
return !isset($html_tags[strtolower($elem)]);
}
} }
...@@ -14,7 +14,7 @@ ...@@ -14,7 +14,7 @@
/** /**
* Defines the standard text editor XSS filter. * Defines the standard text editor XSS filter.
*/ */
class Standard implements EditorXssFilterInterface { class Standard extends Xss implements EditorXssFilterInterface {
/** /**
* {@inheritdoc} * {@inheritdoc}
...@@ -85,7 +85,7 @@ public static function filterXss($html, FilterFormatInterface $format, FilterFor ...@@ -85,7 +85,7 @@ public static function filterXss($html, FilterFormatInterface $format, FilterFor
// Also blacklist tags that are explicitly forbidden in either text format. // Also blacklist tags that are explicitly forbidden in either text format.
$blacklisted_tags = array_merge($blacklisted_tags, $forbidden_tags); $blacklisted_tags = array_merge($blacklisted_tags, $forbidden_tags);
return Xss::filter($html, $blacklisted_tags, Xss::FILTER_MODE_BLACKLIST); return static::filter($html, $blacklisted_tags);
} }
...@@ -133,4 +133,13 @@ protected static function getForbiddenTags($restrictions) { ...@@ -133,4 +133,13 @@ protected static function getForbiddenTags($restrictions) {
} }
} }
/**
* {@inheritdoc}
*/
protected static function needsRemoval($html_tags, $elem) {
// See static::filterXss() about how this class uses blacklisting instead
// of the normal whitelisting.
return !parent::needsRemoval($html_tags, $elem);
}
} }
...@@ -7,6 +7,7 @@ ...@@ -7,6 +7,7 @@
namespace Drupal\Tests\editor\Unit\EditorXssFilter; namespace Drupal\Tests\editor\Unit\EditorXssFilter;
use Drupal\editor\EditorXssFilter\Standard;
use Drupal\Tests\UnitTestCase; use Drupal\Tests\UnitTestCase;
use Drupal\filter\Plugin\FilterInterface; use Drupal\filter\Plugin\FilterInterface;
...@@ -50,12 +51,7 @@ protected function setUp() { ...@@ -50,12 +51,7 @@ protected function setUp() {
/** /**
* Provides test data for testFilterXss(). * Provides test data for testFilterXss().
* *
* \Drupal\editor\EditorXssFilter\Standard uses
* Drupal\Component\Utility\Xss. See \Drupal\Tests\Component\Utility\XssTest::testBlacklistMode()
* for more detailed test coverage.
*
* @see \Drupal\Tests\editor\Unit\editor\EditorXssFilter\StandardTest::testFilterXss() * @see \Drupal\Tests\editor\Unit\editor\EditorXssFilter\StandardTest::testFilterXss()
* @see \Drupal\Tests\Component\Utility\XssTest::testBlacklistMode()
*/ */
public function providerTestFilterXss() { public function providerTestFilterXss() {
$data = array(); $data = array();
...@@ -520,7 +516,7 @@ public function providerTestFilterXss() { ...@@ -520,7 +516,7 @@ public function providerTestFilterXss() {
} }
/** /**
* Tests the method for checking access to routes. * Tests the method for filtering XSS.
* *
* @param string $input * @param string $input
* The input. * The input.
...@@ -530,8 +526,68 @@ public function providerTestFilterXss() { ...@@ -530,8 +526,68 @@ public function providerTestFilterXss() {
* @dataProvider providerTestFilterXss * @dataProvider providerTestFilterXss
*/ */
public function testFilterXss($input, $expected_output) { public function testFilterXss($input, $expected_output) {
$output = call_user_func('\Drupal\editor\EditorXssFilter\Standard::filterXss', $input, $this->format); $output = Standard::filterXss($input, $this->format);
$this->assertSame($expected_output, $output); $this->assertSame($expected_output, $output);
} }
/**
* Tests removing disallowed tags and XSS prevention.
*
* \Drupal\Component\Utility\Xss::filter() has the ability to run in blacklist
* mode, in which it still applies the exact same filtering, with one
* exception: it no longer works with a list of allowed tags, but with a list
* of disallowed tags.
*
* @param string $value
* The value to filter.
* @param string $expected
* The string that is expected to be missing.
* @param string $message
* The assertion message to display upon failure.
* @param array $disallowed_tags
* (optional) The disallowed HTML tags to be passed to \Drupal\Component\Utility\Xss::filter().
*
* @dataProvider providerTestBlackListMode
*/
public function testBlacklistMode($value, $expected, $message, array $disallowed_tags) {
$value = Standard::filter($value, $disallowed_tags);
$this->assertSame($expected, $value, $message);
}
/**
* Data provider for testBlacklistMode().
*
* @see testBlacklistMode()
*
* @return array
* An array of arrays containing the following elements:
* - The value to filter.
* - The value to expect after filtering.
* - The assertion message.
* - (optional) The disallowed HTML tags to be passed to \Drupal\Component\Utility\Xss::filter().
*/
public function providerTestBlackListMode() {
return array(
array(
'<unknown style="visibility:hidden">Pink Fairy Armadillo</unknown><video src="gerenuk.mp4"><script>alert(0)</script>',
'<unknown>Pink Fairy Armadillo</unknown><video src="gerenuk.mp4">alert(0)',
'Disallow only the script tag',
array('script')
),
array(
'<unknown style="visibility:hidden">Pink Fairy Armadillo</unknown><video src="gerenuk.mp4"><script>alert(0)</script>',
'<unknown>Pink Fairy Armadillo</unknown>alert(0)',
'Disallow both the script and video tags',
array('script', 'video')
),
// No real use case for this, but it is an edge case we must ensure works.
array(
'<unknown style="visibility:hidden">Pink Fairy Armadillo</unknown><video src="gerenuk.mp4"><script>alert(0)</script>',
'<unknown>Pink Fairy Armadillo</unknown><video src="gerenuk.mp4"><script>alert(0)</script>',
'Disallow no tags',
array()
),
);
}
} }
...@@ -442,66 +442,6 @@ public function providerTestFilterXssNotNormalized() { ...@@ -442,66 +442,6 @@ public function providerTestFilterXssNotNormalized() {
return $cases; return $cases;
} }
/**
* Tests removing disallowed tags and XSS prevention.
*
* \Drupal\Component\Utility\Xss::filter() has the ability to run in blacklist
* mode, in which it still applies the exact same filtering, with one
* exception: it no longer works with a list of allowed tags, but with a list
* of disallowed tags.
*
* @param string $value
* The value to filter.
* @param string $expected
* The string that is expected to be missing.
* @param string $message
* The assertion message to display upon failure.
* @param array $disallowed_tags
* (optional) The disallowed HTML tags to be passed to \Drupal\Component\Utility\Xss::filter().
*
* @dataProvider providerTestBlackListMode
*/
public function testBlacklistMode($value, $expected, $message, array $disallowed_tags) {
$value = Xss::filter($value, $disallowed_tags, Xss::FILTER_MODE_BLACKLIST);
$this->assertSame($expected, $value, $message);
}
/**
* Data provider for testBlacklistMode().
*
* @see testBlacklistMode()
*
* @return array
* An array of arrays containing the following elements:
* - The value to filter.
* - The value to expect after filtering.
* - The assertion message.
* - (optional) The disallowed HTML tags to be passed to \Drupal\Component\Utility\Xss::filter().
*/
public function providerTestBlackListMode() {
return array(
array(
'<unknown style="visibility:hidden">Pink Fairy Armadillo</unknown><video src="gerenuk.mp4"><script>alert(0)</script>',
'<unknown>Pink Fairy Armadillo</unknown><video src="gerenuk.mp4">alert(0)',
'Disallow only the script tag',
array('script')
),
array(
'<unknown style="visibility:hidden">Pink Fairy Armadillo</unknown><video src="gerenuk.mp4"><script>alert(0)</script>',
'<unknown>Pink Fairy Armadillo</unknown>alert(0)',
'Disallow both the script and video tags',
array('script', 'video')
),
// No real use case for this, but it is an edge case we must ensure works.
array(
'<unknown style="visibility:hidden">Pink Fairy Armadillo</unknown><video src="gerenuk.mp4"><script>alert(0)</script>',
'<unknown>Pink Fairy Armadillo</unknown><video src="gerenuk.mp4"><script>alert(0)</script>',
'Disallow no tags',
array()
),
);
}
/** /**
* Checks that invalid multi-byte sequences are rejected. * Checks that invalid multi-byte sequences are rejected.
* *
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment