Problem/Motivation
Right now access checking happens because of the page flow not because of routing -- it's the kernel firing an event that causes access checking. Now, if you are doing something funny and route another request then you don't get access checking. This is a security weakness. Also an event subscriber can fire between the route matching and the access checker. This is a security weakness too.
This is a regression: in Drupal 7 you got a menu item by calling $item = menu_get_item($path)
then you could just do $item && $item['access']
(arguably, that's not best and access denied should've changed $item itself into FALSE but that's past). Now, compare this to the current drupal_valid_path
: you get a route from the route provider and then need to get a request, run a match and run an access check. This is extremely error prone and if a module author makes an error here that means she now has a sechole.
However as we learned in #2302563: Access check Url objects a route object in itself is not something that can be access checked; only together with parameters it can. That issue explores putting access checks of route derived objects this one explores putting access check on the route matching process which adds the route parameters to the request.
Proposed resolution
Remove the access subscriber. Add a simple class that wraps ChainRouter in all but the actual matching (matchRequest and match). Run the access check from matchRequest and make match call matchRequest . One can turn to the router.insecure
service to get these done in a non-access-aware manner. Now the system defaults to secure and yet you can make a conscious decision to run insecure checks as Url::createFromPath indeed does.
Remaining tasks
Find out how else this can be exploited and then raise this to critical. I'm quite sure there is something really bad lurking here.
If this approach is accepted then file a followup to change the $access_check argument in match and createFromPath to default TRUE -- security should be opt out and not opt in (*cough*) but this patch is truly about just tightening up matchRequest, we can (and will) tighten match in the next step.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#60 | interdiff.txt | 3.28 KB | dawehner |
#60 | router-access-2322809-59.patch | 33.53 KB | dawehner |
#56 | router-access-2322809-56.patch | 32.4 KB | tim.plunkett |
#54 | 2322809_53.patch | 32.36 KB | chx |
#51 | 2322809_51.patch | 32.35 KB | chx |
Comments
Comment #1
chx CreditAttribution: chx commentedAh yes, match needs to do this too.
Comment #2
chx CreditAttribution: chx commentedComment #5
chx CreditAttribution: chx commentedOdd. I tested this locally.
Comment #7
chx CreditAttribution: chx commentedOh, doh! Shortcut is running Url::createFromPath() which calls
\Drupal::service('router')->match('/' . $path, $access_check);
which in turns throws a nice access denied cos we are anonymous at this point. So, I relaxed match a little. The point is tightening matchRequest. We can do more with this later -- and having at least an optional check on createFromPath is quite good -- it complements #2302563: Access check Url objects nicely.Comment #8
chx CreditAttribution: chx commentedComment #10
chx CreditAttribution: chx commentedHopefully this fixes the test failures and also I wrote a test for the router class. Perhaps we should test the match method as well -- but for now, it's testing matchRequest, quite simple tests just takes a bit of setup.
Comment #12
chx CreditAttribution: chx commentedQuite odd but let's try to separate out the problems -- here's a match() version not doing any access checks.
Comment #13
chx CreditAttribution: chx commentedComment #15
Crell CreditAttribution: Crell commentedSo... the idea here is to make it harder to do route matching without running access checks? That seems counter-productive. They are separate tasks in my mind. If someone wants to assemble these components together in such a way as to not have access checks at all in the system, they should be able to. Tying things together in order to make it less flexible in the name of protecting people from themselves doesn't have a great track record in Drupal. I would need to see something more than a "gut feeling" to reduce decoupling here.
Some code-related comments while I'm at it:
AccessAwareRouter?
Even if this patch does move forward, the access check parts are still a separate task from the match itself. Move that to a separate method within the class.
What we really want here is finally {}, but that wasn't added until PHP 5.5. :-( This is a really screwy way of emulating that, though. What's wrong with the approach in the code being replaced?
Comment #16
chx CreditAttribution: chx commentedFTFY. That's what a secure system is: protects people from their stupidity at all costs. We make APIs that are easy to use securely and hard to use insecurely. Compare the quite stellar track record of core security to #2264041: Add a test to ensure title callbacks are not vulnerable to XSS (write up at drupal4hu.com/node/404 ).
Mind you, there is already a security weakness here as the IS mentions: weight yourself just below the access subscriber and be screwed.
Also, if you call matchRequest with a made up request then you won't get an access check at all. Actually core does that already...
Both of these are valid weaknesses.
Ongoing, this will make Url more secure too.
> AccessAwareRouter
renamed. The test too.
> Move that to a separate method within the class.
Moved.
> What's wrong with the approach in the code being replaced?
Code repetition.
Comment #17
chx CreditAttribution: chx commentedComment #18
chx CreditAttribution: chx commentedNote what this boils down to: 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. This is what Drupal 8 moved from and it must move back.
Comment #20
chx CreditAttribution: chx commentedShould be down to three fails in ConfigTranslationUiTest . The fatal was in UrlTest::testCreateFromPath() as the mock route is using returnValueMap and that is strict about supplying all parameters including optional ones.
Comment #22
chx CreditAttribution: chx commentedThis has those three fails but I can't resist submitting this because -- PathValidator exactly does the "let's create a new request" dance and look how much simpler it became. You can just look at the diff of PathValidator.php to see the fundamental architectural difference here:
$this->requestMatcher->matchRequest($request);
does access checking by itself. Updated the issue summary too.Comment #24
chx CreditAttribution: chx commentedLet's hope this passes tests.
Comment #25
realityloopCorrect me if I'm wrong..
My first thought when reading @crell's comment is that I'd expect that if a particular use case didn't need access checks it would be easy enough to write a custom module that returned true in all instances where it wasn't already handled by the permissions system.
Personally I'm in favor of having security as a priority especially where it may protect me from making a site insecure accidentally.
@crell I'd be really interested to hear and example of where this may be counter productive.
Comment #26
tim.plunkettThis patch has changed since @Crell's comments. It now leaves the original router intact, but with a different service name.
I think this is a sufficient allowance for code that wants to run matching without applying access checks.
Comment #27
chx CreditAttribution: chx commented> This patch has changed since @Crell's comments. It now leaves the original router intact, but with a different service name.
Erm, nope, we always made the service available, #24 had it as router.unsafe but safety and security is not the same so I renamed it to router.insecure and added this to the IS.
Comment #28
tim.plunkettI think this hierarchy is a little strange. Why not have AccessAwareRouterInterface extend RequestMatcherInterface as well?
These need full namespaces and one-liners
Drupal
Missing docs for the other params. And this should clarify why there is a second router.
So now this interface is exactly the same as far methods and params.
The only difference is the documented exceptions. Do you think a new interface is needed, or can they be on the implementing class?
Not needed
This deserves a comment about why we purposefully use router.insecure
You remove some classes here, but not their use statements.
Well, these were nice for clarity.
This says matchrR, with an extra "r"
Comment #31
chx CreditAttribution: chx commentedAddressed them.
Comment #33
chx CreditAttribution: chx commentedEasy to fix, these fails are. I provided an extra interdiff to 27 so that it's easier to check that the review is addressed.
Comment #36
tim.plunkettI think this adequately addresses @Crell's concerns, and I like where we ended up. Thanks for working through this @chx!
Comment #37
chx CreditAttribution: chx commentedIf someone is worried that Url is running route.insecure I filed a followup to check whether it can be removed, made optional , etc.
Comment #38
chx CreditAttribution: chx commentedAddressing dawehner's review from IRC
finally
once PHP 5.5 is available.$this->container->get('router')->setCurrentUser($account);
as unnecssary.Comment #39
chx CreditAttribution: chx commentedOne more small change.
Comment #40
dawehnerCool
this. Comitters will see this anyway.
Comment #41
chx CreditAttribution: chx commentedOK. fixed that.
Comment #44
dawehnerNo it didn't, yes it did, not it didn't!
Comment #45
catchPretty sure it should be unsecure, not insecure.
Also it isn't unsecure if you use it properly, could do with a more descriptive name.
While it's true that Url objects might be created and stored for different users, this is also the most common place where you might or might not want access checks. Are you supposed to check access to the route before using this? I don't see any docs in the patch on how to do that.
Comment #46
chx CreditAttribution: chx commentedhttp://english.stackexchange.com/a/19655
http://en.wikipedia.org/wiki/Computer_insecurity
and so on.
For Url objects, there is #2323721: [sechole] Link field item and menu link information leakage and #2302563: Access check Url objects the latter adding the access call on the Url object itself that's why there's nothing here. This is being solved , right now just not in this issue.
Comment #47
chx CreditAttribution: chx commentedDocumented how the Url factory methods work regarding security.
Comment #48
dawehnerThanks for clarifying this!
Comment #49
chx CreditAttribution: chx commentedRenamed to
router.no_access_checks
.Comment #51
chx CreditAttribution: chx commentedTypo in core.services.yml
Comment #54
chx CreditAttribution: chx commentedSigh. This is so hard.
Comment #56
tim.plunkettTrivial fix.
Comment #57
dawehnerWhile reading this I realized that we potentially introduced a non-trivial regression.
Comment #59
tim.plunkettThat expectation shouldn't be in setUp, but in the methods that need it.
Comment #60
dawehnerSometimes phpunit is quite lovely!
Comment #61
tim.plunkettAh, yours is better.
Comment #62
dawehnerOh I didn't actually wanted to do a
issue-queue push --force
but well, this happened :(Comment #63
alexpottCommitted 450cc98 and pushed to 8.0.x. Thanks!
Fixed on commit.
Comment #65
chx CreditAttribution: chx commentedRe #15
How about a security hole? #2323721: [sechole] Link field item and menu link information leakage
Comment #67
Fabianx CreditAttribution: Fabianx commentedTagging a parent issue and adding a title related to the new security initiative