WordPress.org
Get WordPress

Make WordPress Core

Opened 19 months ago

Closed 18 months ago

#58336 closed defect (bug) (fixed)

Potential XSS on admin_body_class hook

Reported by: rafiem's profile rafiem Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: Security Keywords:
Focuses: Cc:

Description (last modified by dd32)

## Description:

We are from Patchstack want to report for a potential XSS onadmin_body_class hook. The admin_body_class hook as stated on ​https://developer.wordpress.org/reference/hooks/admin_body_class/ could be used to filters the CSS classes for the body tag in the admin area. Plugin or theme developer able to use this hook to extend the main body class value with supplied string. This is the implementation of admin_body_class hook on the WordPress core (​https://github.com/WordPress/wordpress-develop/blob/6.2/src/wp-admin/admin-header.php#L245) : 

<?php
$admin_body_classes = apply_filters( 'admin_body_class', '' );
$admin_body_classes = ltrim( $admin_body_classes . ' ' . $admin_body_class );
?>
<body class="wp-admin wp-core-ui no-js <?php echo $admin_body_classes; ?>">
<script type="text/javascript">
        document.body.className = document.body.className.replace('no-js','js');
</script>

Unfortunately, there is no proper sanitization applied to the echoed $admin_body_classes variable on the WordPress core. This could lead to a potential XSS when the value returned from using the hook is not properly sanitized on the plugin or theme code side.

Please note that we are not fully sure if this should be treated as vulnerability or it should fall only under the security code improvement. But we believe that the possible XSS could be fully prevented from WordPress core side if the implementation of the hook is properly sanitized.

## Steps To Reproduce:

Create a plugin or theme that have this example PHP codes :

<?php
add_action( 'admin_body_class', 'added_body_class'  );

public function added_body_class( $classes ) {
    $classes .= sanitize_text_field( $_GET['type'] );

    return $classes;
}

The XSS then could be triggered by visiting the URL that trigger above code using this example payload :

http://localhost/wp-admin?page=test&type=xxxxxxx" onload=alert(document.domain) xxx="

