UserRolesCacheContext can lead to poisoned cache returns for user 1

Some snippets from the previous discussion:

A band-aid solution would be to merge in 'is-super-user' with the roles rather than returning a special string. This will kill caching between UID1 and other users with the same role, but at least it will not lead to stale cache entries for UID 1.

If we do decide to go with a band-aid, we should still remove this once we fix 📌 Add a container parameter that can remove the special behavior of UID#1 Fixed

Instead of 'is-super-user' can UID 1 ensure the administrator role name is always included?

That could again lead to collisions where UID1 without the admin role warms the cache in a way that is then shown to someone who really has the admin role.

Update IS that core/tests/Drupal/KernelTests/Core/Render/RenderCacheTest.php will need to be removed

Updating IS: We've had two security issues reported already that boil down to this bug. It's time to rip the band-aid off.

Let's simply remove the incorrect return value and put out a CR and release note mention. If you're still using user.roles to determine access, which you shouldn't, then the only affected person by this change will be UID1 or people who share the same roles as UID1. And only if you had access logic riding on it. Blocks such as described in the IS will actually start working properly now.

Here's some replication steps I found for this:

1. Install Drupal 11.0.5 with the Standard profile.
2. Log in as user 1.
3. Edit `services.yml` and set security.enable_super_user: false.
4. Clear caches to rebuild the container.
5. Remove the administrator role from user 1.
6. Note user 1 still has access to administrator functions.

Or, from the CLI if you have a Drupal project created with ddev:

cat > web/sites/default/services.yml <<EOD
parameters:
  security.enable_super_user: false
EOD
ddev drush -y si && \
ddev launch $(ddev drush uli) && \
ddev drush user:role:remove administrator admin && \
ddev launch # This browser tab will still have admin until you clear caches.

Here's a screen recording of the issue.

Added a MR to see what tests have to say about this. I'd be surprised if everything goes green, but who knows.

All green, well I'll be damned.

Love the remove in Drupal 9 comment there hehe.

Don't see how this would break anything removal is clean. Lets fix this leftover.

xjm → credited benjifisher → .

xjm → credited catch → .

xjm → credited effulgentsia → .

xjm → credited gabesullice → .

xjm → credited heddn → .

xjm → credited longwave → .

xjm → credited mcdruid → .

xjm → credited catch → .

xjm → credited effulgentsia → .

xjm → credited gabesullice → .

xjm → credited heddn → .

xjm → credited larowlan → .

xjm → credited longwave → .

xjm → credited mcdruid → .

xjm → credited poker10 → .

Adding credit for security team members who discussed the private issue related to this during the fortnightly core security triage as well as a duplicate of it.

We agree with the public handling, and simply removing this brittle code that is incorrectly coupled to the old special behavior of user 1 seems the best course.

xjm → credited drumm → .

Out of curiosity, I grepped to see anywhere else that is-super-user is interacted with like this and found ./core/tests/Drupal/Tests/Core/Session/SuperUserAccessPolicyTest.php, which makes sense.

Meanwhile. The IS says that core/tests/Drupal/KernelTests/Core/Render/RenderCacheTest.php should be removed. However, the MR does not remove it. Should it be removed? I'm a little confused -- that's an awfully generic test name for the scope of this issue. Could we explain further?

Okay so that test specifically tests the quirks that used to be (user.permissions) or still are (user.roles) in Drupal core's cache contexts. Given how we already removed one quirk by introducing the access policy API and we're about to remove the last quirk in this issue, that test has no reason to exist any more.

The only thing that puzzles me is why the test still goes green after we remove the final quirk. I would expect it to go red for the user.roles cache context at least. I'll investigate this and report back, then we can remove the test and set the issue back to NR.

Okay so the reason the test still goes green shows how poorly we were testing things.

Pre-patch:

• $user_1 cache context value = is-super-user
• $admin_user cache context value = authenticated,RANDOM_ADMIN_ROLE_NAME

Post-patch:

