Problem/Motivation

In #2323721: [sechole] Link field item and menu link information leakage we have re-introduced the single access check for linking in menu links, link fields and shortcuts. This issue follows up by adding the same access check to the Url object so that links created via that object can be access checked as well. We will follow with LinkGenerator and UrlGenerator in separate issues.

Proposed resolution

  1. Make AccountManager::checkNamedRoute default to @current_user.
  2. Add a access(AccountInterface $account = NULL) method to the Url class.
  3. Add #access => $this->access() in Url::toRenderArray. (Correction: to make this render cache compatible, use a new #access_callback.)
  4. Convert SystemBrandingBlock as a demonstration. Note: this is a bugfix as the current implementation does not consider that the relevant paths might have their access checks altered.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Issue summary: View changes
dawehner’s picture

Title: Routes are not access checked » Add a route helper that returns access checked routes

Thank you for this glorious title.

There is no service which would match at the moment, given that we do have things separated now.

Make everything that returns "naked" routes protected.

Does that mean that you have to check access just to load a route? This would be horrible for performance.

chx’s picture

> Does that mean that you have to check access just to load a route?

If it gets passed to the outside of the routing system? Yes, absolutely! Why on earth would you need route objects you can't access???? I am missing something, please tell me what.

dawehner’s picture

Random examples: ConfigNamesWrapper::getBaseRoute(), EntityDerivative::getDerivativeDefinitions()
People work with routes not just to display stuff.

catch’s picture

Doesn't current route match deal with access checks already? That's the main use case for checking access - on the current route.

I can see the value in having a helper for access checks on other routes as well, but don't see why that's critical - is that really used much either in core or contrib outside of the menu system?

dawehner’s picture

Doesn't current route match deal with access checks already? That's the main use case for checking access - on the current route.

The CurrentRouteMatch class uses the $request, so it relies on the AccessSubscriber to be triggered during the request event flow, which we will just have to accept, otherwise the world is broken already.

chx’s picture

Unless of course you are a KernelEvents::REQUEST listener with a weight of 31 at which point routing happened but access check not yet. I am reasonably sure that if such a gap exists we will find other code that might run in that gap. This time I do not have the change notice to help me out with a handy exploit code (...) but the separation of routing and access checking is dangerous, no matter what. You rely on the default page flow -- that's like calling the access checker from menu_execute_active_handler instead of menu_get_item. I do not like this and it will come back with a vengeance.

I see your examples in #4; but is any of that performance sensitive? For example, aren't we caching getDerivativeDefinitions?

chx’s picture

Issue summary: View changes
moshe weitzman’s picture

Priority: Critical » Major

Downgrade per https://www.drupal.org/core/issue-priority. Still a big issue but not quite release blocking.

chx’s picture

Priority: Major » Critical

You can't downgrade security regressions.

tim.plunkett’s picture

I don't see how "call this function and do magic with the results" is that different from "call this method and then call another method".
Not getting into the status war.

chx’s picture

So you can't see a difference between "you get a route from the route provider and then need to get a request, run a match and run an access check" and if ($item && $item['access']) ?

tim.plunkett’s picture

I'm not saying it couldn't be easier, just that it's not a regression.

dawehner’s picture

FileSize
12.68 KB

So you can't see a difference between "you get a route from the route provider and then need to get a request, run a match and run an access check" and if ($item && $item['access']) ?

You want to use the system as API. In this case you want a route from a given name you would need the parameters
as well as you can't really do anything without them. IN case you have them, there is the access manager, done.

Things we can improve:

  • Add an access method on the route match
  • Makes it a little bit easier (just supply the route object) + parameters on the access manager.
  • Introduce something like $router->getRouteMatchFromPath() and then use the access() method introduced here.
Crell’s picture

I don't understand the issue here. chx or someone, can you provide a code sample (not description in a paragraph) of the offending/problematic API usage? Or whatever it is that needs to be fixed here?

"Access control is not tightly coupled to the route anymore" isn't a bug, it's a feature. As in, things not being tightly coupled anymore is the story of Drupal 8, for a reason. If the objection is other than that then I don't understand it.

+++ b/core/lib/Drupal/Core/Routing/Route.php
@@ -0,0 +1,49 @@
+ * @TODO Try to bring this into symfony itself.

I'm pretty sure this wouldn't be accepted; Route names are part of the collection, not the route itself, for better or worse. (I think it makes sense, but I do not expect the Symfony PTB to agree.)

jibran’s picture

Status: Active » Needs review

NR for #14