We currently tried to research some of the plugin and theme that could be vulnerable from the admin_body_class implementation. So far, we are able to find the practical XSS on the Advanced Custom Fields plugin (Ref : ​https://patchstack.com/articles/reflected-xss-in-advanced-custom-fields-plugins-affecting-2-million-sites/)

## Recommendations

The intended value of the HTML class parameter should only consist of specific whitelisted character to be valid. WordPress already have a function to sanitize html class value such as ​https://developer.wordpress.org/reference/functions/sanitize_html_class/ , so we recommend to use the function on the implementation of admin_body_class hook

## Impact

Potential XSS on the implementation of admin_body_class hook could lead to theft of information to a privilege escalation.

Attachments (1)

58336.diff​ (662 bytes) - added by SergeyBiryukov 19 months ago.

Download all attachments as: .zip

Change History (15)

#1 @dd32
19 months ago

  • Component changed from General to Security
  • Description modified (diff)

This ticket has been approved to be posted here by the Core Security team

#2 @audrasjb
19 months ago

  • Milestone changed from Awaiting Review to 6.3
  • Version trunk deleted

Hello there, welcome to WP Core Trac and thank you for the ticket!

Let's move this to milestone 6.3.

#3 follow-up: @audrasjb
19 months ago

  • Keywords needs-patch needs-unit-tests added

The easier way to handle this would probably to sanitize $admin_body_classes late, when displaying the classes:

$admin_body_classes = apply_filters( 'admin_body_class', '' );
$admin_body_classes = ltrim( $admin_body_classes . ' ' . $admin_body_class );
?>
<body class="wp-admin wp-core-ui no-js <?php echo sanitize_html_class( $admin_body_classes ); ?>">

Maybe it would also be worth to put together some PHPUnit test cases for this.

#4 in reply to: ↑ 3 @SergeyBiryukov
19 months ago

Replying to audrasjb:

The easier way to handle this would probably to sanitize $admin_body_classes late, when displaying the classes:

$admin_body_classes = apply_filters( 'admin_body_class', '' );
$admin_body_classes = ltrim( $admin_body_classes . ' ' . $admin_body_class );
?>
<body class="wp-admin wp-core-ui no-js <?php echo sanitize_html_class( $admin_body_classes ); ?>">

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

#5 follow-up: @costdev
19 months ago

@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 ​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 ​the HTML spec or ​the CSS Working Draft are so restrictive.

From the HTML spec:

When specified on HTML elements, the class attribute must have a value that is a set of space-separated tokens representing the various classes that the element belongs to.

Note:

Assigning classes to an element affects class matching in selectors in CSS, the getElementsByClassName() method in the DOM, and other such features.

There are no additional restrictions on the tokens authors can use in the class attribute, 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.

my emphasis

Additional Notes

Usage of the two functions in Core:

@SergeyBiryukov
19 months ago

#6 in reply to: ↑ 5 @SergeyBiryukov
19 months ago

  • Keywords has-patch added; needs-patch removed

Thanks for the comparison!

I was thinking of a general principle of ​sanitizing early and escaping as late as possible, so for the output itself, esc_attr() seems more appropriate.

If I'm reading correctly, sanitize_html_class() does not allow spaces, so the suggestion from comment:3 would break if there are multiple classes in $admin_body_classes, which is the case by default.

It is worth noting that wp-admin/customize.php already uses esc_attr() for the output of body classes​, so I believe we should do the same here. It would also resolve the XSS mentioned in the ticket description.

With that in mind, I think 58336.diff​ should address the issue raised here.

#7 @westonruter
19 months ago

See also #20009 in which the same fix was done for body_class() via [48060]. Note that this can break plugins that hack the filter to inject attributes on the body element. (IMO, such plugins should be broken 😬)

#8 @SergeyBiryukov
19 months ago

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

In 55846:

Administration: Add missing escaping for CSS classes on the body tag in the admin.

Follow-up to [5892], [10823], [10868], [18882], [21014], [22000], [48060].

Propos rafiem, costdev, dd32, audrasjb, westonruter, SergeyBiryukov.
Fixes #58336.

#9 @hbhalodia
19 months ago

Hi All, I think this was related to a security issue, hence I have added this issue to hackerone.com instead of trac. You can check the issue here - ​https://hackerone.com/reports/1988670. It is being closed by them, but do not know if this is a correct issue to add it there. I thought it could be public if I add it through trac.

#10 follow-up: @johnbillion
19 months ago

  • Keywords needs-unit-tests has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Additional hardening via escaping is almost always a good idea, but this change was made in isolation with no consideration of the underlying approach that WordPress core takes to filtered values.

  • Where does responsibility lie for sanitising and escaping a filtered value that does not originate from untrusted content (ie. literal strings)? This change suggests it's the responsibility of WordPress core instead of plugins and themes that use filters.
  • What is the approach that WordPress core takes and commits to? Note that WordPress core text translations are treated as "trusted" and not escaped upon output, but they all pass through the gettext filter (and others) and are therefore susceptible to the same concern that was raised in this ticket for the admin_body_class filter.
  • Why is the admin_body_class filter a special case? Is it more likely than others to be filtered to include a value from untrusted input? See the ACF vulnerability in the ticket description for example.

There are hundreds of instances of values in WordPress core that originate from trusted input but pass through a filter and then get echoed without escaping. A quick search for echo $ returns nearly 1,000 results, here are the first few filters I found that operate in this way:

  • login_title
  • login_headertext
  • login_message
  • post_updated_messages
  • enter_title_here

In addition, most instances of translations in WordPress core are not escaped on output but first pass through the gettext filter.

So the question is, is admin_body_class a special case? If it needs to be fixed then many more filters need to be fixed too, or the project needs to be clear on where the responsibility for sanitising and escaping filtered values lies (ie. with plugin and themes).

I don't think [55846] needs to be reverted, but we do need to discuss the overall approach to escaping filtered values. It it not practical for WordPress core to treat all filtered values as untrusted input.

#11 follow-up: @lgladdy
19 months ago

I think for me, the thing this meant this one should have been fixed, is that the same non-admin filter, body_class, does escape everything, but admin_body_class didn't.

That feels like a discrepancy which made it more likely to be overlooked by plugin and theme devs.

#12 in reply to: ↑ 11 @desrosj
19 months ago

Just adding a bit of history for full context to the front end changes. It looks like late escaping was added for body and post classes in WP 5.5 through #20009/[48060]. There was also a follow up ​in WPCS.

#13 in reply to: ↑ 10 @SergeyBiryukov
19 months ago

Replying to johnbillion:

we do need to discuss the overall approach to escaping filtered values.

Thanks for bringing this up, there were a few related discussions recently:

#14 @johnbillion
18 months ago

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

Thanks for the comments. It seems that there's nothing more to add to this ticket so I'll re-close it, but the points above are important as it's definitely not the responsibility of WordPress core to escape output from all filters in general. If this comes up in future tickets, let's discuss it further.

Note: See TracTickets for help on using tickets.