• $user_1 cache context value = authenticated
• $admin_user cache context value = authenticated,RANDOM_ADMIN_ROLE_NAME

So obviously that assert still goes green both before and after the patch.

So RenderCacheTest::testUser1PermissionContext() only goes green because we deliberately set $usesSuperUserAccessPolicy to TRUE and added a todo there to remove said flag. This warrants a test rewrite or removal.

Then RenderCacheTest::testUser1RolesContext() goes green both with and without the change, this is explained above.

The funny thing is that the second test case automatically also tested for user.permissions because those cache contexts are always added. This leads to the test still going green even if we remove the quirk from the UserRolesCacheContext. Adding the following code to the test makes it go green before the removal, but red after the removal.

  public function setContainer(ContainerBuilder $container): void {
    $renderer_config = $container->getParameter('renderer.config');
    $renderer_config['required_cache_contexts'] = [];
    $container->setParameter('renderer.config', $renderer_config);
    $this->container = $container;
  }

So all in all, this test was all sorts of broken and not even testing the thing it promises it was testing. Its removal is long overdue.

Removed the test. Should go green again and in my opinion is ready to ship given my explanation in #30.

📌 [Meta] Fix all tests that rely on UID1's super user behavior Active just in case here's the META that's tracking those $usesSuperUserAccessPolicy replacements, wooo done to 2 and 1 may be in contrib now.

I read the MR changes closely, and the issue summary and comments.

@kristiaanvandeneynde makes a compelling case that RenderCacheTest is really broken. There were already a couple of @todo comments in there about removing it. 😅

The actual change to stop the special case context for UID 1 makes sense to me. grepping core confirms that this and core/tests/Drupal/Tests/Core/Session/SuperUserAccessPolicyTest.php are the only places that mention is-super-user. In that test, it's only used to label cases inside a data provider array, not for any actual code, assertions, etc.

I renamed the MR to be self-documenting, but otherwise, all looked great.

I don't see anything left to do here but commit it. RTBC++

Thanks!
-Derek

Oh, does this need a CR? I guess that'd be the only other possible thing to do here.

It will be released as a security release, so I'd hope people read that even more than they would a CR.

Issues that will be packaged in a security releases and advisories are worked on in private. Working on this in public was agreed which means there will be no advisory nor security release.

The question of whether it needs CR? Not sure personally.

I checked contrib and there's no sign of anyone using the explicit string so that's good. For me that means I'd err on no change record as there's no sign anyone has to change anything.

FWIW it would great to file a follow up to add a TRUE to the in_array in this code.

Committed and pushed 7785f1a0136 to 11.x and 377558fa3b7 to 11.1.x. Thanks!

I wonder if this should be backported to 10.5.x / 10.4.x - like we were supposed to remove this code in 9.0.0 :)

Discussed with @catch and we agreed to backport to 10.4.x

Committed and pushed da8c7555eb8 to 10.5.x and 94c12a2a66b to 10.4.x. Thanks!

Wow. @kristiaanvandeneynde keeps fixing things in the (render) cache system! 🤩👏🙏🙈

The "funny" part is that this publicly solved private security issue is removing a reference to SA-CORE-2015-002, which is precisely why this landed the way it did in #2417895: AccountPermissionsCacheContext/PermissionsHashGenerator must special case user 1, since permissions don't apply to it → .

I wonder if this is only a security issue now, because so many other things have changed in the ~9.5 years since then, or whether it's been wrong since the moment it got committed? 🧐 No time to dig into all this to get enough of an understanding to answer that curiosity, but if it's easily answered, I thank you 🙇

@Wim it's because those issues were for the permissions cache context (where user 1 is always different unless/until the super-user access policy is removed and the administrator role is added instead), and this issue is for the roles cache context, which has very few practical use cases.

@alexpott:

In #37, you wrote,

FWIW it would great to file a follow up to add a TRUE to the in_array in this code.

I just added 📌 Clean up code in UserRolesCacheContext::getContext() Active .

Automatically closed - issue fixed for 2 weeks with no activity.