Problem/Motivation

LinkTypeConstraint::validate does not do access checking. It is possible to link to paths the user can not access. This is information leakage (especially in the presence of pathauto or similar). This behavior is tested(!) in LinkFieldTest by adding a link to admin which the test user can not access.

Here's how the information is leaked: a menu link is created to for eg. "node/1" then the menu UI generates a link to that page for me using the actual URL alias (which might be something like "/secret-plan-to-destroy-the-world") then I can get information from the URL that I otherwise wouldn't be able to.

Proposed resolution

  1. Put documentation on Url::generateFromPath and Url::generateFromRequest to pass only validated paths to it. Remove the try-catch.
  2. Add link to any page permission.
  3. Inject both @router and @router.no_access_checks into PathValidator and make it run the right service based on this permission.
  4. Remove the _menu_admin hack completely.
  5. Make everyone involved use PathValidator instead of Url::generateFromPath when validating, including LinkTypeConstraint::validate and MenuLinkContentForm::extractUrl and everything else that (ab)used Url::generateFromPath in such ways. Add a getUrlIfValid method to the PathValidator.

Remaining tasks

User interface changes

As in Drupal 6-7, you can not link to something you can't access.

Field link items store the system path (route name+parameters, actually) but the default widget shows paths aliased irregardless of whether they were entered with an an alias or not. Given that's how Drupal displays them that seems the right behavior. D7 stored the system path too. Storing the alias would require a mass change in stored links if we were to store the alias.

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

There are places where you need pure API functions which does not fail, when the current user does not have access.
If you are worried then you should also put access restrictions on entity field API.

chx’s picture

FileSize
4.43 KB
chx’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2323721_2.patch, failed testing.

dawehner’s picture

Issue summary: View changes
FileSize
7.82 KB
4.85 KB

Let's ensure that we actually cover the new code with test coverage.

dawehner’s picture

Status: Needs work » Needs review

.

chx’s picture

Title: Investigate the use of route.insecure in Url » [sechole] Link field item information leakage
Category: Task » Bug report
Issue summary: View changes
Priority: Normal » Critical

Status: Needs review » Needs work

The last submitted patch, 5: 2323721-5.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
12.05 KB

Eventually we will need to patch + test createFromRequest too. But for now, here's a patch that fixes LinkFieldTest actually doing this and includes #2326515: Link field item is broken when Drupal is in a subdir and hopefully gets closer to pass.

Status: Needs review » Needs work

The last submitted patch, 9: 2323721_8.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
12.4 KB

This won't pass either but I hope dawehner have some good ideas as to what to do with this: right now the test expects a/path/alias in the link item but instead finds the entity_test/add system path ie. dealiased. I copied the code from menu, mostly, I really do not know what is happening here and why do we do all these conversions.

Status: Needs review » Needs work

The last submitted patch, 11: 2323721_11.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.23 KB
7.68 KB

Fixed the remaining failures.

chx’s picture

Title: [sechole] Link field item information leakage » [sechole] Link field item and menu link information leakage
Issue summary: View changes

We will need to patch + test createFromRequest too.

chx’s picture

Issue summary: View changes
pwolanin’s picture

In LinkFieldTest, let's just use array() notation consistently, rather than mixing notations in the file

Crell’s picture

As in Drupal 6-7, you can not link to something you can't access.

That's actually a long-standing usability bug with the D6/7 menu/routing system: #308263: Allow privileged users to bypass the validation of menu items (Or possibly just related to.)

pwolanin’s picture

So, I think that argues more against putting the access check in Url - there should be an explicit permission-based access bypass for creating links to any valid route. That would be better than some of the current hack-arounds

chx’s picture

Putting access check in url -- yes.

Implementing a SUPER privilege that uid 1 simply has -- yes.

Calling secholes usability bugs -- no.

pwolanin’s picture

I still don't think the Url class should have this logic - it should be specific to link field and menu links.

We need to add a new permission like "link to any page" or "link to any route"?

chx’s picture

Issue summary: View changes
chx’s picture

Status: Needs review » Needs work

The patch doesn't reflect the new issue summary at all.

dawehner’s picture

Issue summary: View changes
dawehner’s picture

Status: Needs work » Needs review
FileSize
31.94 KB
chx’s picture

Issue summary: View changes
Status: Needs review » Needs work

