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
- Put documentation on
Url::generateFromPath
andUrl::generateFromRequest
to pass only validated paths to it. Remove thetry-catch
. - Add
link to any page
permission. - Inject both
@router
and@router.no_access_checks
intoPathValidator
and make it run the right service based on this permission. - Remove the
_menu_admin
hack completely. - Make everyone involved use
PathValidator
instead ofUrl::generateFromPath
when validating, includingLinkTypeConstraint::validate
andMenuLinkContentForm::extractUrl
and everything else that (ab)usedUrl::generateFromPath
in such ways. Add agetUrlIfValid
method to thePathValidator
.
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
Comment | File | Size | Author |
---|---|---|---|
#64 | 2323721-followup-64.patch | 692 bytes | David_Rothstein |
#48 | 2323721_48.patch | 58.97 KB | chx |
#45 | interdiff.txt | 1.85 KB | dawehner |
#45 | 2323721-path_validator-45.patch | 58.79 KB | dawehner |
#43 | interdiff.txt | 1.73 KB | dawehner |
Comments
Comment #1
dawehnerThere 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.
Comment #2
chx CreditAttribution: chx commentedComment #3
chx CreditAttribution: chx commentedComment #5
dawehnerLet's ensure that we actually cover the new code with test coverage.
Comment #6
dawehner.
Comment #7
chx CreditAttribution: chx commentedComment #9
chx CreditAttribution: chx commentedEventually 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.Comment #11
chx CreditAttribution: chx commentedThis 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 theentity_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.Comment #13
dawehnerFixed the remaining failures.
Comment #14
chx CreditAttribution: chx commentedWe will need to patch + test
createFromRequest
too.Comment #15
chx CreditAttribution: chx commentedComment #16
pwolanin CreditAttribution: pwolanin commentedIn LinkFieldTest, let's just use array() notation consistently, rather than mixing notations in the file
Comment #17
Crell CreditAttribution: Crell commentedThat'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.)
Comment #18
pwolanin CreditAttribution: pwolanin commentedSo, 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
Comment #19
chx CreditAttribution: chx commentedPutting access check in
url
-- yes.Implementing a SUPER privilege that uid 1 simply has -- yes.
Calling secholes usability bugs -- no.
Comment #20
pwolanin CreditAttribution: pwolanin commentedI 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"?
Comment #21
chx CreditAttribution: chx commentedComment #22
chx CreditAttribution: chx commentedThe patch doesn't reflect the new issue summary at all.
Comment #23
dawehnerComment #24
dawehnerComment #25
chx CreditAttribution: chx commentedWonderful. 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:
Comment #27
dawehnerO right, I got distracted while working on that.
Fixed a couple of bits, not done yet, though.
Comment #29
dawehnerNot 100% yet, but we are getting there.
Comment #30
dawehnerForgot the interdiff.
Comment #31
dawehnerThis time with the rest of it.
Comment #33
dawehnerSome small details.
Comment #34
tim.plunkettThis 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
''
:(Comment #35
dawehnerWe 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
Comment #36
dawehnerSo we will have proper exception message in CMF 1.3, so let's get this in?
Comment #37
chx CreditAttribution: chx commentedFixed 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.
YES! That's how secure code is written: by default it is not secure! Very good.
Comment #39
dawehner80 chars :(
Comment #40
dawehnerRequest::create("/$path");
Comment #41
chx CreditAttribution: chx commentedOK.
Comment #43
dawehnerFixed it.
Comment #44
Crell CreditAttribution: Crell commentedWhy is this referencing the permission as a string...
... When there's a constant defined? And why a constant for this permission unlike the 400 other permissions we have floating about the system?
This is in a plugin. It can be injected.
Other than those nits, I very much like where this has ended up. +1.
Comment #45
dawehnerWell, 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.
I am really happy about that statement! High five!
It is a plugin, but it doesn't use the plugin factory. Feel free to complain :P Here is a backtrace.
Comment #48
chx CreditAttribution: chx commentedBloody stubborn patch.
Comment #50
chx CreditAttribution: chx commentedI tried #33 on
admin/structure/menu/manage/main/add
and I getThe 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:How #33 passed is a mystery.
Comment #51
dawehnerThere we go.
Comment #52
chx CreditAttribution: chx commentedLet's try it again :)
Comment #53
webchickWith 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.
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.
Comments please. :)
This seems both non-standard and also like twice the number of characters to type. Can we just check the permission name directly?
Do we not have a way to avoid introducing a $GLOBALS here?
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.
Comment #54
dawehnerAdressed the feedback.
Comment #56
chx CreditAttribution: chx commentedLet'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.
Comment #57
chx CreditAttribution: chx commentedNah, it's ltrim ; ltrim is not applicable; it doesn't take a string ; it takes a list of characters. interdiff against #54.
Comment #59
chx CreditAttribution: chx commentedYay green
Comment #60
webchickThat looks like everything.
Committed and pushed to 8.x. Thanks!
Comment #62
Wim LeersHurray! :)
Comment #63
David_Rothstein CreditAttribution: David_Rothstein commentedJust 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:
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.
Comment #64
David_Rothstein CreditAttribution: David_Rothstein commentedAdding 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.
Comment #66
chx CreditAttribution: chx commentedIt failed. Please follow up in a new issue.
Comment #67
David_Rothstein CreditAttribution: David_Rothstein commentedActually 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?
Comment #69
David_Rothstein CreditAttribution: David_Rothstein commentedAnd a couple no-op patches to help see if it's an instability.
Comment #70
David_Rothstein CreditAttribution: David_Rothstein commentedI 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?
Comment #71
chx CreditAttribution: chx commentedRegarding #64, yes that's RTBC. Sorry for not looking at it properly!
Regarding #63 that's essentially it, yes.
Comment #72
chx CreditAttribution: chx commentedComment #73
webchickCommitted and pushed #64 to 8.x. Thanks!
Comment #75
David_Rothstein CreditAttribution: David_Rothstein commentedThanks :)
Comment #76
YesCT CreditAttribution: YesCT commentedcaused #2330751: Random test failures everywhere due to ->with(Request::create())
Comment #77
chx CreditAttribution: chx commentedRegarding random test failures (which I have caught, mind you), let me quote myself from https://www.drupal.org/node/2322809#comment-9063371
A random test failure which lived a week is no reason to shun patches like this. If lasts a year it is still not.
Comment #78
chx CreditAttribution: chx commentedComment #80
andypostI 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