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.
Basically, the way the entity access control handlers (ACH) work is the following:
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.
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.
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.
- kristiaanvandeneynde
- agentrickard
- moshe weitzman
Active
9.5
It is used for security vulnerabilities which do not need a security advisory. For example, security issues in projects which do not have security advisory coverage, or forward-porting a change already disclosed in a security advisory. See Drupal’s security advisory policy for details. Be careful publicly disclosing security vulnerabilities! Use the “Report a security vulnerability” link in the project page’s sidebar. See how to report a security issue for details.
Group module could also check for implementations of hook_node_grants() to decide whether to return forbidden or neutral? Trying to be 100% entity-type agnostic seems to be a nice goal but ignores reality.
For sure we need some better documentation and maybe a parallel access system for other entity types.
Adding a tag and a list of contributors in the private issue