Wonderful. It truly is. PathValidator gets pushed to the limelight and simplified at the same time. Code duplication is decreased restoring the "single path check" system I was so proud of in Drupal 6-7. And that caused this so-called usability problem which gets resolved in one truly glorious patch.

Couple notes:

  1. testCheckAccessWithLinkToAnyPagePermission has a bunch of commented out code
  2. in system_permission 'link to any page' => IMO needs restrict access
  3. as discussed on IRC, let's add a getUrlIfValid -- the current validator already does everything that Url::createFromPath used to do and code often calls Url::createFromPath after so let's no run the same matching twice
  4. Speaking of running the same matching twice, there is no need for try-catch in Url::createFromPath -- the path passed in has already ran through the exact same matching

The last submitted patch, 24: url_access-2323721-24.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
39.35 KB
18.61 KB

testCheckAccessWithLinkToAnyPagePermission has a bunch of commented out code

O right, I got distracted while working on that.

Fixed a couple of bits, not done yet, though.

Status: Needs review » Needs work

The last submitted patch, 27: 2323721-27.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
55.6 KB

Not 100% yet, but we are getting there.

dawehner’s picture

FileSize
28.01 KB

Forgot the interdiff.

dawehner’s picture

FileSize
56.81 KB
2.83 KB

This time with the rest of it.

The last submitted patch, 29: 2323721-29.patch, failed testing.

dawehner’s picture

FileSize
55.59 KB
1.96 KB

