[regression] User roles field access is inconsistent, users with 'administer users' permission can gain full access

Decide on who should actually have access to the field

Anything to do with roles should be controlled by administer permissions. Including CRUDL role entities, editing permissions for roles, or assigning roles to users. The account form isnt the only place that uses the correct permission to edit roles for users.

Seems like a oversight/bug.

Uploading to see what kind of things this breaks.

Not that one, this one.

Still working on it.

Security issue Users with 'administer users' permission can gain full access relates to this issue.

The vulnerability was approved to be fixed publicly since it requires a role with the somewhat trusted administer users permission.

Moved majority of security submission to issue summary, original issue summary left intact

Tests only

Comment #10 contains the patch for review.

NW for comment fix other than that this RTBC.

1. +++ b/core/modules/user/src/UserAccessControlHandler.php
   @@ -78,9 +78,10 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
   +    // Administrative users are allowed to edit and most fields.
   

   and most fields? Let's rewrite this compelety.

2. +++ b/core/modules/user/src/UserAccessControlHandler.php
   @@ -126,6 +127,8 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
          case 'roles':
   +        return AccessResult::allowedIfHasPermission($account, 'administer permissions');
   

   This is the only fix and using admin field totally makes sense here.

#15 should we also update the form element on the account form so that it's actually invoking the access control handler?

I think this makes sense. Attached a change which uses the same method the role action plugins use to determine access.

#16 and most fields? Let's rewrite this compelety.

Agreed and done.

Let's see if testbot agrees.

Random CI failure

I'm going to move it to 8.4.x, while it's security hardening, it's potentially disruptive to sites relying on the existing behaviour.

I reviewed this patch for code and test coverage and it looks solid. Thanks for untangling these permissions ... 8.4 sounds reasonable to me as well.

Yeah, it's clear to me that the permission is wrongly implemented from the summary, but "fixing" it if they are already relying on D8 behavior can lock people out of their sites. So we need to be careful about fixing it.

Also, just wanted to state explicitly that this is okay as a public issue, because the permission is already marked as a restricted admin permission.

I wonder if a PSA would be appropriate to inform people of the current behavior and the upcoming change? Or is that too big a deal for what is already an admin permission? Other than that, at least I think we will need a change record and release notes mention for 8.4.0.

Crossposted. Thanks @moshe weitzman!

First one. :)

NW for the CR, at least.

Created a change record: Required permissions to use add or remove role actions changed

Thanks @dpi for the change record draft! I updated its title to be more clear, since this is an important change users need to understand right away: Administer users permission no longer allows assigning roles.

I think the change record could also use some additional improvements for clarity. "Traditionally" doesn't make sense here. We can just say "In Drupal 7" since this is a change record. It also does not make it clear in the text what exactly was changed in the text. Also, adding headers so the user can quickly update the "before" and "after" would be helpful. What does look really good is the very clear explanation of the impact of this change.

I'll try to propose some improvements.

Alright, I did an extensive update of the change record: https://www.drupal.org/node/2853612. I removed the screenshot because it was confusing as it focused on the past loophole, when actually the CR should communicate the current state to users. I also rewrote it focusing on the before-and-after and what the site owner would need to know. We can then reference the CR in the release notes.

(Updating issue credit.)

CR changes look great, thanks!

Looks like this never got set back to RTBC for the change record review, so doing that now. Thanks @dpi.

It was the first one, but it's not one anymore. :P

