Pages Hooked by add_menu_page() Have No Security

[miqrogroove]

... 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.

2. 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.

3. In wp-admin/menu.php, user_can_access_admin_page() is called.

4. 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.

5. admin.php calls do_action($page_hook);

[miqrogroove]

This should fix the immediate problem.

[westi]

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.

[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.

[westi]

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

[westi]

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

[westi]

That fixes it in my testing.

Feedback welcome.

[westi]

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

[miqrogroove]

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!

[westi]

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.

[westi]

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

[westi]

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

[westi]

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

[westi]

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