core/modules/editor/editor.module | 4 +- .../lib/Drupal/editor/EditorXssFilter/Standard.php | 113 ++++- .../lib/Drupal/editor/EditorXssFilterInterface.php | 10 + .../lib/Drupal/editor/Tests/EditorSecurityTest.php | 104 ++++- .../editor/Tests}/EditorXssFilter/StandardTest.php | 465 ++++++++++++++++++++ 5 files changed, 675 insertions(+), 21 deletions(-) diff --git a/core/modules/editor/editor.module b/core/modules/editor/editor.module index 0caeadb..5e1a602 100644 --- a/core/modules/editor/editor.module +++ b/core/modules/editor/editor.module @@ -438,7 +438,7 @@ function editor_filter_xss($html, FilterFormatInterface $format, FilterFormatInt // e.g.: an admin user creates content in Full HTML and then edits it, no text // format switching happens; in this case, no text editor XSS filtering is // desirable, because it would strip style attributes, amongst others. - $current_filter_types = filter_get_filter_types_by_format($format->id()); + $current_filter_types = $format->getFilterTypes(); if (!in_array(FILTER_TYPE_HTML_RESTRICTOR, $current_filter_types, TRUE)) { if ($original_format === NULL) { return FALSE; @@ -451,7 +451,7 @@ function editor_filter_xss($html, FilterFormatInterface $format, FilterFormatInt // used), and switches to Full HTML (for which a text editor is used). Then // we must apply XSS filtering to protect the admin user. else { - $original_filter_types = filter_get_filter_types_by_format($original_format->id()); + $original_filter_types = $original_format->getFilterTypes(); if (!in_array(FILTER_TYPE_HTML_RESTRICTOR, $original_filter_types, TRUE)) { return FALSE; } diff --git a/core/modules/editor/lib/Drupal/editor/EditorXssFilter/Standard.php b/core/modules/editor/lib/Drupal/editor/EditorXssFilter/Standard.php index d632ccf..3f93863 100644 --- a/core/modules/editor/lib/Drupal/editor/EditorXssFilter/Standard.php +++ b/core/modules/editor/lib/Drupal/editor/EditorXssFilter/Standard.php @@ -20,8 +20,117 @@ class Standard implements EditorXssFilterInterface { * {@inheritdoc} */ public static function filterXss($html, FilterFormatInterface $format, FilterFormatInterface $original_format = NULL) { - // Apply XSS filtering, but only blacklist the '; + protected static $sampleContent = '

Hello, Dumbo Octopus!

'; /** - * The secured sample content to use in all tests. + * The secured sample content to use in most tests. * * @var string */ protected static $sampleContentSecured = '

Hello, Dumbo Octopus!

alert(0)'; /** + * The secured sample content to use in tests when the tag is allowed. + * + * @var string + */ + protected static $sampleContentSecuredEmbedAllowed = '

Hello, Dumbo Octopus!

alert(0)'; + + /** * Modules to enable. * * @var array @@ -47,14 +54,15 @@ public static function getInfo() { function setUp() { parent::setUp(); - // Create 4 text formats, to cover all potential use cases: + // Create 5 text formats, to cover all potential use cases: // 1. restricted_without_editor (untrusted: anonymous) // 2. restricted_with_editor (normal: authenticated) - // 3. unrestricted_without_editor (privileged: admin) - // 4. unrestricted_with_editor (privileged: admin) - // With text formats 2 and 4, we also associate a text editor that does not - // guarantee XSS safety. "restricted" means the text format has XSS filters - // on output, "unrestricted" means the opposite. + // 3. restricted_plus_dangerous_tag_with_editor (privileged: trusted) + // 4. unrestricted_without_editor (privileged: admin) + // 5. unrestricted_with_editor (privileged: admin) + // With text formats 2, 3 and 5, we also associate a text editor that does + // not guarantee XSS safety. "restricted" means the text format has XSS + // filters on output, "unrestricted" means the opposite. $format = entity_create('filter_format', array( 'format' => 'restricted_without_editor', 'name' => 'Restricted HTML, without text editor', @@ -85,12 +93,32 @@ function setUp() { ), )); $format->save(); - $editor = entity_create('editor', array( + $editor = entity_create('editor', array( 'format' => 'restricted_with_editor', 'editor' => 'unicorn', )); $editor->save(); $format = entity_create('filter_format', array( + 'format' => 'restricted_plus_dangerous_tag_with_editor', + 'name' => 'Restricted HTML, dangerous tag allowed, with text editor', + 'weight' => 1, + 'filters' => array( + // A filter of the FILTER_TYPE_HTML_RESTRICTOR type. + 'filter_html' => array( + 'status' => 1, + 'settings' => array( + 'allowed_html' => '


', + ) + ), + ), + )); + $format->save(); + $editor = entity_create('editor', array( + 'format' => 'restricted_plus_dangerous_tag_with_editor', + 'editor' => 'unicorn', + )); + $editor->save(); + $format = entity_create('filter_format', array( 'format' => 'unrestricted_without_editor', 'name' => 'Unrestricted HTML, without text editor', 'weight' => 0, @@ -104,7 +132,7 @@ function setUp() { 'filters' => array(), )); $format->save(); - $editor = entity_create('editor', array( + $editor = entity_create('editor', array( 'format' => 'unrestricted_with_editor', 'editor' => 'unicorn', )); @@ -119,9 +147,11 @@ function setUp() { // Create 3 users, each with access to different text formats/editors: // - "untrusted": restricted_without_editor - // - "normal": restricted_with_editor + // - "normal": restricted_with_editor, + // - "trusted": restricted_plus_dangerous_tag_with_editor // - "privileged": restricted_without_editor, restricted_with_editor, - // unrestricted_without_editor and unrestricted_with_editor + // restricted_plus_dangerous_tag_with_editor, + // unrestricted_without_editor and unrestricted_with_editor $this->untrusted_user = $this->drupalCreateUser(array( 'create article content', 'edit any article content', @@ -132,11 +162,17 @@ function setUp() { 'edit any article content', 'use text format restricted_with_editor', )); + $this->trusted_user = $this->drupalCreateUser(array( + 'create article content', + 'edit any article content', + 'use text format restricted_plus_dangerous_tag_with_editor', + )); $this->privileged_user = $this->drupalCreateUser(array( 'create article content', 'edit any article content', 'use text format restricted_without_editor', 'use text format restricted_with_editor', + 'use text format restricted_plus_dangerous_tag_with_editor', 'use text format unrestricted_without_editor', 'use text format unrestricted_with_editor', )); @@ -146,6 +182,7 @@ function setUp() { $samples = array( array('author' => $this->untrusted_user->id(), 'format' => 'restricted_without_editor'), array('author' => $this->normal_user->id(), 'format' => 'restricted_with_editor'), + array('author' => $this->trusted_user->id(), 'format' => 'restricted_plus_dangerous_tag_with_editor'), array('author' => $this->privileged_user->id(), 'format' => 'unrestricted_without_editor'), array('author' => $this->privileged_user->id(), 'format' => 'unrestricted_with_editor'), ); @@ -163,7 +200,7 @@ function setUp() { /** * Tests initial security: is the user safe without switching text formats? * - * Tests 6 scenarios. Tests only with a text editor that is not XSS-safe. + * Tests 8 scenarios. Tests only with a text editor that is not XSS-safe. */ function testInitialSecurity() { $expected = array( @@ -189,6 +226,16 @@ function testInitialSecurity() { ), array( 'node_id' => 3, + 'format' => 'restricted_plus_dangerous_tag_with_editor', + // Text editor => XSS filtering. + 'value' => self::$sampleContentSecuredEmbedAllowed, + 'users' => array( + $this->trusted_user, + $this->privileged_user, + ), + ), + array( + 'node_id' => 4, 'format' => 'unrestricted_without_editor', // No text editor => no XSS filtering. 'value' => self::$sampleContent, @@ -197,7 +244,7 @@ function testInitialSecurity() { ), ), array( - 'node_id' => 4, + 'node_id' => 5, 'format' => 'unrestricted_with_editor', // Text editor, no security filter => no XSS filtering. 'value' => self::$sampleContent, @@ -225,7 +272,7 @@ function testInitialSecurity() { /** * Tests administrator security: is the user safe when switching text formats? * - * Tests 16 scenarios. Tests only with a text editor that is not XSS-safe. + * Tests 24 scenarios. Tests only with a text editor that is not XSS-safe. * * When changing from a more restrictive text format with a text editor (or a * text format without a text editor) to a less restrictive text format, it is @@ -243,7 +290,9 @@ function testSwitchingSecurity() { 'format' => 'restricted_without_editor', 'switch_to' => array( 'restricted_with_editor' => self::$sampleContentSecured, - // No text editor => no XSS filtering. + // Intersection of restrictions => most strict XSS filtering. + 'restricted_plus_dangerous_tag_with_editor' => self::$sampleContentSecured, + // No text editor => no XSS filtering. 'unrestricted_without_editor' => FALSE, 'unrestricted_with_editor' => self::$sampleContentSecured, ), @@ -255,6 +304,8 @@ function testSwitchingSecurity() { 'switch_to' => array( // No text editor => no XSS filtering. 'restricted_without_editor' => FALSE, + // Intersection of restrictions => most strict XSS filtering. + 'restricted_plus_dangerous_tag_with_editor' => self::$sampleContentSecured, // No text editor => no XSS filtering. 'unrestricted_without_editor' => FALSE, 'unrestricted_with_editor' => self::$sampleContentSecured, @@ -262,12 +313,29 @@ function testSwitchingSecurity() { ), array( 'node_id' => 3, + 'value' => self::$sampleContentSecuredEmbedAllowed, // Text editor => XSS filtering. + 'format' => 'restricted_plus_dangerous_tag_with_editor', + 'switch_to' => array( + // No text editor => no XSS filtering. + 'restricted_without_editor' => FALSE, + // Intersection of restrictions => most strict XSS filtering. + 'restricted_with_editor' => self::$sampleContentSecured, + // No text editor => no XSS filtering. + 'unrestricted_without_editor' => FALSE, + // Intersection of restrictions => most strict XSS filtering. + 'unrestricted_with_editor' => self::$sampleContentSecured, + ), + ), + array( + 'node_id' => 4, 'value' => self::$sampleContent, // No text editor => no XSS filtering. 'format' => 'unrestricted_without_editor', 'switch_to' => array( // No text editor => no XSS filtering. 'restricted_without_editor' => FALSE, 'restricted_with_editor' => self::$sampleContentSecured, + // Intersection of restrictions => most strict XSS filtering. + 'restricted_plus_dangerous_tag_with_editor' => self::$sampleContentSecured, // From no editor, no security filters, to editor, still no security // filters: resulting content when viewed was already vulnerable, so // it must be intentional. @@ -275,7 +343,7 @@ function testSwitchingSecurity() { ), ), array( - 'node_id' => 4, + 'node_id' => 5, 'value' => self::$sampleContentSecured, // Text editor => XSS filtering. 'format' => 'unrestricted_with_editor', 'switch_to' => array( @@ -283,6 +351,8 @@ function testSwitchingSecurity() { // risk. 'restricted_without_editor' => FALSE, 'restricted_with_editor' => self::$sampleContentSecured, + // Intersection of restrictions => most strict XSS filtering. + 'restricted_plus_dangerous_tag_with_editor' => self::$sampleContentSecured, // From no editor, no security filters, to editor, still no security // filters: resulting content when viewed was already vulnerable, so // it must be intentional. diff --git a/core/modules/editor/lib/Drupal/editor/Tests/editor/EditorXssFilter/StandardTest.php b/core/modules/editor/tests/Drupal/editor/Tests/EditorXssFilter/StandardTest.php similarity index 10% rename from core/modules/editor/lib/Drupal/editor/Tests/editor/EditorXssFilter/StandardTest.php rename to core/modules/editor/tests/Drupal/editor/Tests/EditorXssFilter/StandardTest.php index 54a9333..bce3073 100644 --- a/core/modules/editor/lib/Drupal/editor/Tests/editor/EditorXssFilter/StandardTest.php +++ b/core/modules/editor/tests/Drupal/editor/Tests/EditorXssFilter/StandardTest.php @@ -48,6 +48,22 @@ protected function setUp() { $this->format = $this->getMockBuilder('\Drupal\filter\Entity\FilterFormat') ->disableOriginalConstructor() ->getMock(); + $this->format->expects($this->any()) + ->method('getFilterTypes') + ->will($this->returnValue(array(FILTER_TYPE_HTML_RESTRICTOR))); + $restrictions = array( + 'allowed' => array( + 'p' => TRUE, + 'a' => TRUE, + '*' => array( + 'style' => FALSE, + 'on*' => FALSE, + ), + ), + ); + $this->format->expects($this->any()) + ->method('getHtmlRestrictions') + ->will($this->returnValue($restrictions)); } /** @@ -66,6 +82,455 @@ public function providerTestFilterXss() { $data[] = array('

Hello, world!

Pink Fairy Armadillo', '

Hello, world!

Pink Fairy Armadillo'); $data[] = array('

Hello, world!

Pink Fairy Armadillo', '

Hello, world!

Pink Fairy Armadilloalert("evil");'); $data[] = array('

Hello, world!

Pink Fairy Armadillo
test', '

Hello, world!

Pink Fairy Armadillotest'); + + // All cases listed on https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet + + // No Filter Evasion. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#No_Filter_Evasion + $data[] = array('', ''); + + // Image XSS using the JavaScript directive. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Image_XSS_using_the_JavaScript_directive + $data[] = array('', ''); + + // No quotes and no semicolon. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#No_quotes_and_no_semicolon + $data[] = array('', ''); + + // Case insensitive XSS attack vector. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Case_insensitive_XSS_attack_vector + $data[] = array('', ''); + + // HTML entities. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#HTML_entities + $data[] = array('', ''); + + // Grave accent obfuscation. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Grave_accent_obfuscation + $data[] = array('', ''); + + // Malformed A tags. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Malformed_A_tags + $data[] = array('xxs link', 'xxs link'); + $data[] = array('xxs link', 'xxs link'); + + // Malformed IMG tags. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Malformed_IMG_tags + $data[] = array('">', 'alert("XSS")">'); + + // fromCharCode. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#fromCharCode + $data[] = array('', ''); + + // Default SRC tag to get past filters that check SRC domain. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Default_SRC_tag_to_get_past_filters_that_check_SRC_domain + $data[] = array('', ''); + + // Default SRC tag by leaving it empty. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Default_SRC_tag_by_leaving_it_empty + $data[] = array('', ''); + + // Default SRC tag by leaving it out entirely. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Default_SRC_tag_by_leaving_it_out_entirely + $data[] = array('', ''); + + // Decimal HTML character references. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Decimal_HTML_character_references + $data[] = array('', ''); + + // Decimal HTML character references without trailing semicolons. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Decimal_HTML_character_references_without_trailing_semicolons + $data[] = array('', ''); + + // Hexadecimal HTML character references without trailing semicolons. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Hexadecimal_HTML_character_references_without_trailing_semicolons + $data[] = array('', ''); + + // Embedded tab. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Embedded_tab + $data[] = array('', ''); + + // Embedded Encoded tab. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Embedded_Encoded_tab + $data[] = array('', ''); + + // Embedded newline to break up XSS. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Embedded_newline_to_break_up_XSS + $data[] = array('', ''); + + // Embedded carriage return to break up XSS. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Embedded_carriage_return_to_break_up_XSS + $data[] = array('', ''); + + // Null breaks up JavaScript directive. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Null_breaks_up_JavaScript_directive + $data[] = array("", ''); + + // Spaces and meta chars before the JavaScript in images for XSS. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Spaces_and_meta_chars_before_the_JavaScript_in_images_for_XSS + $data[] = array('', ''); + + // Non-alpha-non-digit XSS. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Non-alpha-non-digit_XSS + $data[] = array('', ''); + $data[] = array('', ''); + $data[] = array('', ''); + + // Extraneous open brackets. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Extraneous_open_brackets + $data[] = array('<', '<alert("XSS");//<'); + + // No closing script tags. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#No_closing_script_tags + $data[] = array('', 'alert("XSS");'); + + // INPUT image. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#INPUT_image + $data[] = array('', ''); + + // BODY image. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#BODY_image + $data[] = array('', ''); + + // IMG Dynsrc. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#IMG_Dynsrc + $data[] = array('', ''); + + // IMG lowrsc. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#IMG_lowsrc + $data[] = array('', ''); + + // List-style-image. + // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#List-style-image + $data[] = array('