[wp-trac] [WordPress Trac] #58336: Potential XSS on admin_body_class hook

[WordPress Trac]

#58336: Potential XSS on admin_body_class hook
------------------------------------------+---------------------
 Reporter:  rafiem                        |       Owner:  (none)
     Type:  defect (bug)                  |      Status:  new
 Priority:  normal                        |   Milestone:  6.3
Component:  Security                      |     Version:
 Severity:  normal                        |  Resolution:
 Keywords:  needs-patch needs-unit-tests  |     Focuses:
------------------------------------------+---------------------

Comment (by costdev):

 > @SergeyBiryukov Would `esc_attr()` be more appropriate here? I think
 that's what we generally use for escaping in cases like this.

 == For This One

 I'd say `sanitize_html_class()` is probably the most appropriate as it's
 [https://github.com/WordPress/wordpress-develop/blob/6.2/src/wp-admin
 /admin-header.php#L192 already used in the file for the same variable] and
 changing this would break BC due to the different filters in, and outputs
 from, `sanitize_html_class()` and `esc_attr()`.

 == New Code

 We may need to determine a definitive method of selecting which function
 is used - Assuming there isn't one already that we've forgotten.

 `sanitize_html_class()` is significantly more restrictive than
 `esc_attr()`, which may/may not be desirable in context.

 Example: `<div class="@~:[]()%$#howdy, admin!">Howdy, admin!</div>`
 Targetable with:
 {{{
 .\@\~\:\[\]\(\)\%\$\#howdy\,.admin\! {
     color: red;
 }
 }}}
 Output:
 - `sanitize_html_class()`: `howdyadmin`
 - `esc_attr()`: `@~:[]()%$#howdy, admin!`

 -----

 Most articles will say that a class should only contain something like
 `\.[^0-9][a-zA-Z0-9_-]+`, however I don't believe
 [https://html.spec.whatwg.org/#classes the HTML spec] or
 [https://www.w3.org/TR/selectors-4/#class-html the CSS Working Draft] are
 so restrictive.

 **From the HTML spec:**
 {{{#!html
 <blockquote>
 <p>When specified on HTML elements, <u>the <code>class</code> attribute
 must have a value that is a <a href="https://html.spec.whatwg.org/#set-of-
 space-separated-tokens">set of space-separated tokens</a> representing the
 various classes that the element belongs to</u>.</p>
 <strong>Note:</strong>
 <p>Assigning classes to an element affects class matching in selectors in
 CSS, the <code>getElementsByClassName()</code> method in the DOM, and
 other such features.</p>
 <p><u>There are no additional restrictions on the tokens authors can use
 in the <code>class</code> attribute</u>, but authors are encouraged to use
 values that describe the nature of the content, rather than values that
 describe the desired presentation of the content.</p>
 </blockquote>
 <u>my emphasis</u>
 }}}

 === Additional Notes
 Usage of the two functions in Core:
 - there's ~47 uses of `sanitize_html_class()`
 - there's 1000+ uses of `esc_attr()`
 - `sanitize_html_class()` is used for `body` classes in:
   - [https://github.com/wordpress/wordpress-
 develop/blob/749847445a0cf20b1f3966e7a9b4dcc9aa37b826/src/wp-admin/admin-
 header.php#L191 admin-header.php] (this file)
   - [https://github.com/wordpress/wordpress-
 develop/blob/749847445a0cf20b1f3966e7a9b4dcc9aa37b826/src/wp-
 admin/customize.php#L145 customize.php]
   - [https://github.com/costdev/wordpress-
 develop/blob/749847445a0cf20b1f3966e7a9b4dcc9aa37b826/src/wp-
 admin/includes/template.php#L2163 template.php]
   - [https://github.com/wordpress/wordpress-
 develop/blob/749847445a0cf20b1f3966e7a9b4dcc9aa37b826/src/wp-includes
 /class-wp-editor.php#L730 class-wp-editor.php]

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/58336#comment:5>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform