Make WordPress Core

Opened 9 years ago

Closed 6 years ago

#31518 closed defect (bug) (wontfix)

WP_User::has_cap and 'map_meta_cap' filter

Reported by: dugi-digitaly's profile dugi digitaly Owned by: johnbillion's profile johnbillion
Milestone: Priority: normal
Severity: normal Version: 2.0
Component: Role/Capability Keywords: needs-patch has-unit-tests early
Focuses: Cc:

Description (last modified by johnbillion)

add_filter('map_meta_cap', function(){return array();}, 1,0 ); //<-backdoor virus or any plugin
var_dump( user_can( $admin_user_id = 1, 'unavailable cap' ) ); //return true
var_dump( user_can( $Subscriber_user_id = 3, 'remove_users' ) ); //return true

The alternative I propose:
insert if(!in_array($cap,$caps)) return false; inside WP_User::has_cap( $cap ) after 'map_meta_cap' filter
OR
insert if(empty((array)$caps)) return false; inside WP_User::has_cap( $cap ) before the foreach

https://core.trac.wordpress.org/browser/tags/4.1.1/src/wp-includes/capabilities.php#L965

https://www.diffchecker.com/9cjznf39

Attachments (2)

31518.diff (1013 bytes) - added by swissspidy 9 years ago.
31518.tests.patch (1.9 KB) - added by johnbillion 8 years ago.

Download all attachments as: .zip

Change History (12)

@swissspidy
9 years ago

#1 follow-up: @swissspidy
9 years ago

  • Component changed from Users to Role/Capability
  • Keywords dev-feedback added

The attached patch would solve this. All unit tests related to capabilities still pass.

There must be a reason WP_User::has_cap returns true by default though...

#2 @johnbillion
9 years ago

  • Description modified (diff)

#3 @swissspidy
9 years ago

  • Keywords has-patch added

#4 @johnbillion
9 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to johnbillion
  • Status changed from new to reviewing

This'll need security review and tests adding to Tests_User_Capabilities.

#5 in reply to: ↑ 1 @johnbillion
9 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed

Replying to swissspidy:

There must be a reason WP_User::has_cap returns true by default though...

Turns out there is. 31518.diff breaks Tests_User_Capabilities::test_multisite_user_can_edit_self(). The reason for this is this logic in map_meta_cap() which does not add a primitive capability to the $caps array, resulting in it being empty, which results in WP_User::has_cap() now returning false instead of true.

exist is probably the most appropriate capability to require here.

It looks like there are two further conditions inside map_meta_cap() which can result in the $caps array being empty (when a break statement occurs when $caps hasn't been populated):

  • (delete|edit)_(post|page} on your own trashed post which was never published (ref/ref). Fixed in [35747].
  • (edit|delete|add)_post_meta when an auth_post_meta_{$meta_key} filter is in place and returns true (ref). Invalid.

These will all need fixes and/or unit test coverage before this can go in.

Last edited 7 years ago by johnbillion (previous) (diff)

#6 @dugi digitaly
9 years ago

I'm glad my discovery gained attention.
Good work guys.

#7 @johnbillion
8 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#8 @johnbillion
7 years ago

In 38860:

Role/Capability: Improve the test which asserts that a user can edit their own profile.

All users can edit their own profile, as this ability is not linked to an explicit capability. Technically, it should map to the exist capability, which will be addressed at a later date.

See #31518

#9 @johnbillion
7 years ago

  • Keywords early added
  • Status changed from reviewing to accepted

#10 @johnbillion
6 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from accepted to closed

I believe this change has too high a chance to break something that is relying on the user cap check returning true when the cap check is an empty array. If some malicious code can affect the return value of the map_meta_cap filter, then it can effectively allow anything on the site anyway.

Thanks for the report @dugi digitaly. It lead to a few improvements in the core code and in the unit tests.

Note: See TracTickets for help on using tickets.