Status: Needs review » Needs work

The last submitted patch, 14: 2302563-14.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
18.35 KB

This is how I would do this. ->access() belongs on the route, plain and simple, every theoretical mumbo-jumbo aside.

Status: Needs review » Needs work

The last submitted patch, 18: 2302563_18.patch, failed testing.

chx’s picture

So this would require moving checkNamedRoute on the route provider. Some refactor, yes.

And the use case? See, this is the difference. Drupal 6+ had a coherent vision in security that anyone talking to the menu system would have a coherent access checked item and so any link provided by the menu system or any caller copying its behavior wouldn't need to think. The point is to make it easy to do the right thing. If you want a code sample, the patch itself provides it in AccessManager::checkNamedRoute .

Edit: let me reiterate: the problem is if any code wants to provide a link to a route with certain arguments then it should be trivial to check whether that page will be accessible to the user or not.

dawehner’s picture

Putting access() onto the route is similar to putting access() on the node-type.

chx’s picture

Why?? I must be missing something. In fact, I must be missing a lot of somethings.

Let's take

$href = \Drupal::urlGenerator()->getPathFromRoute('config.diff', array('source_name' => $names['old_name'], 'target_name' => $names['new_name']));

this snippet, I found it in some test. Now imagine you actually want to display a link to this page. Here's how I would imagine this:

$route = $this->routeProvider->getRouteByName('config.diff');
$parameters = array('source_name' => $names['old_name'], 'target_name' => $names['new_name']);
if ($route->access($parameters)) {
  $href = $this->urlGenerator->getPathFromRoute($route, $parameters);
// ... 
}

