This very simple patch makes it so that users other than uid 1 cannot edit edit uid 1. You may want to have several administrators able to futz with user accounts, but the admin should be protected from a cranky user.

(This was suggested by a comment in another thread, but I felt it should be a separate patch.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dries’s picture

Status: Needs review » Closed (won't fix)

I don't think this is necessary.

killes@www.drop.org’s picture

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.

ahoeben’s picture

The point is that if you let anyone who has user edit privileges edit user 1, they can change the password for user 1, log in as user 1 and gain unrestricted access to the site.

No single privilege should give any user the means to gain unrestricted access.

Crell’s picture

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.

Crell’s picture

Status: Closed (won't fix) » Closed (duplicate)

I like hunmonk's patch in this thread better: http://drupal.org/node/46149 :-)

gaele’s picture

Version: x.y.z » 6.x-dev
Category: feature » bug
Status: Closed (duplicate) » Active

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?)

jrabeemer’s picture

Gaele, speaking from a security standpoint, that (supplying current pw) would be a good security feature. But I think that deserves its own bug report/feature request.

chx’s picture

Version: 6.x-dev » 7.x-dev
Category: bug » feature
RobLoach’s picture

Subscribing... Although I want the moderators to be able to edit users, I don't want to give them root access.

RobLoach’s picture

Title: Don't let anyone else edit uid 1 » Security: The "administer users" permission exposes user/1
Assigned: Crell » Unassigned
Status: Active » Needs review
FileSize
899 bytes

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.

macgirvin’s picture

FileSize
986 bytes

Patch works just fine - updated to chase HEAD. For many people, the issue is a critical, top priority security issue which shouldn't have remained in the queue for two days, let alone almost 2 1/2 years.

catch’s picture

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.

ahoeben’s picture

I think we need both; noone should have the ability to break into the super-user account by resetting its name/password; the super-user account is much too powerful for that. More granular user administration would be nice to have.

catch’s picture

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.

Crell’s picture

+1 to catch's suggestion in #13, as more fine-grained permissions are always a good thing. That still doesn't solve the problem of uid 1 still being editable, though, since uid 1 may still be in a role. If we ever introduce a "wheel" group for admins, then I'd give them all special treatment, not remove special treatment.

catch’s picture

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.

macgirvin’s picture

A very common configuration is to allow 'staff' members of an organization permission to manage the daily operations of a site, whilst leaving the critical settings (which could mess things up completely) protected. In this regard, we mostly have the ability now to set things up in a manner that works and prevents somebody with 'some' privileges from blowing the site away completely. There's rarely an organizational need to delegate the administer filters or even the administer modules permissions once the site is set up and running smoothly.

However, administering users is one of the primary tasks we (in this case technical operations and IT personnel) wish to delegate to non-technical admin staff - and this doesn't work very well in the current model because it's basically all or nothing. I'm personally ambivalent about whether this is based on user/1; but it's fairly trivial to make this single level of delegation work and seems like the easiest solution short of a general permissions overhaul that would probably be overkill for the vast majority of sites.

gaele’s picture

David_Rothstein’s picture

This patch seems like it may go a bit too far. Not being able to edit the email address and password is one thing, but if I'm not mistaken, this would also prevent much more benign things (like signature and picture) from being edited too. Is that really the desired behavior?

In the long run, however, I agree that breaking "administer users" into more fine-grained permissions is the real solution here...

timoratd’s picture

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.

Dave Reid’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

rhimes’s picture

+million for this patch
just discovered this thread & IMO should be a security update - thanks all!

now a question - I've a dist. profile which creates a "psuedo-user/1" with almost all permissions which needs the security/protection this patch affords a "real" #1 - how do I add that (code syntax) for user/3 - which my "psuedo #1" will always be?

Thanks in advance

RobLoach’s picture

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?
Crell’s picture

Relevant related discussion: #381584: Hierarchical Permissions System

sun’s picture

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’s picture

For my application, I want to have "super admin" and "regular admin" (I created a role for that)

I want the "regular admin" to have most permissions like the "super admin" with a few exceptions, such as administer modules and php filter access.

However, if I grant "administer users" to a "regular admin" or anybody, then he can change his own permissions to anything he wants. The problem is "administer users" permission is an all-or-nothing option.

I think the permission control should use a policy somewhat like this: a user CANNOT grant more permissions to anyone (include himself) than what his "superior" has granted him.

I understand "superior" implies a hiarchical chain. If we don't want to have a full hirachy, we should have at least two levels: Super Admin and everyone else - except "super admin", no one should be able to grant a permission to someone, if the former doesn't have that permission in the first place.

rhimes’s picture

@ 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:

<?php
  return (($GLOBALS['user']->uid == $account->uid) || user_access('administer users')) && $account->uid > 0;
?>

to:

<?php
  return (($GLOBALS['user']->uid == $account->uid) || (user_access('administer users') && $account->uid != 1 && $account->uid != 3)) && $account->uid > 0;
?>
ahoeben’s picture

> 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".

AFAIK, it is all or nothing; anyone with "administer users" permission can change the password for user 1. And once you have access to the user 1 account, you have access to everything.

rhimes’s picture

@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".

newbuntu’s picture

@rhimes, #31

I misspoke when I said "administer users" was all-or-nothing. I meant "administer permissions" was all-or-nothing. I came up with a simple idea that I believe can fundamentally alter how drupal manages permission control. Please take a look at http://drupal.org/node/491732. Feedback will be appreciated!

Coupled with this patch, I believe my proposed change creates a quasi-hierarchical permission management scheme for drupal, where super admin (uid=1) can grant "administer permissions" easily to "regular admin" without worrying about losing total control.

sun’s picture

Status: Needs work » Closed (duplicate)

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.

David_Rothstein’s picture

Status: Closed (duplicate) » Needs work

I don't think this is actually a duplicate. See my comment at http://drupal.org/node/86299#comment-2201598 for why.

sourcesoft’s picture

Status: Needs work » Needs review

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

droplet’s picture

Version: 7.x-dev » 8.x-dev
klausi’s picture

Status: Needs review » Needs work

patch does not apply

lpalgarvio’s picture

Version: 8.0.x-dev » 8.2.x-dev
Issue summary: View changes

+1

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.