Add role filtering to user editing code to secure edit_users capabiltity (security)

[jeremyclarke]

For history see: #6014

I'm updating that patch so it can be added to 2.8, but i'm splitting up the various parts so they can be added more easily.

Part 1 was #8760 (fix display of checkboxes on user listing), now commited. Part 2 was #8761 (add filter to wp_dropdown_roles), now committed. #8764 is another important patch that changes the user editing page (user-edit.php) to use the wp_dropdown_roles() function instead of duplicating its effect.

What I want (same as #8760): To add security to the capabilities system because right now edit_users can't be delegated to non-admins (in our case our content editors). If someone has 'edit_users' they can make themself admin because nothing stops them from editing themselves or others to be admin. I think it should be integrated into core but don't care enough to fight. It can be done with a plugin so my priority is to make sure that my plugin (and Role Manager plugin) can hook into the appropriate places and add a role comparison such that wp only lets people edit users/roles "lower" than them (i.e. users that don't have any powers that the editor don't have).

This is hopefully the final ticket to make the system safe and secure (or make it so a plugin can secure the system).

In the patch to wp-admin/includes/user.php I make several changes

• add a new function get_editable_roles() : This matches similar functions like get_editable_user_ids() and serves to centralize the use of the new filter 'editable_roles'. This function should be used any time a role is going to be or is about to be changed. I use it in the updated wp_dropdown_roles() to filter the list of roles before printing them out as <option> elements. It also needs to be used in functions like edit_user(), where it returns a list of roles that the logged-in user is allowed to edit, thus giving an opportunity to stop the edit if the new role is innapropriate (i.e. stop an 'author' or someone similar from crafting a malicious $_POST request to get around the limitation)

• add the new function and a check to edit_user() and add_user() so that those functions die if the roles are innapropriate

• fixed, added etc phpdoc for functions i worked on.

In the patch to wp-admin/includes/template.php:

• I edit the wp_dropdown_roles() function to use the new get_editable_roles() function (instead of running the filter manually), as well as changing the format a bit to use the $wp_roles->roles array instead of the $wp_roles->role_names one (i figure we might as well pass in the more detailed version for the filter, as the effect is the same anyway and the filter didn't exist before).

• Note: When my first changes to wp_dropdown_roles were committed it used a filter called "role_names_listing". This has been completely replaced with the more generic filter "editable_roles", which is used in get_editable_roles. No mention of the old label should exist. This is just mentioned here in case someone gets confused about the previous patch. I'm sure no one will start using the old filter between these two patches (we're in very early 2.8 right now).

Finally, my patch for /wp-admin/users.php (the user listing page) :

• I added a call to get_editable_roles and a check against them in the section that handles bulk user role changes (i.e. using checkboxes and the pulldowns). This should be useful to stop any malicious $_POST requests that try to get around the already limited pulldown.

• I also removed a redundant check to "if ( !current_user_can('edit_users') )" because a few lines later we do a more specific check to "current_user_can('edit_user', $id)", which will kill the process anyway if hte logged-in user can't 'edit_users'.

So these three patches need to be integrated as well as the one in #8764, then I will leave you alone :)

Feedback welcome.

[jeremyclarke]

update wp-admin/includes/user.php with get_editable_roles()

[jeremyclarke]

patch wp-admin/includes/template.php fix wp_dropdown_roles()

[jeremyclarke]

patch wp-admin/users.php to check editable_roles before saving bulk role changes

[jeremyclarke]

Oh yeah, to see the effects of these patches (which are only relevant if you have a user with 'edit_users' but who isn't an admin (doesnt' have all other privileges), you also need to have the following plugin code running somewhere (updated since the previous tickets to use the new filter name):

​http://www.pastie.org/349868

You can use the Role Manager plugin (which will hopefully have that code integrated) to set up a user who is an author or editor with the edit_users capability.

[ryan]

Should get_editable_roles() return an empty array if the user can't edit_users? The change to wp-admin/users.php seems like it would allow promoting to any role even without edit_users.

[jeremyclarke]

re: empty array from get_editable_roles() - I don't think this is necessary because any situation where a user is being edited already has a check in it to make sure. In fact I think by the time you've edited a user the current_user_can('edit_users') has been run many many times (whcih is good because it avoids various sneaky attacks using $_POST). In all the cases I saw it was very well established that the user can edit_users, both in the processing of $_POST and before even displaying the ui elements needed to initiate a user edit.

re: wp-admin/users.php changes allowing edits - the patch is deceptive, if you look just below the changes in the actual file you see that there are specific checks to current_user_can('edit_user', $id), which will return false if just 'edit_users' was false, and goes even further to ensure that each specific user is editable. I just removed the plain edit_users check because it was redundant and would have some miniscule effect on performance. If that makes you nervous please just undo that change and keep the rest.

[ryan]

Replying to jeremyclarke:

re: empty array from get_editable_roles() - I don't think this is necessary because any situation where a user is being edited already has a check in it to make sure. In fact I think by the time you've edited a user the current_user_can('edit_users') has been run many many times (whcih is good because it avoids various sneaky attacks using $_POST). In all the cases I saw it was very well established that the user can edit_users, both in the processing of $_POST and before even displaying the ui elements needed to initiate a user edit.

The phpdoc for get_editable_roles() is incorrect, however, if it always returns a full set of roles regardless of user.

re: wp-admin/users.php changes allowing edits - the patch is deceptive, if you look just below the changes in the actual file you see that there are specific checks to current_user_can('edit_user', $id), which will return false if just 'edit_users' was false, and goes even further to ensure that each specific user is editable. I just removed the plain edit_users check because it was redundant and would have some miniscule effect on performance. If that makes you nervous please just undo that change and keep the rest.

Okay, sounds like we're fine.

[ryan]

(In [10323]) Add get_editable_roles() and role filtering. Props jeremyclarke. fixes #8770

[hakre]

[10323] was breaking tests, reported in #11892, suggested fix provided there (add of wp_die() caused problems here.)