Security: The "administer users" permission exposes user/1

This is not only not neccessary it also doesn't make much sense. There might be multiple users which have all permissions on a site. A usefull way to prevent tampering with these accounts would be to only allow people who have some permission to edit all these accounts.

Agreed.

@killes: This isn't as much intended to prevent an arbitrary user from taking over an arbitrary user as it is to ensure that UID 1 remains the site owner's "trump card". Yes, other people can self-escalate, but only UID 1 is UID 1, and only UID 1 can change UID 1.

http://drupal.org/node/46149 is about deletion of user #1. This issue is about editing.

When I first encountered this I was stupefied this is part of core. As has been said before the administer users permission allows anyone to change the password of user #1, effectively allowing "root access" to the site. In my opinion this renders the administer users permission pretty useless. Without any mention of this "feature" in the handbooks it's a security hole.

So, what to do? Block editing of user #1? Require supplying the current password when changing it? (Shouldn't we do this for all users?)

This is a very important issue, as anyone with the "administer users" permission can take super user access (user/1). The attached patch makes it so that users with "administer users" cannot edit user/1. Only user/1 can edit the super user account.

I'd like to see less special-casing of user/1 rather than more.
What about something like "administer $role users" permissions (a bit like content types)? That'd allow for proper delegation of user administration, and protect 'admin users' alongside user/1.

Plenty of sites have admin roles where they can do everything user/1 can (except running update.php which I think is hardcoded to user/1) - and the patch as it stands doesn't offer protection to those roles. Giving someone the php filter permission allows them to do anything they like in your database too - but I don't think we should special case that module to be only enabled by user/1 or similar.

Well if you had an admin role, and put user/1 in that admin role - then you'd have to give users the "administer admin users" permission in order for user/1 to be editable going by #13. I'd be in favour of an admin role with user/1 being placed into it as part of the default install profile, or something similar, in fact it'd be quite nice to get rid of the whole idea of user/1 if there's a clean way to do this.

Various combinations of administer input formats, administer permissions and administer modules can let you escalate yourself to a situation where 'DELETE FROM {users};' is just a couple of googles away for any moderately competent malicious user. In fact that'd be easier than escalating yourself to user/1.

So if we're going to start special casing permissions then this'll need to be done in a lot more places, otherwise it's gives a false sense of security. And if we do that, then http://drupal.org/node/248598 needs some more consideration.

Related: #251370: Require current password before changing it

subscribe.

What a lifesaver! now I can open 'administer users' permission to PR guys, it was a nightmare to add new hired to the system every week by myself.

The last submitted patch failed testing.

So the general conception is that we should split the "administer users" role into "administer %role users"? If this is the case:

1. What happens for users that are part of multiple roles? Do the credentials have to match all roles that they are part of?
2. What happens at admin/user/user? Since there is no more "administer users" role, the listing of users page has to have a new permission.
3. What about deleting users?
4. What about creating new users?

Require supplying the current password when changing it? (Shouldn't we do this for all users?)

Amen to that. Would have greatly simplified node/8 (canceling own user account).

The same way we need to special case uid 1 to prevent its deletion (via UI), we need to special case editing its password. However, with regard to aforementioned, badly needed check for the current password, we want to unconditionally expose the "current password" field for uid 1.

@ newbuntu -
I've done the same - hence #24 question & BTW see fixed below

Your "regular admin" can't change his or anyone else's perms without you granting him "administer permissions" even if you do grant him "administer users" - it's currently NOT "all or nothing".

However, if you want the "regular admin" (or others) TO be able to grant others "some" permissions, you can do that by creating roles (like your "regular admin" role), assign those roles perms, and use Role_Assign module and allow specific roles assignable (or not) by those w/ "administer users" AND "assign roles" perms.

#24 question fixed (afford same protection given uid1 to "almost"1 [#3 in my case]) w/ changing user.module / function user_edit_access from:

@ahoeben - I think we're both restating the basic issue but in different ways

In #29 I'm telling newbuntu that u1 edit protection (by hack supplied [which includes "psuedo-1" protection for his case & mine] or other patches et al which effect same) makes "administer users" NOT "all or nothing", because "administer users" does not include "administer permissions".

Thanks for taking the time to report this issue.

However, marking as duplicate of #86299: Add "current password" field to "change password form".
You can follow up on that issue to track its status instead. If any information from this issue is missing in the other issue, please make sure you provide it over there.

#12: no-edit-admin-39636-3.patch queued for re-testing.

patch does not apply