Make WordPress Core

#56655 closed defect (bug) (wontfix)

add_menu_page - escaping $page_title, $menu_title

Reported by: soupia18's profile soupia18 Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-screenshots has-patch 2nd-opinion
Focuses: administration Cc:

Description

Just wondering - shouldn't something like below, be escaped before the output?

<?php
add_menu_page(
        __('<script>alert("hi1")</script>Page Title','text-domain'),
        __('<script>alert("hi2")</script>Menu Title','text-domain'),
        'manage_options',
        'menu-slug',
        array($this, 'my_callback'),
        'dashicons-images-alt2', 
);

Custom Post Types labels are going through escaping, but adding admin menus like above, it doesn't.

Attachments (1)

Screenshot 2022-09-26 at 12.42.34.png (61.7 KB) - added by martin.krcho 18 months ago.

Download all attachments as: .zip

Change History (10)

#1 @martin.krcho
18 months ago

  • Focuses administration added
  • Keywords needs-patch has-screenshots added
  • Type changed from enhancement to defect (bug)
  • Version set to trunk

Hey @soupia18, thank you very much for creating this ticket. Also, congratulations on creating your first ticket in WordPress Trac.

I can confirm that this is an issue in the latest version of WordPress (trunk). I was able to replicate the issue using the following mu-plugin.

<?php
add_action( 'admin_init', function () {
        add_menu_page(
                __( '<script>alert("hi1")</script>Page Title', 'text-domain' ),
                __( '<script>alert("hi2")</script>Menu Title', 'text-domain' ),
                'manage_options',
                'menu-slug',
                function () {
                        echo 'testing XSS issue';
                },
                'dashicons-images-alt2'
        );
} );

I can see a JS alert saying "hi2" when loading the WP Admin - see the screenshot above.

#2 @soupia18
18 months ago

Hello @martinkrcho - thank you for the welcome.

The issue is in /wp-admin/menu-header.php

Both menu and submenus titles are printed unescaped.

It looks like wp_kses() might be needed there - with some allowed_html tags. E.g. the Plugins menu contains span elements.

There are 3 variables that need to be escaped there (at least with what I have tested so far).

#3 @soupia18
18 months ago

@martinkrcho

Hi! I just noticed that the same issue exists also when we use add_meta_boxes().

This example:

<?php
add_meta_box(
        'prefix_my_metabox',
        __('<script>alert('hola!');</script>Metabox Title', 'text-domain'),
        'prefix_my_callback',
        null,
        'normal',
        'default',
        array(),
);

Would make sense to create a new ticket/report for this one?

This ticket was mentioned in PR #3356 on WordPress/wordpress-develop by joomlabeat.


18 months ago
#4

  • Keywords has-patch added; needs-patch removed

Admin menu-header - Printing menu and submenu titles goes through escaping with wp_kses() and a custom list of $allowed_html tags. The $allowed_html is defined early in the _wp_menu_output() and later on, the wp_kses() is used in 3 locations, where the title is either printed or defined on a variable and is ready for printing.

Trac ticket: [](https://core.trac.wordpress.org/ticket/56655)

This ticket was mentioned in Slack in #core by desrosj. View the logs.


17 months ago

#6 @desrosj
17 months ago

  • Milestone changed from Awaiting Review to 6.1.1
  • Severity changed from critical to normal
  • Version 6.1 deleted

#7 @azaozz
17 months ago

  • Keywords 2nd-opinion added

I can confirm that this is an issue...
...
Both menu and submenus titles are printed unescaped.
...
I just noticed that the same issue exists also when we use add_meta_boxes()

The question here is where these strings come from? Can they be entered by (unidentified) users or are they coming from plugins?

As far as I see the reason for not escaping the menu titles is that a plugin may want to add a bit of HTML to make a menu "bubble" with a count, like for comments, available updates, etc. These strings are not different than any other string outputted by a plugin, i.e. a plugin can add any HTML and any JS to pretty much any place in wp-admin. Don't see why menu titles should be restricted, doesn't seem to serve any purpose. :)

It looks like wp_kses() might be needed there

Not sure that is a good idea. KSES is pretty slow and is intended only for use when user supplied content is saved to the DB. All other uses of it, especially on outputting/displaying content, make WP slow.

For example: if a plugin would allow a user to enter a menu title, it should use KSES on saving the title entered by the user to make sue it is safe if it wanted to allow users to enter HTML tags at all. Usually a plugin would use sanitize_text_field() to remove tags and make the text "safe". This is the same for any other user input.

Last edited 17 months ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


17 months ago

#9 @peterwilsoncc
17 months ago

  • Milestone 6.1.1 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Can they be entered by (unidentified) users or are they coming from plugins?

The intent the function is that the strings come from plugins rather than users.

If a plugin developer creates a plugin that uses this function based on user input, then the plugin becomes responsible for sanitizing the user input appropriately. WordPress takes a similar approach with custom post type labels.


I think this can be closed without a fix.

Anything that requires PHP to exploit (for want of a better word) is generally considered fine as PHP code has full access to the WordPress install by definition. Anyone wishing to do something nasty via PHP can find far more suitable APIs to use to do far nastier things.

The plugin architectures flexibility has it's upsides and it's downsides.

Thanks for your report.

Note: See TracTickets for help on using tickets.