Make WordPress Core

Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#11922 closed defect (bug) (fixed)

Pages Hooked by add_menu_page() Have No Security

Reported by: miqrogroove's profile miqrogroove Owned by: westi's profile westi
Milestone: 2.9.2 Priority: high
Severity: normal Version:
Component: Role/Capability Keywords: has-patch
Focuses: Cc:

Description

... continued from #10310

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.

For clarity, the flow of control:

  1. In add_menu_page(), the callback function gets hooked unconditionally. $access_level is ignored.
  1. In wp-admin/menu.php, current_user_can() is called after the user has failed every submenu permissions check unanimously. If any one test passes, then current_user_can() never runs.
  1. In wp-admin/menu.php, user_can_access_admin_page() is called.
  1. user_can_access_admin_page() performs a last-ditch check for $_wp_menu_nopriv, which is never set unless current_user_can() gets called in step 2.
  1. admin.php calls do_action($page_hook);

Attachments (1)

11922-stop-gap.patch (599 bytes) - added by miqrogroove 14 years ago.
This should fix the immediate problem.

Download all attachments as: .zip

Change History (17)

@miqrogroove
14 years ago

This should fix the immediate problem.

#1 @miqrogroove
14 years ago

  • Keywords has-patch added

#2 @westi
14 years ago

  • Keywords reporter-feedback added; has-patch removed
  • Owner set to westi
  • Status changed from new to accepted

Can you give more detail on how to reproduce this as I can't reproduce it with the following test code, based on the examples in the linked ticket:

function my_hack() {
	add_menu_page('Page Title', 'Works', 'manage_options', 'add-menu-page-works', 'menu_output_func_parent' );
	add_submenu_page('add-menu-page-works', 'Test Sublevel', 'Test Sublevel', 'manage_options', 'sub-page-works', 'menu_output_func_child');
	add_menu_page('Page Title', 'Not allowed', 'bad-cap', 'add-menu-page-not-allowed', 'menu_output_func_parent' );
	add_submenu_page('add-menu-page-not-allowed', 'Test Sublevel', 'Test Sublevel', 'bad-cap', 'sub-page-not-allowed', 'menu_output_func_child');
	add_menu_page('Page Title', 'Only Child allowed', 'bad-cap', 'add-menu-page-only-child-allowed', 'menu_output_func_parent' );
	add_submenu_page('add-menu-page-only-child-allowed', 'Test Sublevel', 'Test Sublevel', 'manage_options', 'sub-page-only-child-allowed', 'menu_output_func_child');
}
add_action('admin_menu','my_hack');

function menu_output_func_parent() {
	echo 'Hello Parent World!';
}

function menu_output_func_child() {
	echo 'Hello Child World!';
}

Code was placed in a file in mu-plugins to get auto loaded.

#3 @scribu
14 years ago

  • Keywords has-patch added

#4 @miqrogroove
14 years ago

  • Keywords reporter-feedback removed

#5 follow-up: @miqrogroove
14 years ago

At step 2 one of the tests must pass. So if you changed your first submenu $access_level to 'read' then any user would be able to trigger the parent hook, even though it's still set to 'manage_options'. Also test the page= query on different php files to see how $pagenow is ignored.

#6 in reply to: ↑ 5 @westi
14 years ago

Replying to miqrogroove:

At step 2 one of the tests must pass. So if you changed your first submenu $access_level to 'read' then any user would be able to trigger the parent hook, even though it's still set to 'manage_options'. Also test the page= query on different php files to see how $pagenow is ignored.

I have done this although this and still can't reproduce the issue.

As far as I can tell this is the same as the third example in my tests but you have to be a non-admin to test.

The test case you have is Parent Menu requires a cap we don't have but child requires one we do.

This is the first piece of code which runs after plugins add menus:
http://core.trac.wordpress.org/browser/branches/2.9/wp-admin/menu.php#L200

This strips out Top Level menus which are not accessible and have no accessible children.

So for a Lowest level user this strips out the 2nd and 3rd menus in my updated example where I have changed the first submenu to 'read' and leaves only the first.

Ok reading back carefully through the referenced ticket the missing clue is how the page is accessed index.php rather than admin.php

#7 @westi
14 years ago

(In [12737]) Ensure that inaccessible parent menus are marked as such even if they have accessible children. See #11922.

#8 @westi
14 years ago

That fixes it in my testing.

Feedback welcome.

#9 @westi
14 years ago

(In [12738]) Ensure that inaccessible parent menus are marked as such even if they have accessible children. See #11922.

#10 @miqrogroove
14 years ago

If there's no momentum for checking the callback hook, then let's call this fixed. (?) I added a permissions check to every public function in my own plugins, and that's good enough for me!

#11 @ryan
14 years ago

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

#12 @westi
14 years ago

I'm going to put in the extra cap check just for safety sake and also port this to the 2.8 branch in case we do any updates to that.

I'll also improve the phpdoc to make these functions clearer to use and publicise best practise.

#13 @westi
14 years ago

(In [12891]) Don't both hooking in menu functions which will never get called. See #11922 props miqrogroove.

#14 @westi
14 years ago

  • Severity changed from critical to normal

Moving to normal as this isn't a common use case and plugins should always check caps too.

#15 @westi
14 years ago

(In [12892]) Don't both hooking in menu functions which will never get called. See #11922 props miqrogroove for 2.9 branch.

#16 @westi
14 years ago

(In [12893]) Menu hardening for the 2.8 branch see #11922

Note: See TracTickets for help on using tickets.