Originally reported to the Drupal security team as a security issue by
@kristiaanvandeneynde →
on 4 August 2020, but has been moved to the public issue queue, as there are no security implications.
---
Also affects Drupal 9. Not sure if it's a security issue in its own right but I figured I'd start the discussion here and then we can always create a public issue about this if allowed.
The problem
Basically, the way the entity access control handlers (ACH) work is the following:
- Fire all hook_entity_access() and hook_ENTITY_TYPE_access() hooks.
- If nothing explicitly returned Forbidden, run the ACH's checkAccess() method.
This checkAccess() method often returns Allowed based on whether an account has a given permission. In some cases it's a very simple check such as "access comments", in other cases it's (slightly) more complex such as MediaAccessControlHandler. Either way, this method is often the last chance to allow access before it stays Neutral, leading to an access denied screen.
So for all entity types, if your module makes a promise that "entity X will never be visible unless you're on domain Y", "entity X will never be visible unless you're a member of Group Y", etc. they should return Forbidden when their condition is not met. Otherwise, one of these simple permission based access checks will return Allowed and your module fails to keep its security promises.
However, because most modules only care about nodes (or start with nodes, such as in Group's case), a large amount of modules return Neutral when they really ought to return Forbidden. The reason is that node.module's ACH offloads its responsibility to NodeGrantDatabaseStorage (NGDS) and that class returns different results based on whether any module implements hook_node_grants().
This is exactly the reason why Group 1.0 had a security vulnerability by removing hook_node_grants() for websites where Group was the only module implementing said hook. All of a sudden, returning Neutral was no longer sufficient because NGDS allows access to all published nodes whereas it used to not do so.
The worrisome part
Ideally, we should get rid of node grants altogether in favor of Entity API's entity query access or something similar. But when we do so, a lot of modules will still return Neutral when they really ought to return Forbidden and, just like Group, will all of a sudden become insecure. So we can't get rid of this archaic system that only serves nodes rather than all entity types without first having to reeducate all module maintainers and make sure they update their code.
In short: We may be stuck with node grants for a really long time because removing it will probably make a large amount of modules insecure. And we really, really ought to get rid of node grants because at this point it's ridiculous that nodes are still core's special little darling that are the only entity type to have full entity and query access after all these years.
Further reading
I left some notes in the Group issue where people complain that all of a sudden their other node access modules/code can't work with Group that well any more because Group now returns Forbidden. The thing is, I have to return Forbidden because my code is entity type agnostic and for all other entity types, returning Neutral will lead to information disclosures and because I can't be sure any other module will have implemented hook_node_grants().
See:
https://www.drupal.org/project/group/issues/3162511#comment-13774054 →
It contains a good example of how Domain Access should be working, but when you inspect Domain Access for D8 you'll notice that it doesn't work that way. I'm sure if I inspect a dozen other node access modules that I would come to the same results.
Problem/Motivation
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
