[sechole] Returning TRUE from hook_entity_access()/hook_ENTITYTYPE_access() must not bypass EntityAccessController::checkAccess()

Hm. This is how hook_node_access() always worked.

Absolutely, that Hm was just me thinking out aloud..

NodeAccessController deals with this problem correctly, as the only one, as it was a direct conversion of the node_access() implementation which has (very) extensive test coverage. It both has a minimal requirement (access content) that is always checked and always-allow-access check (bypass node access). That implementation is however also pretty complicated ...

The failure in #1 is "Block was displayed according to block visibility rules." but that's not the failure we want. It's the anonymous front page we want to fail, right?

Yes, this is by design. Yes, this is documented. But it does not match my expectations as a developer, and could easily did result in a security hole.

Maybe it can be solved by improving the naming? I.e., checkAccess() really is only defaultAccess() which can be overridden by any module.

Yes, defaultAccess() sounds like an interesting idea, but it would be quite an API change :)

Ok, what about this?

- Rename check... to default...
- Refactor access() and createAccess() to integrate the default implementation into the access array
- Added an alter hook so that modules can still change he default implementation if they want to.
- Updated NodeAccessController based on this.

This is the only thing I can think of that makes sense for both block and node.

No idea if there's going to be a problem with tests. Didn't add the tests from above yet and will need documentation updates.

In other news, createAccess() is a mess... the entity bundle nor $context isn't passed to the hooks which makes them useless, nor is context considered for the static cache.

Actually, now that I think about it, The NodeAccess override is probably still necessary because FALSE from a hook would override the bypass node access check...

Fixing my patch.

@chx's approach looks interesting, but it's going to require a lot more work and is quite the API change while my approach tries to make the API change as small as possible.

Sure; if you can get yours working I will be happy with that. Care to update the IS to reflect what you are doing please?

Too much bool casting.

This still need tests and documentation updates but it should be ready for reviews...

the default access methods now *must* return boolean values and not something like '1', isActive() ensures that for us.

And AreaEntityTest relied on the fact that hook implementations used to override the default access check, which is no longer the case.

I just noticed that #2278541: Refactor block visibility to use condition plugins should solve this problem as well because it (mostly) does away with using entity access API for block visibility and uses context & conditions instead.

So once that is in, this is no longer a security issue I think.

Because I'm worried that adding more hooks here is a performance regression.

I wonder though whether that really solves the issue or just punts it. I mean, what if another entity - plugin subsystem runs into this?

So now the conditions is in, the question in #27 stands, and I think a test needs to be added to demo the sechole gone.

I agree with #27: #2278541: Refactor block visibility to use condition plugins just happens to fix it for block entities. It doesn't fix the general problem.

The general problem is (from the IS):

Many entity access controllers have specialized code in their AccessController classes.
If a module returns TRUE from hook_entity_access() or hook_ENTITYTYPE_access(), then none of that code will ever run.

Yes, this is by design. Yes, this is documented. But it does not match my expectations as a developer, and did result in a security hole.

The code that does this is:

if (($return = $this->processAccessHookResults($access)) === NULL) {
  // No module had an opinion about the access, so let's the access
  // handler check access.
  $return = (bool) $this->checkAccess($entity, $operation, $langcode, $account);
}
return $this->setCache($return, $entity->uuid(), $operation, $langcode, $account);

Note how $return is overwritten.

Now, #2287071: Add cacheability metadata to access checks changed that to :

$return = $this->processAccessHookResults($access);
if (!$return->isAllowed() && !$return->isForbidden()) {
  $return->orIf($this->checkAccess($entity, $operation, $langcode, $account));
}
return $this->setCache($return, $entity->uuid(), $operation, $langcode, $account);

That's slightly better already, because we don't throw away the old value, we just combine it. But that if-test is still the same, so the same problem still exists!

We can fix it very easily now though: just remove the if statement.

I also wrote a test that explicitly tests ::checkAccess() being invoked.

Note: the above is equivalent to just doing:

    $access = array_merge(
      $this->moduleHandler()->invokeAll('entity_access', array($entity, $operation, $account, $langcode)),
      $this->moduleHandler()->invokeAll($entity->getEntityTypeId() . '_access', array($entity, $operation, $account, $langcode))
      [':default' => $this->checkAccess($entity, $operation, $langcode, $account)]
    );
    $return = $this->processAccessHookResults($access);

… which is what the above patches used to do.

Discussed with berdir and chx in IRC. Postponing this on #2340507: Make the new AccessResult API and implementation even better.