easy as pie. Remember that snap to grid article? I modelled this on the code flow from there. And it seems to me this flows. (Note: I wanted to pass $parameters to getRouteByName but #2316537: Upgrade symfony CMF to 1.2, remove confusing parameters from getRouteByName* )

I do not even know how would you do this currently.

Edit: to be honest, $route->getPath($parameters); would be ideal.

chx’s picture

Status: Needs work » Needs review
FileSize
41.47 KB

A new approach invented by dawehner: a ParameterizedRoute object. This is so pretty:

  public function access(AccountInterface $account = NULL) {
    return $this->route->getParameterizedRoute($this->parameters)->access($account);
  }

Status: Needs review » Needs work

The last submitted patch, 23: 2302563_23.patch, failed testing.

chx’s picture

dawehner points out this thing could possibly be on the Url object. I will rework in that direction. This is getting better and better...

chx’s picture

Issue summary: View changes
chx’s picture

Issue summary: View changes
chx’s picture

Title: Add a route helper that returns access checked routes » Access check (route derived) Url objects
Issue summary: View changes
chx’s picture

Title: Access check (route derived) Url objects » Access check (route derived) Url and RouteMatch objects
Status: Needs work » Needs review
FileSize
20.94 KB

Like this. The access manager is using setter injection, I guess that's fine. Should I write a trait for it?

Status: Needs review » Needs work

The last submitted patch, 29: 2302563_29.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
22.19 KB

Like this. The access manager is using setter injection, I guess that's fine. Should I write a trait for it?

chx’s picture

FileSize
8.83 KB

OK we can seriously simplify this.

The last submitted patch, 31: 2302563_31.patch, failed testing.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
@@ -56,10 +66,13 @@ class RouteMatch implements RouteMatchInterface {
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   A request object.
    */
-  public function __construct($route_name, Route $route, array $parameters = array(), array $raw_parameters = array()) {
+  public function __construct($route_name, Route $route, array $parameters = array(), array $raw_parameters = array(), Request $request = NULL) {
     $this->routeName = $route_name;
     $this->route = $route;
+    $this->request = $request;
 

@@ -90,7 +103,8 @@ public static function createFromRequest(Request $request) {
+        $request);

@@ -140,6 +154,39 @@ public function getRawParameters() {
+    $this->getAccessManager()->checkNamedRoute($this->getRouteName(), $this->getRawParameters(), $account, $this->request);

You don't want to do that, believe me! this would leak "data" from the current request into that checking method.

chx’s picture

This is another code smell if #34 is correct. Are you telling me that the request passed to the factory method for a RouteMatch is somehow not the request that should be used whether that particular route match is accessible to a certain account?

This whole thing is so badly over engineered it takes weeks to cut through the layers. Like, I was under the impression just based on the interfaces that RouteProvider is the primary place to get a route but turns out this is not the case, so much so that you actually never should call RouteProvider. More, apparently you should never have a route object yourself but pass the route name to various methods (some of which indicates it takes a route name, but for example UrlGenerator doesn't bother with such details) and the parameters. This, of course, is documented exactly nowhere. We will be here for a long time to fix all this.

dawehner’s picture

Issue tags: +Security improvements
FileSize
14.22 KB
9.65 KB

ha, glad that everyone of us makes one (or more) errors while writing code.

Yes, we do currently have an implicit dependency between the action and the user module :)

pwolanin’s picture

discussing #2323721: [sechole] Link field item and menu link information leakage I'm not sure this is the right direction, I'd rather move to seeing Url object/class as a more pure value object

chx’s picture

I have budged on that issue and we won't run access checks from the factory methods but adding these methods is quite necessary. As the ultimate goal , they should be automatic in toString / toRenderArray.

chx’s picture

Issue summary: View changes
znerol’s picture

Would it be possible to convert some access-checks in existing core code to the newly introduced methods? From what I gather from the patch they are currently only invoked from test code.

chx’s picture

Consider this typical fragment:

    $administer_themes_access = $this->currentUser->hasPermission('administer themes');
    if ($administer_themes_access) {
      // Get paths to theme settings pages.
      $appearance_settings_url = $this->urlGenerator->generateFromRoute('system.theme_settings');

This should be

$appearance_settings_url = (new Url('system.theme_settings'))->toString();
if ($appearance_settings_url) {
}

because it is arguably a bug that we suppose the route system.theme_settings is access checked by administer themes. Yes, that's how we ship Drupal but there's no guarantee of that.

chx’s picture

Issue summary: View changes
FileSize
20.37 KB
8.71 KB

For now, dawehner asked to skip access check in toString so I am descoping that from the issue. I have merged the AccessManager changes from #2317833: SystemBrandingBlock::blockForm should checkNamedRoute instead of permissions . That issue is now a duplicate because I used it here as a demo on how this can be done.

I have added a link to any page bypass to Url::access but not into RouteMatch. Discuss.

I was afraid that #access would be incompatible with render caching, I added a simple #access_callback property. The simplicity after is a thing of beauty: the Url::renderAccess is a single line of code. I wonder whether we will get any test fails from the new tightening...

Status: Needs review » Needs work

The last submitted patch, 42: 2302563_42.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
20.38 KB

Status: Needs review » Needs work

The last submitted patch, 44: 2302563_44.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
20.34 KB

I have added a link to any page bypass to Url::access

Yeah and account is optional, there... that's why we got so many fails.

Status: Needs review » Needs work

The last submitted patch, 46: 2302563_46.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
32.39 KB

That's odd somewhere the merge got lost from the other issue. I ran FileFieldAttributesTest and it passed.

chx’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 48: 2302563_48.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
36.43 KB
  1. The confirm form test asserts whether a user who does not have access to any admin pages lands on those pages. Hilarity ensues.
  2. The TaxonomyTermReferenceRdfaTest runs in such an impossible environment that the anonymous role doesn't exist (added normally when user module is installed) nor does it have access to content and so the formatter formats into a big fat nothing as it should be -- remember, you are not supposed to link to pages you can't access. Added those.

Status: Needs review » Needs work

The last submitted patch, 51: 2302563_51.patch, failed testing.

dawehner’s picture

  1. +++ b/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php
    @@ -49,6 +50,13 @@ class SystemBrandingBlock extends BlockBase implements ContainerFactoryPluginInt
       /**
    +   * The access manager.
    +   *
    +   * @var \Drupal\Core\Access\AccessManagerInterface
    +   */
    +  protected $accessManager;
    +
    
    @@ -64,11 +72,12 @@ class SystemBrandingBlock extends BlockBase implements ContainerFactoryPluginInt
    +    $this->accessManager = $access_manager;
    
    @@ -81,7 +90,8 @@ public static function create(ContainerInterface $container, array $configuratio
    +      $container->get('access_manager')
    

    We don't need the access manager in this class...

  2. +++ b/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php
    @@ -64,11 +72,12 @@ class SystemBrandingBlock extends BlockBase implements ContainerFactoryPluginInt
         $this->urlGenerator = $url_generator;
    
    @@ -81,7 +90,8 @@ public static function create(ContainerInterface $container, array $configuratio
           $container->get('url_generator'),
    

    If I see it correct the url generator is no longer needed

chx’s picture

Status: Needs work » Needs review
FileSize
36.52 KB
3.18 KB

Oh of course, #access_callback should only run when there's no #access already set.

chx’s picture

FileSize
38.06 KB

I wrote a test for renderAccess. Caught a random test fail, filed #2330751: Random test failures everywhere due to ->with(Request::create())

znerol’s picture

I like the improved DX of Url::access(). This is also very well illustrated by the code changes in SystemBrandingBlock. Still I do not quite understand the changes to RouteMatch. Can you come up with a use case where the additional methods in that class will improve the situation for core / contrib code?

chx’s picture

FileSize
38.58 KB
4.1 KB

I am not sure whether I can but it costs nothing to have them there. RouteMatch is very new (got added two months ago) and it will only spread for eg #2330363: Enhance the controller resolver to get a route match class . We might want to consider caching the access though because CurrentRouteMatch is already quite widespread.

chx’s picture

Title: Access check (route derived) Url and RouteMatch objects » Access check Url objects
Issue summary: View changes
FileSize
29.6 KB

Removed RouteMatch because only CurrentRouteMatch is used and that's access checked since the access aware router went in.

Status: Needs review » Needs work

The last submitted patch, 58: 2302563_58.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
30.49 KB

I got slightly overeager with removing changes.

chx’s picture

Issue summary: View changes
dawehner’s picture

  1. +++ b/core/includes/common.inc
    @@ -2809,6 +2809,17 @@ function drupal_render(&$elements, $is_recursive_call = FALSE) {
       };
    +  /** @var \Drupal\Core\Controller\ControllerResolverInterface $controller_resolver */
    +  $controller_resolver = \Drupal::service('controller_resolver');
    +  if (!isset($elements['#access']) && isset($elements['#access_callback'])) {
    +    if (is_string($elements['#access_callback']) && strpos($elements['#access_callback'], '::') === FALSE) {
    +      $callable = $controller_resolver->getControllerFromDefinition($elements['#access_callback']);
    +    }
    +    else {
    +      $callable = $elements['#access_callback'];
    +    }
    +    $elements['#access'] = call_user_func($callable, $elements);
    +  }
    

    Do you mind asking for a dedicated test coverage for the access callback bit?

  2. +++ b/core/lib/Drupal/Core/Url.php
    @@ -386,6 +388,7 @@ public function toRenderArray() {
    +        '#access_callback' => array(get_class(), 'renderAccess'),
    

    instead of get_class() afaik we usually use static, doesn't that work here?

  3. +++ b/core/lib/Drupal/Core/Url.php
    @@ -412,6 +415,53 @@ public function getInternalPath() {
    +  public static function renderAccess(array $element) {
    +    return (new static($element['#route_name'], $element['#route_parameters'], $element['#options']))->access();
    +  }
    

    We could think about inlining the accessManager call

  4. +++ b/core/modules/rdf/src/Tests/Field/TaxonomyTermReferenceRdfaTest.php
    @@ -100,6 +98,10 @@ protected function setUp() {
    +    // Grant the access content permission to the anonymous user.
    +    entity_create('user_role', array('id' => DRUPAL_ANONYMOUS_RID))->save();
    +    user_role_grant_permissions(DRUPAL_ANONYMOUS_RID, array('access content'));
    

    Have you considerd to use UserRole::create(...). I really like that new way. Then you can also use $role = UserRole::create(). $role->grantPermissions(); $role->save()

chx’s picture

FileSize
31.66 KB
2.3 KB

> Do you mind asking for a dedicated test coverage for the access callback bit?

Not the least.

> instead of get_class() afaik we usually use static, doesn't that work here?

You are right: it doesn't. __CLASS__ would but that would be Url always.

> We could think about inlining the accessManager call

We could but we are not in an object content, this is a static call so we can't call $this->accessManager(). If you mean that instead of ->access() I should copy the whole body of access() , I do not think so.

> I really like that new way

Yes, that's very pretty.

chx’s picture

dawehner’s picture

We could but we are not in an object content, this is a static call so we can't call $this->accessManager(). If you mean that instead of ->access() I should copy the whole body of access() , I do not think so.

Is it not possible at all to just use \Drupal::accessManager()->access(...)?

> instead of get_class() afaik we usually use static, doesn't that work here?

oh php!

chx’s picture

> Is it not possible at all to just use \Drupal::accessManager()->access(...)?

We discussed on IRC and since there is no accessManager the resulting code would look uglier so -- yes possible, but no we aren't doing it.

tim.plunkett’s picture

  1. +++ b/core/includes/common.inc
    @@ -2809,6 +2809,17 @@ function drupal_render(&$elements, $is_recursive_call = FALSE) {
    +    if (is_string($elements['#access_callback']) && strpos($elements['#access_callback'], '::') === FALSE) {
    

    getControllerFromDefinition supports much more than just class::method. I think you can just always call this and assign it to $callable, right?

  2. +++ b/core/lib/Drupal/Core/Access/AccessManagerInterface.php
    @@ -45,7 +45,8 @@
    +   *   (Optional) Run access checks for this account. Defaults to the current
    
    @@ -86,11 +87,12 @@ public function addCheckService($service_id, $service_method, array $applies_che
    +   *   (Optional) Run access checks for this account. Defaults to the current
    

    Lowercase o in optional

  3. +++ b/core/lib/Drupal/Core/Url.php
    @@ -412,6 +415,53 @@ public function getInternalPath() {
    +  /**
    +   * Sets the access manager.
    +   *
    +   * @param \Drupal\Core\Access\AccessManagerInterface $access_manager
    +   */
    +  public function setAccessManager(AccessManagerInterface $access_manager) {
    +    $this->accessManager = $access_manager;
    +  }
    

    Do we need this or is it just for tests?

dawehner’s picture

@tim.plunkett
Can you write down what you figured out in IRC?

chx’s picture

FileSize
4.62 KB
31.83 KB

> getControllerFromDefinition supports much more than just class::method. I think you can just always call this and assign it to $callable, right?

Nope. But that's not in scope for this issue as this code is present in drupal_render twice already. Please file a followup for better doxygen. When I tried to summarize it, I failed. This might need a helper method with a longish doxygen.

> Lowercase o in optional

Done; also fixed relevant doxygen at another spot.

> Do we need this or is it just for tests?

Moved it into a TestUrl class.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, #67.1 is the fault of #2247779: Allow #pre_render, #post_render and #post_render_cache callbacks to be service methods.

Note that in #2247779-32: Allow #pre_render, #post_render and #post_render_cache callbacks to be service methods @catch called this out as something needing to be documented.

#2323301: Deprecate the "two double colons" check in Renderer::doCallback() was opened to ostensibly address that, but it's still not documenting it, just trying to "fix" it.

Since that is a pre-existing weirdness, RTBC for the rest.

chx’s picture

The gift that keeps giving: #2333265: Add access checking to link render structures adds the access callback introduced in this issue to a number of places and shows that we are displaying an ungodly amount of links which might lead to pages the users can't access.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Url.php
    @@ -412,6 +414,45 @@ public function getInternalPath() {
    +      $this->accessManager = \Drupal::service('access_manager');
    

    Property is not documented.

  2. +++ b/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php
    @@ -64,11 +49,9 @@ class SystemBrandingBlock extends BlockBase implements ContainerFactoryPluginInt
    -  public function __construct(array $configuration, $plugin_id, $plugin_definition, ConfigFactoryInterface $config_factory, UrlGeneratorInterface $url_generator, AccountInterface $current_user) {
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, ConfigFactoryInterface $config_factory) {
    

    The doc block needs a prune :)

  3. +++ b/core/modules/user/user.module
    @@ -1101,6 +1101,10 @@ function user_user_role_insert(RoleInterface $role) {
    +  if (!\Drupal::entityManager()->hasDefinition('action')) {
    +    return;
    +  }
    

    This needs a comment - also how do we end up with this? User depends on system which provides the action entity afaik. Or is this for Kernel tests - and if so then we need to enable the system module in my opinion.

  4. +++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
    @@ -12,6 +12,7 @@
    +use Drupal\Core\Session\AccountProxy;
    

    Not used.

  5. +++ b/core/tests/Drupal/Tests/Core/UrlTest.php
    @@ -318,4 +324,79 @@ public function testGetOptions($urls) {
    +   * @param \Drupal\Core\Session\AccountInterface|NULL $account
    +   */
    +  protected function getMockAccessManager($access, $account = NULL) {
    

    Let's document the return type.

chx’s picture

Status: Needs work » Needs review
FileSize
31.62 KB
3.09 KB

3. I simply removed; let's see whether it fails. The rest is addressed.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Yeah the fix for actions was wrong, but I know that there was a plugin not found exception at some point. Maybe this got fixed in the meantime by properly clearing some cache on module install.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6115051 and pushed to 8.0.x. Thanks!

  • alexpott committed 6115051 on 8.0.x
    Issue #2302563 by chx, dawehner: Fixed Access check Url objects.
    

Status: Fixed » Closed (fixed)

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