So there are a couple of small things that need work in this patch (I did not do a full review in #37):

1. +++ b/core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php
   @@ -40,13 +40,20 @@ class UserAccessControlHandlerTest extends UnitTestCase {
   +   * A user with 'administer users' permission.
       *
       * @var \Drupal\Core\Session\AccountInterface
       */
      protected $admin;
   
   @@ -336,6 +362,60 @@ public function passwordAccessProvider() {
   +        'viewer' => 'admin',
   

   Since we now have two accounts, one that has admin users but not admin roles, and one that has admin roles but not admin user, I think we should probably rename the $admin property and the admin user name to be admin_users instead. Otherwise, the test is difficult to read and confusing.

2. +++ b/core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php
   @@ -336,6 +362,60 @@ public function passwordAccessProvider() {
   +  /**
   +   * Provides test data for passwordAccessProvider().
   +   */
   +  public function rolesAccessProvider() {
   

   This docblock is incorrect.

3. +++ b/core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php
   @@ -336,6 +362,60 @@ public function passwordAccessProvider() {
   +    $pass_access = array(
   ...
        );
        return $pass_access;
   

   The variable name also is a copy-and-paste name that does not fit this new access provider.

4. +++ b/core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php
   @@ -336,6 +362,60 @@ public function passwordAccessProvider() {
   +      array(
   

   All of these old-style arrays should be converted to the [] syntax.

These are all small cleanups that could be done by a novice contributor. Thanks!

Needs reroll

Straight re-roll (before addressing @xjm's comments in #40).

Addressed @xjm's comments in #40.

1. Property and username changed from 'admin' to 'admin_users'.
2. Corrected the docblock (and also the docblock for passwordAccessProvider()).
3. Updated $pass_access to $role_access.
4. Short array syntax is now used throughout.

Please could the Core experts consider another potential security aspect - at present there is a way in which this fix could reduce security.

Summary

• In the patch AccountForm::form() calls $account->roles->access().
• This leads to hook_entity_field_access being invoked based on the current roles.
• If that passes, the form allow the current user to assign any role.
• From reading the hook_entity_field_access docs, I expect the hook to be invoked with the proposed new roles. Otherwise it seems impossible for the hook to control which roles can be assigned.

Example

An example is the module Administer Users by Role (where I am a maintainer). It grants access to "sub-admin" users to

• edit users with safe roles: hook_entity_access
• assign/remove selected "safe" roles to users that currently have only safe roles: hook_entity_field_access.

In D8.4, this code is perfectly safe. The roles field is by default not present on /user/XX/edit.

When this fix is applied, this code has a security bug. The roles field is by default present on /user/XX/edit and all roles are available, including admin. The sub-admin can assign admin permission to themselves.

The fix is for the custom code to use hook_form_FORM_ID_alter to filter 'roles' to include only safe values. Fortunately "Administer Users by Role" already does this. However it seems possible that other code exists that doesn't.

Proposal

Ideally AccountForm::Form would automatically reduce the list of roles to only those allowed. However I can't see how.

Fallback is to ensure that when the form verifies, the actual assigned roles are checked again with $account->roles->access.

For a module to customise access to roles (e.g. to assign specific roles but not other roles) it needs

• hook_entity_field_access for 'roles'
• hook _form_user_form_alter for 'roles' to set #access and #options

Unless someone can see a clever way to do both parts of the second bullet automatically then I reckon the safest approach is to leave AccountForm.php unchanged in this patch. Changing #access but not #options is not safe or helpful.

@kevin.dutra Thanks for the response.

The background to #47 is in #46. In summary, I believe the current patch is insecure. Now I may be wrong and I'm quite happy to have someone explain why (but this is not just theoretical - I did an actual test). However my comment is not "an entirely different issue" - I am talking about the security of this patch. Apologies if I didn't make that clear.

I'll try and explain the scenario more clearly.

• Create a custom module that implements hook_entity_field_access to allow a user to remove all of their own roles (rough code below, but not tested).
• Log in as a user with no roles and edit own account.
• Form displays input for roles (the roles field has no roles hence the access check passes) with all roles available.
• Assign self administrator role and save.

This custom module is perfectly safe in D8.4, but not safe after the patch.

function XX_entity_field_access($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemList $items = NULL) {
  if ($field_definition->getTargetEntityTypeId() != 'user') {
    return AccessResult::neutral();
  }

  if (($field_definition->getName() == 'roles') && ($operation == 'edit') && ($account->id() ==$items->getEntity()->id())) {
   // @todo Or perhaps it needs to be count(==1) because of authenticated user role
    return AccessResult::allowedIf(empty($items->getValue()));
  }
  return AccessResult::neutral();
}

@kevin.dutra Thanks for a clear explanation. The point at which my understanding differs is

Even without this patch, if a third party module is extending access in the manner you describe, it is already allowing the exploit you've described if they can operate via the REST API, a bulk action in a View, etc.

With my hook

• The user attempts to remove their roles with bulk API, that leads to access('update, 'roles'. []) which succeeds.
• The user attempts to add admin roles with bulk API, that leads to access('update, 'roles'. ['admin']) which fails.
• The user accesses user/edit in D8.4, there is no roles field

So it's all safe. However including this patch, if the user (with no roles) accesses user/edit to add admin role, that leads to access('update, 'roles'. []), which succeeds.

The problem is as I said in #46: that the patch is calling $account->roles->access() passing the current value of role then allowing the user to assign an arbitrary new value. The correct usage of access is to pass the new value.

The key to my argument is that access check is not just "can user U access field F" but rather "can user U access field F to set value V" - where V is the $items parameter.

@kevin.dutra I am rather baffled by your comment. It's great that, as far as I understand, you now accept my argument that this patch introduces a new security vulnerability. However I can't understand why you seem to think it's fine to go ahead anyway (it feels rather dismissive of my point of view:-))

Unless I have missed something, this issue would still be solved even without the change to AccountForm - so why can't we go that way? You suggest "It reduces the number of one-offs floating around" which is a reasonable but low-priority argument - surely way, way less important than security?

I am not just commenting here to make your life difficult! I am the maintainer of Administer Users by Role. The latest D8 version has I hope now successfully achieved the goal of allowing granular assignment of permission to edit users and assign roles fully integrated with Core - using Entity Access API, the existing forms, the existing Actions and so on. It wasn't altogether easy, and has lead to some tidy up of Core #2921112: [META] User access control that is progressing towards getting committed.

I don't think the API really guarantees when the access check is going to occur

The access API has a field $items, which as far as I can see when called from REST API, Views Bulk etc. contains the new value. This gives D8 an access API where the decision can depend on the value. Surely this is a good thing - why try to throw it away? I struggle to see how the $items parameter can be used unless it has this meaning.

if someone wanted to do something along the lines of the code sample from above, it wouldn't be much more effort to throw in a quick form alter

How would someone know they needed to do that? What if another contrib module had copied the the code from Core assuming it was good code? - there would be a missing form alter for the copied code. Why make it more effort for Drupal to be secure as part of a "security hardening" patch:-)?

the change record is there

For the record, the change record does not seem to make any mention that this change makes Drupal less secure in certain scenarios - and quite rightly so. Please don't edit the CR to say that - let's change the patch instead.

Thanks for the follow up.

I don't think the current documentation around the $items parameter is sufficient

Certainly we can agree on that.

In no way do I accept that this patch introduces a security vulnerability.

Wait a mo. I thought I explained in detail a specific scenario in which this patch changes a site from being secure to not being secure, and in #52 you said "Ah, I see what you're getting at." Please can you clarify? Aha I guess that in your view the code in the specific scenario was wrong in the first place, because of the next point. Now "I see what you are getting at":-).

It's perfectly fine to make use of the field items, but you have to understand that the contents therein may be current values or proposed new values, just depending on when the access is being checked.

So this seems the key to it all. Please can you explain what evidence you have to support this? So far you have merely asserted that my interpretation is wrong and yours is right:-)

I realise you may have an overwhelming reason that I don't know about. But otherwise I don't favour that interpretation as it seems confusing and hard to use. I can't see how to control access to allow one value but not another. I can't see how to write secure code that references $items (other than to call ->getEntity() etc.). I can't see how to grant field access to 'roles' (except to a full unlimited administrator - who presumably has 'administer permissions' anyway so there is no point). Because in each case, the attacker can arrange for their old value to pass your access test, then supply an arbitrary new value.

It's a shame as it seems to prevent modules like "Administer Users by Role" from granting selective field access on 'roles' (and same for any other field where access needs to depend on value).

BTW I am already aware that $items an optional parameter, and I don't see any difficulty with that case - I'd say it's the non-NULL that we should focus on.

If we create a followup to improve that documentation, would you agree that the user form change becomes a non-issue?

Not yet - but if you convince me of the previous point then yes.

@kevin.dutra I have raised #2925042: Unclear documentation for field access - please can you read it and add your comments?

Related / duplicate of #151311: Split 'administer permissions' into a new administer roles permissions

@kevin.dutra With some help from @Berdir on #2925042: Unclear documentation for field access I have found the evidence that backs up your claim about how the access functions should be used. I no longer have any concerns from a security perspective and I withdraw my request from #46. Thanks for your patience, and I hope you will agree it's worth taking time to remove security doubts. In this case the debate has lead me to discover there was a security flaw in the alpha release of the Administer Users by Role module, so thank you.

I have one minor item of feedback on the patch

-      '#access' => $roles && $user->hasPermission('administer permissions'),
+      '#access' => $roles && $account->roles->access('edit', $user, TRUE),

Why not just $account->roles->access('edit')?

@Jo Fitzgerald's patch in #43 no longer applied to 8.6.x. A reroll with no further modifications is attached. @AdamPS's suggestion to simplify the access check has not been implemented.

changing status to allow tests last patch

• Fixed the test failure.
• Using non-deprecated code on the new test.
• Renamed the new account from the unit test to a more meaningful names.
• Fixed the coding standards failures

Also, this, being a bug is eligible for 8.5.x.

@claudiu.cristea Nice work, good to get this issue moving again. I think the review comment from #58 is still valid.

  '#access' => $roles && $user->hasPermission('administer permissions'),
+      '#access' => $roles && $account->roles->access('edit', $user, TRUE),

Why not just $account->roles->access('edit')?

However, we are facing a UX problem here. We are showing the option (eg "Add the XXXX role to the selected user(s)") to the unprivileged user but in reality he's not able to execute that operation, receiving an error message. What a flawed user experience for the 3rd millennium AD!!! Instead, additionally to what has been done so far, we should also hide the options. But this is hard to do because the action plugin access is checking the access on a per-object (entity) basis. So, it's a runtime check. We need a check that doesn't go that far, we have to check if the current user is able to access that action. For this reason, this patch is expanding a little bit the scope of this issue:

• The action config entity defines now an access handler.
• The access handler delegates the check to a new plugin method ActionInterface::userAccess().
• The ActionBase abstract implements ::userAccess() by returning TRUE. In this way, we ensure the BC.
• We implement ::userAccess() also in ChangeUserRoleBase and we check there for the administer permissions permission.
• In BulkForm Views field plugin, we check also the access when we build the list of actions.

The patch contains also the #58 fix.

Fixed some docs and test failures. If the user has 'administer actions', permission, that should prevail over plugin check.

Updated the IS to reflect the patch. Updated the CR and, yes, because this introduces kind of disruption, it should go in 8.6.x. Changed the title to reflect that this is a regression from D7.

+++ b/core/modules/system/src/ActionAccessControlHandler.php
@@ -0,0 +1,26 @@
+    return AccessResult::allowedIfHasPermission($account, $admin_permission)->orIf($plugin_result);

I haven't done a full review on this, just some local tests. This line troubles me a bit though.
What this does is that it allows the user to view the action if he has the entity type's admin permission OR if the plugin allows it.
However, if the user has the 'administer actions' permission, and not the 'administer permissions' permission, in a bulk form, he will be able to view the Add/Remove roles actions but if he attempts to execute them, he will get an access denied error.

While this fixes the security issue of the ticket, it is a bad UX as - since there are not per-action permissions - we give the user the impression that he can mangle with the roles of the user until he actually tries to execute the action.

Initially I was thinking that an andIf would be a solution to that but in reality, this would mean that it would not suffice that the user would have the administer permission of the entity type.

For the record, I do not think that this makes the current implementation a bad approach, I just think that we still lack a per-action permissions system to take care of that.

Reroll of #67 that applies to 8.7.x and 8.8.x

This issue seems to have been lost in the shuffle for 2 years? I'm experiencing it on 8.9.13

The manager who has the right to create accounts and grant elevated roles to users is not allowed to change role permissions as they could change the way the site is configured. Changes that require more knowledge about how Drupal works than someone who is just managing users would not normally need to know. Changes that would have been based on decisions when the site was created. The fact that "Roles" are not displayed on the user maintenance screen, but are available on the bulk operations is inconsistent and may not be noticed because the bulk operations must be viewed as a drop down selection. Granted, the https://www.drupal.org/project/role_delegation and https://www.drupal.org/project/administerusersbyrole are options, but another user might be the one delegated to control permissions. Then an additional module would not be required.

Changes should be made one way or the other to avoid inconsistencies. Either allow both bulk operations AND the edit profile form to change the roles (by removing the authority check from the edit profile form), or remove the options from the bulk operations list (by applying the patch above). Or maybe a 3rd way of adding "Change user roles" and "Change own roles" to core. Then all that would be needed additionally, if wanted, would be an option to limit changing roles to those below your own role, or a selection of available roles allowed.

Still relevant in 9.3.

Patch doesn't apply.

Reroll of #67 that applies to 9.3.x and 9.4.x

Here is a reroll for 9.5.10 for anyone who might need it.

Leaving is as needs work until someone posts a patch for at least 10.x

Re-uploading the same patch with the correct name.
Also adding an interdiff.

Patch for the latest 11.x. Please review.

Please include interdiffs with patches or a comment of what you changed.

The last patch doesn't pass commit checks, could you make sure to run ./core/scripts/dev/commit-code-check.sh before uploading a patch to make sure there are no issues with code formatting. see https://www.drupal.org/docs/develop/development-tools/running-core-devel...