Fresh start, let's see what fails, I think the other access patches cleaned up a lot and the create access hook fixes got in elsewhere.

No longer renaming any methods, not worth the effort (anymore).

So, green, which is because the other access issues cleaned up the inconsistencies already.

So what we need here:

a) Decide that this is what we should do
b) A test that verifies that this now works as expected ( a test module that allows access, a default access check that denies access, both under some specific condition, so it does not affect other code, like a specific entity label)

Added a test. Which means only a) is left to do here?

Note that this could have a negative performance impact, for example, it means that node grants are always checked even if a hook explicitly forbids/allows access. This is exactly why node_access() used to work like HEAD does now and entity access was modeled after that.

So, this is either more or less ready or a won't fix if we don't want to performance regression for the mentioned cases.

Option c) Pass in the current result into checkAccess() and specifically in NodeAccessControlHandler, don't do the node grant check if the access result is not neutral...

/me no longer knows how to upload test only patches...

.

I think this should be needs review, as it needs a decision, not code :)

Clarified the IS further: this patch solves the problem at hand, the remaining question is: what's the performance impact, and is it acceptable?

So, all you need to know is this:

• In HEAD, we invoke hook_entity_access() and hook_ENTITY_TYPE_access() and OR them. This is still the same in the patch.
• In HEAD, we only call the entity type's access control handler's default access method if that OR'd result is isNeutral() === TRUE. Which means that either there were no entity access hook implementations, or none of them had an opinion.
• In this patch, we call it always, and OR it with the result from the hooks.

So far, the perf call is just an extra AccessResult::orIf() call (negligible cost) and a call to a default access method (::checkAccess() or ::checkCreateAccess()). Usually, that default access method is very fast. And… it happens if isNeutral() === TRUE… which is most of the time!

Back to that default access method. Usually, it's cheap, but in a few cases, it's relatively expensive (I looked at every single one to do this write-up):

• MenuLinkContentAccessControlHandler::checkAccess() has a code path that is executed for non-admin users trying to update a menu link, where it does access checking of the route that the menu link links to. Clearly fairly expensive.
• FileAccessControlHandler::checkAccess(), upon download and view, looks at all entities referencing this file, and if the user can view any one of them, access is granted
• BlockAccessControlHandler::checkAccess(), loads contexts and executes visibility conditions
• and then finally, the most important one: NodeAccessControlHandler::checkAccess(): it almost always uses the node grant system, which amounts to a DB query, IF and only if there are implementations of hook_node_grants().
• … all others (dozens) are cheap

Nodes are still much more important than any other entity type. So let's put aside the fact that the performance impact for all other entity types will basically be negligible, and only look at the impact for nodes.

Let's make it clear: yes, always applying node grants for node access, which amounts to one DB query per node that is access checked, is expensive and undesirable. But!

There are factors that significantly diminish the actual performance impact, from bottom to top (node grants -> access hooks -> NodeAccessControlHandler):

• If there are no hook_node_grants() implementations, then it amounts to a few cheap function calls and a static cache hit.
• By default, a Drupal 8 site does NOT have hook_entity_access() or hook_ENTITY_TYPE_access() implementations. Hence implementing them can be used for two things: customizing behavior, and enhancing performance. (In HEAD.) This change would imply that having such hook implementations no longer is a way for enhancing performance, only for customizing behavior
• Also important: orIf() applies lazy evaluation: if the result so far is isForbidden() === TRUE, then orIf() won't execute the default access method!

So, in conclusion, only if you are on a very advanced setup that uses node grants hooks and entity access hooks, you are going to see worse performance. But if you're doing something as advanced as that, you could as well override NodeAccessControlHandler and choose to undo this general DX/sechole-preventing change, for the sake of performance, at the cost of DX/sechole-risk.

Code review

Patch looks great. Only nitpicks:

