Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#52894 closed defect (bug) (fixed)

The wp_sanitize_script_attributes function added in version 5.7 does not escape attributes in some cases.

Reported by: tmatsuur's profile tmatsuur Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 5.7.1 Priority: normal
Severity: critical Version: 5.7
Component: Script Loader Keywords: has-patch commit fixed-major dev-feedback
Focuses: Cc:

Description

The wp_sanitize_script_attributes function added in version 5.7 is not escaped if the array value is true.

Steps To Reproduce:

1.Add the following code to your theme's functions.php.

echo wp_get_script_tag( ['>console.log("hello")</script><script' => true ] );

2.Display the admin page.

3.The following script tag is output and executed.

<script >console.log("hello")</script><script></script>

Problematic source code:

line 2356 in the wp-includes/script-loader.php file.

$attributes_string .= $html5_script_support ? sprintf( ' %1$s="%2$s"', esc_attr( $attribute_name ), esc_attr( $attribute_name ) ) : ' ' . $attribute_name;

I think I need to escape as follows:

$attributes_string .= $html5_script_support ? sprintf( ' %1$s="%2$s"', esc_attr( $attribute_name ), esc_attr( $attribute_name ) ) : ' ' . esc_attr( $attribute_name );

Attachments (1)

52894.diff (840 bytes) - added by tmatsuur 4 years ago.

Download all attachments as: .zip

Change History (9)

@tmatsuur
4 years ago

#1 @johnbillion
4 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 5.7.1

#2 @joyously
4 years ago

This new function isn't sanitizing; it is only escaping. I think it's misnamed.
When the example is output, it has the angle brackets changed to entities, but the quotes are still nested, so it's not good.
If it were to sanitize, it would strip_tags and convert quotes.

#3 @tmatsuur
4 years ago

With the patch applied, I added the following code to the theme's functions.php.

echo wp_get_script_tag( ['>console.log("hello");console.log(\'world\')</script><script' => true ] );

The output script tag looks like this, and the quotes appear to be converted.

<script &gt;console.log(&quot;hello&quot;);console.log(&#039;world&#039;)&lt;/script&gt;&lt;script></script>

#4 @joyously
4 years ago

Fine, but that is still escaping, not sanitizing. Some of those characters are not valid for attributes.

#5 @peterwilsoncc
4 years ago

  • Keywords commit added

Strictly speaking, @joyously is correct. While browsers can handle entities, the / in the example above confuses browsers and causes the attribute to be split in to two (tested in Firefox on macOS):

&gt;console.log(&quot;hello&quot;);console.log(&#039;world&#039;)&lt; script&gt;&lt;script

From the security perspective of keeping the visitor safe, it's doing the job so I think it's good to go in. As sanitization is a semi defined umbrella term for some combination of validation, escaping and filtering I think the name is fine for now.

#6 @peterwilsoncc
4 years ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 50575:

Script Loader: Escape HTML5 boolean attribute names.

Add escaping of boolean attribute names in wp_sanitize_script_attributes() for themes supporting HTML5 script elements.

Props tmatsuur, johnbillion, joyously.
Fixes #52894.

#7 @peterwilsoncc
4 years ago

  • Keywords fixed-major dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for committing to the 5.7 branch.

Although it's not required until RC1, for my own piece of mind it would be great if a second committer could sign-off on the backport with the dev-reviewed keyword. Ta.

#8 @peterwilsoncc
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 50661:

Script Loader: Escape HTML5 boolean attribute names.

Add escaping of boolean attribute names in wp_sanitize_script_attributes() for themes supporting HTML5 script elements.

Props tmatsuur, johnbillion, joyously.
Merges [50575] to the 5.7 branch.
Fixes #52894.

Note: See TracTickets for help on using tickets.