Some small details.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Url.php
@@ -123,14 +126,9 @@ public static function createFromPath($path) {
-        throw new MatchingRouteNotFoundException(sprintf('No matching route could be found for the path "%s"', $path), 0, $e);

@@ -152,14 +153,9 @@ public static function createFromPath($path) {
-      throw new MatchingRouteNotFoundException(sprintf('No matching route could be found for the request: %s', $request), 0, $e);

This is documented in @throws on Url, and use'd in LinkTypeConstraint. Also, it's now a completely unused exception class.
Should we just remove the class?

It's a shame, because that was a useful exception message, and the new one is '' :(

dawehner’s picture

We filled some issue against upstream to improve the exception messages: https://github.com/symfony-cmf/Routing/pull/110 https://github.com/symfony/symfony/pull/11767

dawehner’s picture

So we will have proper exception message in CMF 1.3, so let's get this in?

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
56.41 KB
4.42 KB

Fixed a couple minor issues but nothing substantial enough that I would feel I am RTBC'ing my own code. Also, written big scary warnings on the url factory methods.

-      $url_is_valid = TRUE;
+      $url_is_valid = FALSE;

YES! That's how secure code is written: by default it is not secure! Very good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 37: 2323721_37.patch, failed testing.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Url.php
@@ -121,25 +124,15 @@ public static function createFromPath($path) {
+   * Returns the Url object matching a request. READ THE SECURITY NOTE ON createFromPath().

80 chars :(

dawehner’s picture

+++ b/core/lib/Drupal/Core/Url.php
@@ -118,27 +124,15 @@ public static function createFromPath($path) {
+      return static::createFromRequest(Request::create($path));

Request::create("/$path");

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
56.41 KB
439 bytes

OK.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 41: 2323721_41.patch, failed testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +WSCCI
FileSize
58.01 KB
1.73 KB

Fixed it.

Crell’s picture

  1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
    @@ -93,6 +93,9 @@ public function checkAccess(array $tree) {
    +    if ($this->account->hasPermission('link to any page')) {
    

    Why is this referencing the permission as a string...

  2. +++ b/core/lib/Drupal/Core/Path/PathValidator.php
    @@ -21,66 +25,115 @@
    +  const LINK_PERMISSION = 'link to any page';
    

    ... When there's a constant defined? And why a constant for this permission unlike the 400 other permissions we have floating about the system?

  3. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php
    @@ -53,36 +53,20 @@ public function validatedBy() {
    +        if ($url = \Drupal::pathValidator()->getUrlIfValid($url_string)) {
    

    This is in a plugin. It can be injected.

Other than those nits, I very much like where this has ended up. +1.

dawehner’s picture

... When there's a constant defined? And why a constant for this permission unlike the 400 other permissions we have floating about the system?

Well, i just like the better semantic behaviour of having a permission. It also felt wrong to introduce a permission inside Drupal/Core but at least having it properly defined
makes it somehow to act like an interface.

Other than those nits, I very much like where this has ended up. +1.

I am really happy about that statement! High five!

This is in a plugin. It can be injected.

It is a plugin, but it doesn't use the plugin factory. Feel free to complain :P Here is a backtrace.

Drupal\link\Plugin\Validation\Constraint\LinkTypeConstraint->__construct()
Symfony\Component\Validator\ConstraintValidatorFactory->getInstance(Object)
Symfony\Component\Validator\ExecutionContext->executeConstraintValidators(Object, Array)
Symfony\Component\Validator\ExecutionContext->validateValue(Object, Array)
Symfony\Component\Validator\ValidationVisitor->visit(Object, Object, 'Default', '0')
Drupal\Core\TypedData\Validation\PropertyContainerMetadata->accept(Object, Object, 'Default', '0')
Drupal\Core\TypedData\Validation\PropertyContainerMetadata->accept(Object, Object, 'Default', '')
Symfony\Component\Validator\ValidationVisitor->validate(Object, 'Default', '', , )
Symfony\Component\Validator\Validator->validate(Object)
Drupal\Core\TypedData\TypedData->validate()
Drupal\entity\Entity\EntityFormDisplay->validateFormValues(Object, Array, Object)
Drupal\Core\Entity\ContentEntityForm->validate(Array, Object)
Drupal\node\NodeForm->validate(Array, Object)
call_user_func_array(Array, Array)
Drupal\Core\Form\FormValidator->executeValidateHandlers(Array, Object)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'article_node_form')

The last submitted patch, 43: 2323721-access-43.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 45: 2323721-path_validator-45.patch, failed testing.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
58.97 KB
654 bytes

Bloody stubborn patch.

Status: Reviewed & tested by the community » Needs work

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

chx’s picture

I tried #33 on admin/structure/menu/manage/main/add and I get The path '<front>?arg1=value#fragment' is either invalid or you do not have access to it.. And that's what we pass to the pathValidator:

    $extracted = $this->pathValidator->getUrlIfValid($form_state->getValue('url'));

How #33 passed is a mystery.

dawehner’s picture

Status: Needs work » Needs review
FileSize
61.31 KB
4.14 KB

There we go.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Let's try it again :)

webchick’s picture

With the help of dawehner and chx, tested the behavior before/after the patch. Overall, this is removing a whole lot of convoluted code which is a very welcome improvement.

+++ b/core/modules/shortcut/src/ShortcutForm.php
@@ -65,7 +96,7 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
-    if (!shortcut_valid_link($form_state->getValue('path'))) {
+    if (!$this->pathValidator->isValid($form_state->getValue('path'))) {

An API for this is very welcome; I like how we eliminate a bunch of one-off access functions (no two of which are checking access in exactly the same way ;)) in favor of this one approach.

  1. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
    @@ -151,7 +151,11 @@ public static function parse($url) {
    +      list(, $path) = explode('://', $parts[0], 2);
    +      if ($path != '') {
    +        $options['path'] = $parts[0];
    +      }
    

    Comments please. :)

  2. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
    @@ -93,6 +94,9 @@ public function checkAccess(array $tree) {
    +    if ($this->account->hasPermission(PathValidator::LINK_PERMISSION)) {
    
    +++ b/core/lib/Drupal/Core/Path/PathValidator.php
    @@ -21,66 +25,124 @@
    +  const LINK_PERMISSION = 'link to any page';
    

    This seems both non-standard and also like twice the number of characters to type. Can we just check the permission name directly?

  3. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -204,32 +200,16 @@ public function validateTitle(&$element, FormStateInterface $form_state, $form)
    +        // Reset the URL value to contain only the path.
    +        if (!$url->isExternal() && $this->supportsInternalLinks()) {
    +          $value['url'] = substr($url->toString(), strlen($GLOBALS['base_path']));
             }
    

    Do we not have a way to avoid introducing a $GLOBALS here?

  4. +++ b/core/modules/system/system.module
    @@ -261,6 +261,10 @@ function system_permission() {
    +    'link to any page' => [
    +      'title' => t('This allows to bypass access checking when linking to internal paths.'),
    +      'restrict access' => TRUE,
    +    ],
    

    That looks like a description. Title should be something like "Link to any page." This should be fixed because it makes that column of the permissions page out of whack.

    Also, my impressions of this permission were wrong, since by "any page" I thought it meant restoring the D5 behaviour of being able to build out your menu tree ahead of time, even to pages that don't exist yet. Only suggestion would be "Link to any existing page" but it's not a huge deal unless others are left with the same impression.

dawehner’s picture

FileSize
61.36 KB
3.45 KB

Adressed the feedback.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 54: path_validator-2323721-54.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
61.35 KB
625 bytes

Let's try a revert of the base path change -- I submitted a getBasePath version to https://www.drupal.org/node/2320269#comment-9103167 without much success. I will debug the failure meanwhile.

chx’s picture

FileSize
61.28 KB
643 bytes

Nah, it's ltrim ; ltrim is not applicable; it doesn't take a string ; it takes a list of characters. interdiff against #54.

The last submitted patch, 56: path_validator-2323721-54.patch, failed testing.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Yay green

webchick’s picture

Status: Reviewed & tested by the community » Fixed

That looks like everything.

Committed and pushed to 8.x. Thanks!

  • webchick committed 68f576f on 8.0.x
    Issue #2323721 by dawehner, chx: Fixed [sechole] Link field item and...
Wim Leers’s picture

Remove the _menu_admin hack completely.

Hurray! :)

David_Rothstein’s picture

It is possible to link to paths the user can not access. This is information leakage (especially in the presence of pathauto or similar).

Just to clarify, since this is being discussed for possible Drupal 7 backport in #308263: Allow privileged users to bypass the validation of menu items, is the security concern here (that requires adding a new permission with "restrict access" set to TRUE) the fact that it allows me to see URL aliases of content I don't have access to - i.e. if I create a menu link to "node/1" and the menu UI generates a link to that page for me using the actual URL alias (which might be something like "/secret-plan-to-destroy-the-world") then I can get information from the URL that I otherwise wouldn't be able to? Or is there something else also?

While trying to figure out the above, I skimmed through the patch and also noticed this small issue:

--- a/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
+++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
.......
- array('path' => ''),
........
+ array('path' => '<front>', 'route_name' => '<front>'),

That new code is not testing the same thing as the old code. See #934714: Cannot add frontpage as shortcut for why the old test is important.

David_Rothstein’s picture

Status: Fixed » Needs review
FileSize
692 bytes

Adding that test back looks as simple as this, and might actually pass...

I'll create a new issue if the tests fail and it turns out to be more complicated than that, but otherwise this is a pretty simple followup.

Status: Needs review » Needs work

The last submitted patch, 64: 2323721-followup-64.patch, failed testing.

chx’s picture

Status: Needs work » Fixed

It failed. Please follow up in a new issue.

David_Rothstein’s picture

Status: Fixed » Needs review

Actually the test that failed (Drupal\Tests\Core\UrlTest) is not the one that the patch modifies. So this is either a completely random failure, or something else is badly broken (the patch that was committed here did touch Drupal\Tests\Core\UrlTest pretty heavily and was the most recent commit to do so, so could be some kind of new instability introduced via this issue?). Let's try one more time to double-check.

Any thoughts on the question I asked above?

David_Rothstein’s picture

FileSize
309 bytes
309 bytes

And a couple no-op patches to help see if it's an instability.

David_Rothstein’s picture

I didn't post enough no-op patches to be sure, but at least there's no evidence of a widespread instability, hopefully just a single random failure.

The followup in #64 now passes, so RTBC?

chx’s picture

Regarding #64, yes that's RTBC. Sorry for not looking at it properly!

Regarding #63 that's essentially it, yes.

chx’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed #64 to 8.x. Thanks!

  • webchick committed 285df2c on 8.0.x
    Issue #2323721 follow-up by David_Rothstein: Restore missing test.
    
David_Rothstein’s picture

Thanks :)

YesCT’s picture

chx’s picture

Regarding random test failures (which I have caught, mind you), let me quote myself from https://www.drupal.org/node/2322809#comment-9063371

what's the first driving principle of Drupal API design? Security. Everything else comes as a rather distant second, including performance, usability and all that jazz

A random test failure which lived a week is no reason to shun patches like this. If lasts a year it is still not.

chx’s picture

Issue summary: View changes

Status: Fixed » Closed (fixed)

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

andypost’s picture

I filed regression issue #2463753: [regression] Do not bypass route access with 'link to any page' permissions for menu links
there's no way now to hide menu links from UID1 and users with link to any page permission