AccountPermissionsCacheContext/PermissionsHashGenerator must special case user 1, since permissions don't apply to it

Great catch! This could lead to information disclosure, or worse.

This is a side-effect of Drupal being role-based… but with this one exception.

This is a Drupal 7 issue too, so I've posted it to the private tracker.

Unpublishing this issue for now - I'll let willzyx know via contact form.

Re-publishing.

This has been fixed in Drupal 7: https://www.drupal.org/SA-CORE-2015-002

Here's a cleaner solution that I wrote while working on the D7 fix. May no longer apply, just uploading verbatim. Dates back from March 21, 2015.

In D8, role IDs are strings (e.g. 'anonymous'), so there r1 for UID 1 makes sense.

For reference, the Drupal 7 patch (which also contains tests that could be ported to Drupal 8) can be found here:
http://cgit.drupalcode.org/drupal/commit/?id=5cb79b4b217e9aa315d61284398...

(see the includes/common.inc and modules/simpletest/tests/common.test parts of the commit)

Credit for that Drupal 7 fix is: David_Rothstein, Wim Leers, willzyx

Talking about those kind of permissions, don't we also have a problem now with the admin roles being implemented outside of permissions?

Updating issue; we now have \Drupal\Core\Cache\Context\AccountPermissionsCacheContext for permission checking. UserRolesCacheContext is not for permission checking, only for presence-of-role checking.

Until #2428703: Add a 'user.permissions' cache context (was: "Should cache contexts be able to associate a cache tag?"), Drupal 8 was awkwardly and erroneously assuming that "permissions" === "roles".

#10: No, because User and UserSession (i.e. the AccountInterface implementations) have this:

  public function hasPermission($permission) {
    // User #1 has all privileges.
    if ((int) $this->id() === 1) {
      return TRUE;
    }

    return $this->getRoleStorage()->isPermissionInRoles($permission, $this->getRoles());
  }

i.e. they only special-case UID 1.

It is then RoleStorage::isPermissionInRoles() that takes advantage of the ::isAdmin() stuff you're referring to:

  public function isPermissionInRoles($permission, array $rids) {
    $has_permission = FALSE;
    foreach ($this->loadMultiple($rids) as $role) {
      /** @var \Drupal\user\RoleInterface $role */
      if ($role->isAdmin() || $role->hasPermission($permission)) {
        $has_permission = TRUE;
        break;
      }
    }

    return $has_permission;
  }

In other words: a role being an "admin" role has zero consequences for the "roles a user has", but does have consequences when looking at permissions. But that's the job of the AccountPermissionsCacheContext, so that is what we need to fix.

That being said, we could add a user.is_admin cache context for "admin user or not" variations. This is similar to the user.is_super_user (\Drupal\Core\Cache\Context\IsSuperUserCacheContext) cache context that we have for "UID 1 or not" variations.

The real bug lies not in AccountPermissionsCacheContex, but in PermissionsHashGenerator: that should've been special-casing the super user (user 1) all along, but it has not. The current implementation there is wrong, because the permissions for the super user are NOT determined by the union of the permissions of the roles of user 1: it simply gets all permissions.

So, fixed PermissionsHashGenerator instead, and updated its test coverage.

Its unit test was also named wrongly: PermissionsHashTest instead of PermissionsHashGeneratorTest — fixed that too.

Adding a clarifying comment, based on a discussion with catch in IRC.

It's nice to have one tag to find all of the public critical security issues, and this is on 5 of them already.

I think we should better add also an integration test for this particular issue, you know, just to be safe in the future.

Fair.

Yes, I think the tests mentioned in #9 should be forward-ported.

Not quite sure how to do that, honestly.

Yes, we could try to port those tests as close as possible, but we actually converted that test to unit tests in 8.x. And since this is now pretty nicely decoupled from the render system, it seems kind of backwards to go back to kernel-integration-something tests for the render system. It doesn't make sense to cover that in the unit test for the render system because as I mentioned, the actual code is completely decoupled.

So.. that kind of leaves only a full-blown web tests with either a test block implementation or a real example somewhere that uses permission access inside a block/entity.

Thoughts?

There is already Drupal\system\Tests\Render for rendering-related non-unit tests; I think it can just go there.

The Drupal 7 tests for this issue seem like about the right level to me; they are high-level enough that they ensure the cache doesn't leak between users when rendering is performed in an actual Drupal environment, but low-level enough that they aren't tied to a specific rendering situation.

The attached patch forward-ports those tests, but it isn't working yet. I think the code that tries to override the current request method and force it to be a GET request isn't taking effect correctly. So each of these patches should currently have the same two test failures.

I think the code that tries to override the current request method and force it to be a GET request isn't taking effect correctly.

Came back and took another look, and that turned out to just be due to some random caching Symfony does of the REQUEST_METHOD...

That's fixed, but the tests still aren't passing. However now the failures are the ones you'd expect to see if the bug isn't actually fixed. I haven't looked into it enough to see if that's real, or if there's something still wrong with the test.

