Hook needed on wp_dropdown_roles() to secure 'edit_users' capability (see last comment)

[jeremyclarke]

This is only relevant if you are using the 'role manager' plugin to modify caps, but if a role or user is given the 'edit_users' capability using role manager, they are able to change the roles of existing users to anything they want, even if the new role is above their user level.

Thus an 'editor' who is given the edit_users cap so they can create new user accounts for new 'authors' is able to make themselves or anyone else and 'administrator' (or assign individual capabilities such that the same effect is achieved).

This is obviously disastrous from a security perspective, and after having our system compromised this bug was exploited by our attackers to create new admin users (and promote inactive accounts to administrator status) for their various nefarious purposes.

The expected behavior, IMHO, is that a user can alter other users to NO HIGHER than their current level, and simililarly that they can not add any capability to a user that they do not have themselves.

On a deeper level, it seems like they should only be able to assign roles and capabilities BELOW their current level (e.g. an editor could create and modify 'authors', but not editors or admins). However I understand that the intricacies of controlling priviliges at that level is so complicated it's probably not worht attempting.

A basic fix would need to alter the user editing scripts such that:

• when loading the user profile edit page it checks the privileges of the logged-in user
• if the edited user has a role ABOVE the logged-in user the logged-in user just gets an error (they should not see the edit link on the listing screen in the first place).
• the list of roles and capabilities displayed to the logged-in user are truncated to only show those that the user already has access to themselves.
• on execution, the script checks that the logged-in user has correct priviliges to be making that change to the privileges/role of the edited user.

Let me know if this is a duplicate or something. I searched and coudlnt' find anything about this problem.

[pishmishy]

I'm not sure this can be fixed as you describe. It requires roles to have a clear ordering imposed on them, that Administrator > Editor > ... > Subscriber. This is something that we're trying to move away from as we transition from access levels to roles and capabilities. What may seem like an intuitive ordering of roles may not be expected by others (especially those using this "role manager" plugin).

My recommendation is that any tool which allows the assigning of the edit_user capability to a user, or role, to make the consequences of that action very clear. The documentation in the codex should also make this clear.

I hope that doesn't sound like I'm brushing aside the issue but I'm reluctant to consider a solution that looks at particular roles as being higher or lower than others.

[jeremyclarke]

Hmmm, while writing this my thinking was that the user_level's, still being in use, could be used to create the hierarchy. If you're saying that the goal is to 100% stop using user levels then I understand how that complicates any possible solution.

Here's another option, what if users editing other users cannot give them any role that has a capability that they do not have, nor can they add capabilities that they themselves to do not have. It would stop the negative behavior I described without affecting the ability of middle-admins to take a load off of the administrators in managing users (with the ultimate goal of limiting the number of admin accounts in the system to those people who actually need to modify options and plugins etc. which has inherent security benefits)

One thing that I think is important to remember is that in almost all situations these changes will have no effect. For installs where only admins have edit_users privileges they will always qualify to make whatever edits they want.

I'm open to the idea that this is an innapropriate goal for wordpress to aim at (Because WP "insn't a cms" or something, which I don't believe but recognize is an opinion some people have), but I think that it is definitely possible within the system, and the whole existence of the edit_users capability implies that it should work for users that don't nevessarily have root control of the system. Also, the kind of documentation you're describing would be very hard to implement, and would involve forcing everyone already using the plugin to read the new documentation (as opposed to having the problem automatically fixed behind the scenes when they upgrade to the latest wp).

Thanks for showing interest in the ticket pishmishy.

[pishmishy]

Replying to jeremyclarke:

Hmmm, while writing this my thinking was that the user_level's, still being in use, could be used to create the hierarchy. If you're saying that the goal is to 100% stop using user levels then I understand how that complicates any possible solution.

I can't speak on behalf of the core developers, and so can't be certain, but tracking the development of access control in WordPress it's the direction that WordPress is taking.

One thing that I think is important to remember is that in almost all situations these changes will have no effect. For installs where only admins have edit_users privileges they will always qualify to make whatever edits they want.

