Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tag Processor: Document and test XSS prevention in set_attribute #44447

Merged
merged 10 commits into from
Sep 29, 2022
Merged

Tag Processor: Document and test XSS prevention in set_attribute #44447

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8bd5823
Document and test encoding double quotes
adamziel Sep 26, 2022
4c522dd
Reword the docstring
adamziel Sep 26, 2022
55a0a83
Encode < and > characters in set_attribute
adamziel Sep 27, 2022
1a84536
Use esc_attr to escape the attributes values
adamziel Sep 28, 2022
6c309a8
Escape attribute values using esc_attr
adamziel Sep 28, 2022
eb8bda4
decouple tests from behavior of `esc_attr()`
dmsnell Sep 28, 2022
907d37d
Decode character entities in `get_attribute()`
dmsnell Sep 28, 2022
8cf66b0
Add WP_HTML_Tag_Processor_Test_WP file with tests requiring WordPress…
adamziel Sep 29, 2022
a1153e4
Lint
adamziel Sep 29, 2022
50bf5f0
Merge branch 'trunk' into update/tag-processor-escape-attribute-values
adamziel Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/experimental/html/class-wp-html-tag-processor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -938,6 +938,8 @@ public function get_tag() {
* - When `true` is passed as the value, then only the attribute name is added to the tag.
* - When `false` is passed, the attribute gets removed if it existed before.
*
* For string attributes, the value is escaped using the `esc_attr` function.
*
* @since 6.1.0
*
* @param string $name The attribute name to target.
Expand All @@ -961,8 +963,7 @@ public function set_attribute( $name, $value ) {
if ( true === $value ) {
$updated_attribute = $name;
} else {
// @TODO: What escaping and sanitization do we need here?
$escaped_new_value = str_replace( '"', '&quot;', $value );
$escaped_new_value = esc_attr( $value );
$updated_attribute = "{$name}=\"{$escaped_new_value}\"";
}

Expand Down
83 changes: 79 additions & 4 deletions phpunit/html/wp-html-tag-processor-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,16 @@
*/

if ( ! function_exists( 'esc_attr' ) ) {
function esc_attr( $s ) {
return str_replace( '"', '&quot;', $s );
function get_option( $option ) {
if ( 'blog_charset' === $option ) {
return 'UTF-8';
}
}
function wp_load_alloptions(){}
function apply_filters( $name, $value ) {
return $value; }
require_once( __DIR__ . '/../../../../wordpress-develop/src/wp-includes/kses.php' );
require_once( __DIR__ . '/../../../../wordpress-develop/src/wp-includes/formatting.php' );
adamziel marked this conversation as resolved.
Show resolved Hide resolved
}

if ( ! class_exists( 'WP_UnitTestCase' ) ) {
Expand Down Expand Up @@ -235,6 +242,74 @@ public function test_set_attribute_on_a_non_existing_tag_does_not_change_the_mar
);
}

/**
* Passing a double quote inside of an attribute values could lead to an XSS attack as follows:
*
* <code>
* $p = new WP_HTML_Tag_Processor( '<div class="header"></div>' );
* $p->next_tag();
* $p->set_attribute('class', '" onclick="alert');
* echo $p;
* // <div class="" onclick="alert"></div>
* </code>
*
* To prevent it, `set_attribute` encodes the double quote character as an HTML entity:
*
* <code>
* <div class="&quot; onclick=&quot;alert"></div>
* </code>
*
* As per the HTML standard, the only way to escape out of the double quoted attribute value is
* via the terminating double quote character ", so there is no need to encode other characters
* such as <, >, /, or '.
*
* @see https://html.spec.whatwg.org/#attribute-value-(double-quoted)-state
*
* @ticket 56299
*
* @dataProvider data_set_attribute_xss_values
* @covers set_attribute
*/
public function test_set_attribute_prevents_xss( $initial_html, $attribute_value, $expected_html ) {
$p = new WP_HTML_Tag_Processor( $initial_html );
$p->next_tag();
$p->set_attribute( 'test', $attribute_value );
$this->assertSame(
$expected_html,
(string) $p,
'Setting an attribute to a malicious value did not escape it correctly – this is an XSS vulnerability.'
);
}

/**
* Data provider with malicious HTML attribute values.
*
* @return array {
* @type string $initial_html The initial HTML to process.
* @type string $attribute_value The value to set for the `test` attribute of the first encountered tag.
* @type string $expected_html The expected HTML after the processing.
* }
*/
public function data_set_attribute_xss_values() {
return array(
'Double quotes should be encoded as HTML entities' => array( '<div></div>', '"', '<div test="&quot;"></div>' ),
'Encoded quotes should not be encoded again' => array( '<div></div>', '&quot;', '<div test="&quot;"></div>' ),
'Ampersand should be encoded' => array( '<div></div>', '&', '<div test="&amp;"></div>' ),
'An encoded ampersand should not be encoded again' => array( '<div></div>', '&amp;', '<div test="&amp;"></div>' ),
'An encoded euro entity should not be encoded again' => array( '<div></div>', '&euro;', '<div test="&euro;"></div>' ),
'Single quotes should be encoded' => array( '<div></div>', "'", '<div test="&#039;"></div>' ),
'< and > characters should not be encoded' => array( '<div></div>', '<>', '<div test="&lt;&gt;"></div>' ),
'A quote inside of an HTML entity should still be encoded' => array( '<div></div>', '&quot";', '<div test="&amp;quot&quot;;"></div>' ),
'Single-quoted attributes should be rewritten using double quotes' => array( "<div test='foo'></div>", 'foo', '<div test="foo"></div>' ),
'Unquoted attributes should be rewritten using double quotes' => array( '<div test=foo></div>', 'foo', '<div test="foo"></div>' ),
'An HTML snippet passed as a value should only have the double quote characters encoded ' => array(
'<div test=foo></div>',
'" onclick="alert(\'1\');"><span onclick=""></span><script>alert("1")</script>',
'<div test="&quot; onclick=&quot;alert(&#039;1&#039;);&quot;&gt;&lt;span onclick=&quot;&quot;&gt;&lt;/span&gt;&lt;script&gt;alert(&quot;1&quot;)&lt;/script&gt;"></div>',
),
);
}

/**
* @ticket 56299
*
Expand Down Expand Up @@ -999,12 +1074,12 @@ public function data_malformed_tag() {

$examples['HTML tag opening inside attribute value'] = array(
'<pre id="<code" class="wp-block-code <code is poetry&gt;"><code>This &lt;is> a &lt;strong is="true">thing.</code></pre><span>test</span>',
'<pre foo="bar" id="<code" class="wp-block-code <code is poetry&gt; firstTag"><code class="secondTag">This &lt;is> a &lt;strong is="true">thing.</code></pre><span>test</span>',
'<pre foo="bar" id="<code" class="wp-block-code &lt;code is poetry&gt; firstTag"><code class="secondTag">This &lt;is> a &lt;strong is="true">thing.</code></pre><span>test</span>',
);

$examples['HTML tag brackets in attribute values and data markup'] = array(
'<pre id="<code-&gt;-block-&gt;" class="wp-block-code <code is poetry&gt;"><code>This &lt;is> a &lt;strong is="true">thing.</code></pre><span>test</span>',
'<pre foo="bar" id="<code-&gt;-block-&gt;" class="wp-block-code <code is poetry&gt; firstTag"><code class="secondTag">This &lt;is> a &lt;strong is="true">thing.</code></pre><span>test</span>',
'<pre foo="bar" id="<code-&gt;-block-&gt;" class="wp-block-code &lt;code is poetry&gt; firstTag"><code class="secondTag">This &lt;is> a &lt;strong is="true">thing.</code></pre><span>test</span>',
);

$examples['Single and double quotes in attribute value'] = array(
Expand Down