Add role filtering to user editing code to secure edit_users capabiltity (security)
|Reported by:||jeremyclarke||Owned by:||jeremyclarke|
|Severity:||normal||Keywords:||has-patch capabilities needs-testing|
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 :)