[wp-trac] [WordPress Trac] #8770: Add role filtering to user editing code to secure edit_users capabiltity (security)

[WordPress Trac]

#8770: Add role filtering to user editing code to secure edit_users capabiltity
(security)
--------------------------+-------------------------------------------------
 Reporter:  jeremyclarke  |       Owner:  jeremyclarke                        
     Type:  defect (bug)  |      Status:  new                                 
 Priority:  normal        |   Milestone:  2.8                                 
Component:  Security      |     Version:                                      
 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 :)

 Feedback welcome.

-- 
Ticket URL: <http://trac.wordpress.org/ticket/8770>
WordPress Trac <http://trac.wordpress.org/>
WordPress blogging software