WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#10310 closed defect (bug) (fixed)

add_menu_page Security Bug

Reported by: shazahm1@… Owned by: ryan
Milestone: 2.8.1 Priority: normal
Severity: major Version: 2.8
Component: Menus Keywords:
Focuses: Cc:

Description

I think there is a security issue with the add_menu_page() function but I'm a noob and might be doing something wrong but I was able to duplicate it with the sample code from the codex.

<?php
/*
Plugin Name: Menu Test
Plugin URI: http://wordpress.org
Description: Menu Test
Author: Nobody
Author URI: http://example.com
*/

// Hook for adding admin menus
add_action('admin_menu', 'mt_add_pages');

// action function for above hook
function mt_add_pages() {
    // Add a new submenu under Options:
    add_options_page('Test Options', 'Test Options', 8, 'testoptions', 'mt_options_page');

    // Add a new submenu under Manage:
    add_management_page('Test Manage', 'Test Manage', 8, 'testmanage', 'mt_manage_page');

    // Add a new top-level menu (ill-advised):
    add_menu_page('Test Toplevel', 'Test Toplevel', 8, __FILE__, 'mt_toplevel_page');

    // Add a submenu to the custom top-level menu:
    add_submenu_page(__FILE__, 'Test Sublevel', 'Test Sublevel', 8, 'sub-page', 'mt_sublevel_page');

    // Add a second submenu to the custom top-level menu:
    add_submenu_page(__FILE__, 'Test Sublevel 2', 'Test Sublevel 2', 8, 'sub-page2', 'mt_sublevel_page2');
}

// mt_options_page() displays the page content for the Test Options submenu
function mt_options_page() {
    echo "<h2>Test Options</h2>";
}

// mt_manage_page() displays the page content for the Test Manage submenu
function mt_manage_page() {
    echo "<h2>Test Manage</h2>";
}

// mt_toplevel_page() displays the page content for the custom Test Toplevel menu
function mt_toplevel_page() {
    echo "<h2>Test Toplevel</h2>";
}

// mt_sublevel_page() displays the page content for the first submenu
// of the custom Test Toplevel menu
function mt_sublevel_page() {
    echo "<h2>Test Sublevel</h2>";
}

// mt_sublevel_page2() displays the page content for the second submenu
// of the custom Test Toplevel menu
function mt_sublevel_page2() {
    echo "<h2>Test Sublevel 2</h2>";
}

?>

Let's say a user is logged in as a subscriber and types in the query string to access the top level menu ?page=menu_test.php the page will be displayed even though only admins should see the page as set in the parameter set in add_menu_page. However type in the query string for the subpages are blocked as expected. I've also attached my code that shows the same problem.

Attachments (1)

test-plugin.zip (1.1 KB) - added by shazahm1@… 5 years ago.

Download all attachments as: .zip

Change History (16)

shazahm1@…5 years ago

comment:1 ryan5 years ago

Try with the 2.8.1 beta, which enforces more strictly. Plugins should always do explicit capability checks, regardless.

comment:2 Denis-de-Bernardy5 years ago

as admin in 2.8.1-beta-2, I can't access an admin page that isn't registered explicitly.

comment:3 follow-up: hakre5 years ago

(for me the same)

Just installed your plugin.

/wordpress-trunk/wp-admin/admin.php?page=menu_test.php gives me a WP_Die with this message:

You do not have sufficient permissions to access this page.

I am admin. When logged out WP display the login form.

So this is worksforme because I'm unable to reproduce.

Tested against current trunk. Maybe you have got other Plugins activated, not the default Theme?

comment:4 Denis-de-Bernardy5 years ago

  • Milestone changed from Unassigned to 2.8.1
  • Resolution set to fixed
  • Status changed from new to closed

he's testing using 2.8, and his bug works then indeed.

comment:5 shazahm1@…5 years ago

I just installed 2.8.1-beta2 and retested as suggested. I can no longer access the page using the query string as I reported. However this can be reproduced in both released versions of 2.7 and 2.8.

comment:6 hakre5 years ago

As ryan properly pointed out, it is the plugin page itself that needs to check for propper access rights.

comment:7 shazahm1@…5 years ago

Understood... I'm off to Google to see how this is done properly. None of the tutorials have come across so far mentioned needing an additional check. I know I should use current_user_can and wp_die. I'm just not exactly sure where the right place to use.

comment:9 DeNeusbeer4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I recently encountered this bug again in WP 2.9.1. I reproduced it using a fresh install of wordpress and this very simple plugin i wrote. It only adds 1 menu page for Administrators, and 1 submenu page for Contributors:

<?php
/*
Plugin Name: Menu testing
Plugin URI: 
Description: Testing menu page structure
Author: DeNeusbeer
*/
 
add_action('admin_menu', 'admin_menu_custom');
 
 
function admin_menu_custom() {
    
    add_menu_page('Adminpage', 'Adminpage', 'administrator', 'adminpage', 'get_adminpage');
    
    add_submenu_page('index.php', 'Testpage', 'Testpage', 'contributor', 'testpage', 'get_contributor_page');
}
 
function get_adminpage() {
    echo 'This is an administrator page, only administrators should see it.';    
}
    
function get_contributor_page() {
    echo 'this is a contributor page.';
}
?>

I added 2 users, one Administrator and one Contributor. The normal link to the newly created Adminpage menu is /wp-admin/admin.php?page=adminpage and this is accesible only be the Administrator, as expected. But if i point the browser to /wp-admin/index.php?page=adminpage , the Contributor can see the page aswell.

This only occurs for menu pages, not for submenu pages.

comment:10 ryan4 years ago

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

'administrator' and 'contributor' are not valid capabilities. They are roles. That could be the problem.

Could you create a new ticket for this? We prefer not to reopen old tickets, and creating a new one will give the issue more visibility.

comment:11 miqrogroove4 years ago

They are roles. That could be the problem.

Nah, the $access_level parameter has never been implemented for add_menu_page. wp-admin/menu.php displays all top level menus unless all children are forbidden. The hooks for those top level pages are totally unchecked.

comment:12 in reply to: ↑ 3 miqrogroove4 years ago

Replying to hakre:

/wordpress-trunk/wp-admin/admin.php?page=menu_test.php gives me a WP_Die

Looks like that test was bogus. It doesn't match anything in the plugin.

+1 to reopen this ticket instead of making a new one.

comment:13 follow-up: miqrogroove4 years ago

Wow it's happening with some roles and not others. Isn't that something...

comment:14 in reply to: ↑ 13 nacin4 years ago

Replying to miqrogroove:

Wow it's happening with some roles and not others. Isn't that something...

What you might be noticing is this. On an admin user:

var_dump( current_user_can('administrator') ); // bool(true)
var_dump( current_user_can('editor') ); // bool(false)

Which seems to makes sense, as we don't compare the caps assigned to two roles in core.

comment:15 miqrogroove4 years ago

Okay I'm starting to see significant differences in the test cases, so for the sake of clarity, here's the separate ticket:

#11922

Enjoy!

Note: See TracTickets for help on using tickets.