Some more minor fixes, but this won't change anything mentioned in the previous comment.

Fine, let's do that.

The problem with your test is that you used the user.roles context. That one *is* the same for user 1 and correctly so. The one that should be used is user.permissions. user.roles should only be used when roles are checked explicitly. Then user 1 doesn't have anything special.

I also converted it to a kernel test and removed the request method stuff... I'm 99.9% sure that can never be anything but GET. This isn't a superglobal like in D7.. the request object is manually created in KernelTestBase::setUp(), it's always the same.

This fails/passes as expected locally.

@willzyx: Nice crosspost, good thing you didn't already fix it yourself :)

I thought about special casing user.roles but I don't think we need that.. user.roles has several other flaws that would bite you if used incorrectly, for example, it doesn't detect permission changes in roles, so if you would grant a new permission, you'd still be using the same cache context and would get a cached response..

Also, slightly related issue that I opened yesterday: #2509478: PermissionsHashGenerator should have a static cache

1. OK, on user.roles vs. user.permissions, hm, this is odd. Drupal 7 just has one (DRUPAL_CACHE_PER_ROLE) to cover both cases which seems more natural for most situations. I would expect people to frequently cache per role when they care about permissions because even Drupal 8 core is doing this. For example, in Drupal\comment\CommentForm::form():

       $is_admin = $comment->id() && $this->currentUser->hasPermission('administer comments');
   ....
       // Vary per role, because we check a permission above and attach an asset
       // library only for authenticated users.
       $form['#cache']['contexts'][] = 'user.roles';
   

   And in Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter::viewElements():

       // Only show the add comment form if the user has permission.
       $elements['#cache']['contexts'][] = 'user.roles';
       if ($this->currentUser->hasPermission('post comments')) {
   

   I did some brief testing with the comment form (and with this patch applied) and couldn't reproduce an actual vulnerability with user 1 in the above cases, but I think it's really only a coincidence if you can't...

2. I had thought about using KernelTestBase but went with WebTestBase in order to test in a full Drupal environment. Is there an advantage to KernelTestBase except that it runs faster and doesn't require forcing a GET request? I guess it's OK but somehow it seemed wrong to have no tests for this security issue that run on a "real", full-fledged Drupal site.

1. That usage might be wrong. user.permissions was introduced not too long ago, because user.roles had problems. As mentioned, it has other problems too, so using it is just wrong anyway, even if we would add another check to that one as well. See https://www.drupal.org/node/2459373 for the change record and #2428703: Add a 'user.permissions' cache context (was: "Should cache contexts be able to associate a cache tag?") for the issue that explains why user.permissions exists.

2. Being faster is the biggest reason I think. Since we're just doing API calls, I don't think there's a reason to use WebTestBase, it's not that different then. The request for web tests is AFAIK also always GET, it's created in prepareRequestForGenerator().

Agreed with Berdir's reasoning about the user.roles cache context. See a longer explanation in my comments #11, #12 and #13.

#33/#34.1: indeed: that one is wrong. It was missed during the conversion, i.e. it was missed in #2428703: Add a 'user.permissions' cache context (was: "Should cache contexts be able to associate a cache tag?").

RTBC'ing #30.

So given the history (this was changed nearly six months after Drupal 8 entered beta) and the fact that core still has some cases that use user.roles to handle permissions (I guess there should be a separate issue to fix that?) I think we want the same protection for user.roles as user.permissions. There isn't really a good reason for user 1 to share any role-based or permission-based cache with other users anyway.

So basically I think the test should change to do a foreach() over user.roles and user.permissions (or something similar), so that it tests both cases separately... and then fix both :)

The request for web tests is AFAIK also always GET, it's created in prepareRequestForGenerator().

When running this as a webtest via the SimpleTest UI, it needed to be forced to GET or the test won't pass. Probably not needed for the testbot, though, since that runs via the command line.

I don't think adding the user 1 check to user.roles is helpful. If you're using that for permissions checks, then you already have a potential information disclosure issue when your permissions change. Also when reviewing both against each other, if they both special case user 1, this implies that user/1 behaviour should be different when it isn't, so could even misdirect people to using user.roles inappropriately.

Is there an existing core issue for the bad user.roles usage?

I looked at all remaining user.roles usages:

1. BookNavigationBlock: wrong, is being fixed in #2483181: Make book navigation block cacheable.
2. CommentDefaultFormatter: wrong, needed both user.permissions and user.roles, but can be updated to be more granular (use user.roles:authenticated, and only add it a level deeper in the if-branching)
3. CommentForm: wrong, needed both user.permissions (always) and user.roles, but can be updated to be more granular (use user.roles:authenticated, and only add it a level deeper in the if-branching)
4. history.module: used correctly
5. LoginStatusCheck: used correctly
6. \Drupal\user\Plugin\views\access\Role: used correctly
7. RoleAccessCheck: used correctly

Opening an issue for fixing points 2 and 3 now.

Here we go, includes a patch: #2510034: Remove one useless cache context in CommentDefaultFormatter.