1. +++ b/core/modules/system/src/Tests/Entity/EntityAccessControlHandlerTest.php
   @@ -67,6 +67,32 @@ function testEntityAccess() {
   +   * entity access control handler is always considered and can forbid access
   

   "can forbid access, even after access was already explicitly allowed by hook_entity_access()."

2. +++ b/core/modules/system/src/Tests/Entity/EntityAccessControlHandlerTest.php
   @@ -67,6 +67,32 @@ function testEntityAccess() {
   +   *
   +   *
   

   Two lines of whitespace, should be only one.

3. +++ b/core/modules/system/tests/modules/entity_test/entity_test.module
   @@ -484,6 +484,13 @@ function entity_test_entity_prepare_view($entity_type, array $entities, array $d
   +  // Drupal\entity_test\EntityTestAccessControlHandler::checkAccess().
   

   Missing leading backslash.

4. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
   @@ -76,11 +76,9 @@ public function access(EntityInterface $entity, $operation, $langcode = Language
   -    }
   +
   +    // Also execute the default access check.
   
   @@ -231,11 +229,8 @@ public function createAccess($entity_bundle = NULL, AccountInterface $account =
   -    }
   +    // Also execute the default access check.
   

   Inconsistent whitespace.

From IRC:

berdir: WimLeers: what about changing isNeutral() to !isForbidden()? if it is, then we know it will be forbidden, so we only have to call it if someone allows access in a hook?

Yes!

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
@@ -76,11 +76,9 @@ public function access(EntityInterface $entity, $operation, $langcode = Language
+    $return = $return->orIf($this->checkAccess($entity, $operation, $langcode, $account));

This always executes ::checkAccess(), even if it's going to be ignored because $return->isForbidden() === TRUE.

By only doing this if !$return->isForbidden(), we avoid calling ::checkAccess().

Without this change, this thing I said in #49 would actually not be true:

Also important: orIf() applies lazy evaluation: if the result so far is isForbidden() === TRUE, then orIf() won't execute the default access method!

Ok, did that, also updated for the nitpick review.

I'm betting one of those test-only patches should've been an interdiff :) I generated that interdiff for you.

There's nothing left for me to nitpick. This is now effectively blocked on somebody else than me reviewing this issue and hopefully RTBC'ing it. I can't find any more flaws in it. But I also introduced this approach in #30, almost 3 months ago.

Please review!

+++ b/core/modules/system/src/Tests/Entity/EntityAccessControlHandlerTest.php
@@ -67,6 +67,32 @@ function testEntityAccess() {
+   * Ensures default entity access is always checked.

This comment isn't exactly correct, since it's not checked if a hook returns forbid(), but I don't think we need to hold up commit on fixing that. Can be a followup or done if the patch needs a reroll anyway.

One way to work around this, is to pass along the $result from the hooks to the specific checkAccess() method, and don't call the grant system, in case you already allowed access.

What I don't get about this suggestion is that in HEAD, NodeGrantDatabaseStorage::access() returns a forbid, rather than a neutral, in the absence of a grant record, and that is unchanged in this patch. Therefore, what this patch does is correct, because that must be respected no matter what hooks say (forbid always wins). However, if the goal is for a lack of grant record to be a neutral(), so as to allow a hook that allows access to win, then I could see the case for passing the earlier result deeper into the call stack so that the grant implementation can skip the query if it knows that something else has already allowed access. Either that, or we don't need to change that API at all, and instead make NodeGrantDatabaseStorage::access() return a lazy object that implements AccessResultInterface. But, I think even if we want to do that, it's a separate issue from this one, since as NodeGrantDatabaseStorage is currently written, this issue's patch implements a necessary security fix that must not be bypassed even if we did pass the additional information.

Therefore, RTBC.

@effulgentsia: Right, the grant storage says forbid or allow, but *if* a hook implementations returns allow in HEAD, then we never check the default implementation which means that we never ask the grant storage in the first place. This is not the security issue that the issue was originally about (it was about blocks), this behavior has been ported from node_access() in 7.x and earlier versions.

Right now, that logic applies to all entity types, with mentioned approach, each entity type can decide for himself it that is what he wants or always do his checks.

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

diff --git a/core/modules/system/src/Tests/Entity/EntityAccessControlHandlerTest.php b/core/modules/system/src/Tests/Entity/EntityAccessControlHandlerTest.php
index fc12b89..d11a945 100644
--- a/core/modules/system/src/Tests/Entity/EntityAccessControlHandlerTest.php
+++ b/core/modules/system/src/Tests/Entity/EntityAccessControlHandlerTest.php
@@ -67,11 +67,13 @@ function testEntityAccess() {
   }
 
   /**
-   * Ensures default entity access is always checked.
+   * Ensures default entity access is checked when necessary.
    *
    * This ensures that the default checkAccess() implementation of the
-   * entity access control handler is always considered and can forbid access,
-   * even after access was already explicitly allowed by hook_entity_access().
+   * entity access control handler is considered if hook_entity_access() has not
+   * explicitly forbidden access. Therefore the default checkAccess()
+   * implementation can forbid access, even after access was already explicitly
+   * allowed by hook_entity_access().
    *
    * @see \Drupal\entity_test\EntityTestAccessControlHandler::checkAccess()
    * @see entity_test_entity_access()

I improved the comment on commit.