Now we're considering rare edge cases. We could also end up with weird situations whereby a user A with edit_users capabilities but without capability "x" could give edit_users capabilities to a user B with capability "x" who would then give capability "x" back to A. It's almost certainly not a situation that the administrator intended, and not intuitive.

Also, the kind of documentation you're describing would be very hard to implement, and would involve forcing everyone already using the plugin to read the new documentation (as opposed to having the problem automatically fixed behind the scenes when they upgrade to the latest wp).

Again, I don't wish sound as if I'm dismissing the problem, but this really is an issue for the plugin's author. With direct access to the database, plugins have the ability to do lots of really stupid things if they chose to, and we don't (can't?) spend time worrying about them.

[jeremyclarke]

Replying to pishmishy:

Now we're considering rare edge cases. We could also end up with weird situations whereby a user A with edit_users capabilities but without capability "x" could give edit_users capabilities to a user B with capability "x" who would then give capability "x" back to A. It's almost certainly not a situation that the administrator intended, and not intuitive.

This is a valid edge case where my system doesn't behave exactly as expected, but it is just that, an edge case, and an extreme one at that. A responsible administrator would ensure that no one has permissions that the edit_users-ified users weren't allowed to have (like say "manage_plugins" or "manage_options", which in a system with many users are likely only needed by a small handfull). It is so unlikely to happen that I can't take it very seriously actually, and if it did happen the administrator could fairly easily fix the situation by modifying the existing permissions to be hierarchically consistent.

My personal edge-case on the other hand is not nearly as extreme. All I'm asking is that an administrator be able to allow non-admins to create and edit users of a role lower than theirs. This is not a controversial behavior or an unexpected one, it's a completely normal thing to want, but right now it is impossible to do within the existing permissions structure.

Again, I don't wish sound as if I'm dismissing the problem, but this really is an issue for the plugin's author. With direct access to the database, plugins have the ability to do lots of really stupid things if they chose to, and we don't (can't?) spend time worrying about them.

I think your comment about direct DB access points to the problem here. The plugin at hand isn't accessing the DB, or anything like that at all, it is using completely standard API calls to manipulate the system capabilities in exactly the way envisioned by the coders when they designed the system. The problem is that this one specific capability does not perform in the way that an API user would expect it to.

I guess that an API hook could potentially solve this problem by allowing the Role Manager (or other) plugin to filter the list of capabilities/roles whenever it is generated for use in modifying users, but there isnt' any such hook at the moment, at least not in the edit-users.php file (which generates the normal user editing screen with the pulldown for roles).

Actually, looking at the code it seems the function wp_dropdown_roles() is used in all situations EXCEPT the edit-users page. If this funciton was modified to work on that page and an api filter on the list of roles was added then the plugin could easily do this filtering for itself in all places where the role dropdown is visible.

That still wouldn't have the desired effect of avoiding edit_users privileged people from editing the passwords/information of administrators maliciously, but it would at least avoid them giving themselves access to plugins/options etc.

I will try to write a patch to add the filter to wp_dropdown_roles() if I can. If anyone is reading this and has advice/recommendations please feel free to lay them on me. Would 'dropdown_roles' be the best name for it?

[pishmishy]

Replying to jeremyclarke:

My personal edge-case on the other hand is not nearly as extreme. All I'm asking is that an administrator be able to allow non-admins to create and edit users of a role lower than theirs. This is not a controversial behavior or an unexpected one, it's a completely normal thing to want, but right now it is impossible to do within the existing permissions structure.

It's still got this ordering and an idea of an ordering existing on roles. I'm pretty sure we could create a pluggable function to define the ordering - so that it could be changed if necessary, but I'm not sure that everyone would want, or could impose an order, on their ideas of what the roles should be.

How about splitting the edit_users capability into edit_users and edit_roles ?

[jeremyclarke]

Actually, the role manager plugin already has a 'manage roles' cap in the newest version (which i didn't have yet), so you can have only admins be able to individually modify capabilities or modify roles globally, which makes sense.

Looking into the situation I found that one can control the display of the edit links on the users.php screen using a filter on user_has_cap, which is run near the end of current_user_can. It turns out that when it prints the user editing links (on their username in 2.5) it actually does the check with the edited user's id, i.e.

 if (current_user_can('edit_user', $user_id)) {}

As of now, the only thing it does with the user_id is make sure it's not the same as the logged in user (so that you dont "edit yourself" but instead "modify your profile"). I coded up an example plugin that hooks into that filter and returns false if the edited user has any capabilities that the logged-in user doesnt (as I described above). I think your'e right that this might as well be part of the plugin (i'm going to get in touch with the current maintainer about it).

The one change that still needs to happen in core is to the wp_dropdown_roles() function, which needs to have a filter installed on the $wp_roles->role_names so that innapropriate ones can be removed by a plugin (In my example plugin I compare all caps as described above but you could have an option or code if differently if you wanted). It's not quite done but here's the function I have so far if anyone has input (from /wp-admin/includes/template.php ~line 900 ):

function wp_dropdown_roles( $default = false ) {
	global $wp_roles;
	
	// filter the roles to remove ones the logged-in user shouldn't
	// be able to apply to others, or whatever other filters people
	// might want.	
	$filtered_roles = apply_filters('wp_role_listing', $wp_roles);
	
	$r = '';
	foreach( $filtered_roles->role_names as $role => $name ) {
		$name = translate_with_context($name);
		if ( $default == $role ) // Make default first in list
			$p = "\n\t<option selected='selected' value='$role'>$name</option>";
		else
			$r .= "\n\t<option value='$role'>$name</option>";
	}
	echo $p . $r;
}

The other thing I still have to do is replace the dropdown menu which is hardcoded in user-edit.php with one generated by the plugin above so that it can be filtered also.

[jeremyclarke]

Okay! I got everything working. It requires edits to 2 files in 3 places total.

1) in wp-admin/includes/template.php

function user_row() - This function generates the rows of the table seen on users.php, it was already checking if the logged-in user had privs to edit that row's user. I added an extra variable based on that check which adds disabled=true to the checkbox if the row's user is not editable. This has no effect on anything unless a plugin hooks into the user_has_cap filter (so it won't hurt normal users), but it allows plugin authors (i.e. Role Manager) to disable the checkboxes when necessary (rather than throwing errors if you try to modify someone you dont' have privs to modify using the checkboxes and pulldowns at the bottom).

2) in wp-admin/includes/template.php

function wp_dropdown_roles() - This function was generating the role lists for the top and bottom of users.php, I modified it to add a filter on the role_names (as well as making a copy of it from $wp_roles rather than using it directly, so that plugins can unset elements in the copy (to hide those list items) without removing them from the actual $wp_roles object). I also changed the name of the function's argument to 'selected' rather than default. The old name was because on users.php the 'default' new role was set as selected by default, but when used on user_edit.php the selected option should be the actual role of the user you're editing, so the new argument name reflects what the value will actually do.

3) in wp-admin/user-edit.php

The main reason for changing this file was to integrade the wp_dropdown_roles() function rather than having it print the roles from scratch into the <select>. As a result I removed several lines that were no longer necessary and reworked a few lines so that they made more sense with the wp_dropdown_roles() function. I left several comments and some of the old code commented out in case someone wants to check that everything makes sense.

I have tested the changes with an SVN copy of wp2.5 and the newest version of the Role Manager plugin. Between the new hook on wp_dropdown_roles() and some hooks on the user_has_cap filter I was able to secure the user editing interface so that users can only edit other users at or below their level (and can't promote users or themselves to any role or capability that they do not have).

Any feedback is appreciated and please let me know if there are problems with this. I think this should definitely be part of core (especially becaue I avoided actually putting any of the user comparison code into the patch, so WP remains pure and hierarchy-less). Thanks in advance to anyone who tests this.

[I am also attatching the sample plugin I wrote for hooking into this and adding the security to Role Manager. To see the effect you will have to install Role Manager, give the 'edit_users' privilege to a user or role that is not administrator, and turn on this extra plugin called "Role Manager Security Core Patch Example Plugin". At that point you'll see that the lower user is unable to edit administrators or anyone above their level, and won't see administrator in role dropdowns on user_edit.php or users.php]

[jeremyclarke]

the patch to core (with comments)

[jeremyclarke]

example plugin addition to role-manager

[dpe415]

After talking with Jeremy Clarke about the functionality he discuses in this article, it would appear that I've found a related issue with WP roles & creating users. below are the details of an email I sent to Jeremy, outline the issue I've discovered:

---

I'm running WP 2.3.3 and the Role Manager plugin version 2.2.1. With Role Manager, I have given Editors the capability to Edit users, but not create new ones or delete existing ones. Plainly stated, the delete restriction works great. But no matter what I try Editors can still add new users of any Role including Administrator (which defeats the whole point of having separate Roles--an Editor can create their own Administrator account!).

I’ve dug into the WP core code a bit and it appears to me that the check that prevents Editors from being deleted is located in \wp-admin\users.php at or around line 161 and includes this:

      if ( !current_user_can('delete_users') )
            wp_die(__('You can’t delete users.'));

When an Editor tries to delete any user, they end up getting being told that they can't delete users. Perfect. Everything functions great.

The same check is (supposedly) performed when adding users as well. It is located at or around line 257 and includes this:

      if ( !current_user_can('create_users') )
            wp_die(__('You can’t create users.'));

However, this check never seems to fire when an Editor tries to create a user. It would seem that the create_users capability check isn't functioning properly. Which allows Editors to create users even though they don't have that capability assigned to them.

[dpe415]

Arg! Sorry...I mentioned this issue to the creator of the Role Manager plugin and he has created a new Trac Ticket here:​http://trac.wordpress.org/ticket/6662.

​(View my comment to the Role Manager website)

Please ignore my previous comment. Sorry for any confusion or trouble.

[CrazySerb]

Jeremy, that sure fixed the problem.

And I say yes, this should definitely be a part of the WP core in some way, shape or form.

[dpe415]

Replying to pishmishy:
@pishmishy: Just curious, why am I now the owner of this ticket, and why did you change it's status to new?

[pishmishy]

I figured that since you were doing most of the investigation since I took a little break from trac, you may as well take ownership. I hope you didn't mind that. I believe the status resets to new until a new owner accepts the ticket.

[dpe415]

Replying to pishmishy:
Actually, jeremyclarke did all the work for this ticket, I only mentioned another possible issue connected to it, and then quickly realized that my issue had been added & resolved as a new ticket. See comment 10 & 11 above.

[jeremyclarke]

ha ha, yeah, I wrote the patch and stuff. So now me and CrazySerb have tested it and found it to work. If anything is needed to get this integrated please let me know and I will do it, basically just waiting for feedback from one of the core devs.

[CrazySerb]

This patch has been made almost unusable by the new 2.6.2 version of wordpress, as now the higher level users cannot edit any other users aside from themselves... what changed?

[pishmishy]

Please you clarify what you mean by 'higher level users' in precise terms. It may just be that there's some confusion as to which roles can do what (people seem to have different expectations).

[jeremyclarke]

The problem is probably just that the patch was written for 2.5 and the user management page may have changed. CrazySerb, are you also running my old patch to Role Manager? Cause it could be broken by now. This patch doesn't actually fix anything without also using a plugin to hook in and add the actual filtering logic, so you must be using something else if you notice any difference from a vanilla install.

I actually never implemented this on my own live site because I was waiting for this patch to be implemented, UGH!

If a core dev would come by and confirm that my concept is acceptable I'll recheck my patch and make sure it works in 2.7 so that we can at least depend on it working there.

[jeremyclarke]

I'm closing this ticket because it has been replaced by a new, 2.8-ready set. see #8764