Given #44 + Berdir's + my comments, and the fact that the wrong usages are being fixed, and patches exist: moving back to RTBC.

I've bee quite involved with this one, so I won't commit it - just to give time for David to come back or a second opinion from another committer.

I don't think adding the user 1 check to user.roles is helpful. If you're using that for permissions checks, then you already have a potential information disclosure issue when your permissions change. Also when reviewing both against each other, if they both special case user 1, this implies that user/1 behaviour should be different when it isn't, so could even misdirect people to using user.roles inappropriately.

I commented earlier on the closed issue that if the permissions change problem is considered a security hole we should deal with it too. But I didn't think it was really a security issue; if it only grants you access to things you previously had access to (but for longer than the site administrator intended) then it just seems like a regular cache-invalidation bug to me.

This issue meanwhile is an actual security issue. I think it's too late in the Drupal 8 cycle to assume people will change their code in response to every change notice, so given the history I think it's important to apply the security protection to both cases. (And a code comment could be added to the user.roles case explaining why this is done, so it doesn't introduce confusion for anyone reading the code.) I don't see any harm in adding this, and I think it's the only way to fully solve the security issue - so I don't think this patch should be committed in its current form.

I don't see any harm in adding this, and I think it's the only way to fully solve the security issue - so I don't think this patch should be committed in its current form.

I agree with @David_Rothstein I think being cautious makes sense.

On the basis that we added the permissions cache context quite recently, then doing this as a bc layer I can see the argument for.

We should add a @todo to remove that for 9.x though - by then modules (and core!) should be using the proper cache contexts all the time.

I also agree and am bold to set back to needs work.

While we introduced permissions, we still support user.roles and it is the closest that comes to mind when one had used DRUPAL_CACHE_PER_ROLE.

The one more bucket used should not hurt.

I still think this is a bad idea :)

Yes, using the right cache contexts is not easy, and being cautious and on the safe side is a good thing. That's what #2493033: Make 'user.permissions' a required cache context* is about, which i think we should really do. This I consider to be more confusing than helpful.

But, if that's whats' needed to get it in, here's a patch for it.

While writing this, I actually noticed a much bigger problem, which would have been a "fun" security issue. The previous patch returned 'is-super-user', unhashed. But this hash is also used client-side, so anyone would have been able to set their local hash to 'is-super-user'. Improved this. Interesting that nobody noticed this :)

* With that issue, it would actually no longer be possible to write a failing patch for the user.roles test method.

RTBC, lets get this in - this provides parity with D7. We can remove uid 1 in D9 ;).

The changes to the test look good to me. Reading through #2493033: Make 'user.permissions' a required cache context I think I agree; that issue might be the best way to get the tests here to pass.

While writing this, I actually noticed a much bigger problem, which would have been a "fun" security issue. The previous patch returned 'is-super-user', unhashed. But this hash is also used client-side, so anyone would have been able to set their local hash to 'is-super-user'. Improved this. Interesting that nobody noticed this :)

Yikes. From what I can tell, core itself is only using this client-side for cache invalidation, though. In other words, setting your local hash to 'is-super-user' wouldn't actually do anything to get the server to send you the super-user's data, would it? Still seems worth hashing though, I agree.

I am not sure if the intention is to allow contrib modules to use those client-side hashes to actually request data from the server... if so, I would have questions about whether the hashing here is secure enough (since it doesn't have any built-in expiration; once you know it, it's essentially good for the lifetime of the site). But that's a preexisting issue.

---

Otherwise looks good - I basically agree with RTBC (almost cross-posted). But it's missing the comment explaining why we do this (and the @todo suggested by catch in #51)...

Hm, thanks David (#55). Sorry, I missed those two tasks ...

( tagging - sorry for the noise )

Something like this?

Yup, looks great to me! :)

While writing this, I actually noticed a much bigger problem, which would have been a "fun" security issue.

/me hides in shame, despite the total lack of an actual vulnerability.

From what I can tell, core itself is only using this client-side for cache invalidation, though. In other words, setting your local hash to 'is-super-user' wouldn't actually do anything to get the server to send you the super-user's data, would it?

Correct. This is why there is indeed nothing vulnerable about this.

#58's comment additions look excellent.

+    // User 1 does not actually have any special behavior for roles, this is
+    // added as additional security and BC compatibility for SA-CORE-2015-002.
+    // user.

1. What's up with the last line - looks like it was supposed to be deleted?
2. What's "BC compatibility"? Does it stand for "backwards compatibility compatibility"? :)
3. Grammar isn't really right.

I could probably just leave this RTBC since I only changed the code comment, but I made enough changes that I'm putting this back for review; I'm sure someone will re-RTBC it soon.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 8d80b7a and pushed to 8.0.x. Thanks!

Looks like there was some cross-posting above. Reuploading the interdiff from #61 as an actual patch. @Fabianx already RTBC'd this in #62.

Committed 51656b3 and pushed to 8.0.x. Thanks!