Problem/Motivation
The secure way to make "links that do things" is to have the link include a token in the URL in a GET parameter, and then check that parameter as part of the access callback on the menu item.
Of course, that is something everyone has to do themselves, which means developers frequently forget to do so.
Proposed resolution
Whether or not a route needs an XSRF token on it should be centralized. In the new routing system, we can flag a route as needing such a token. Then we can always access check for it, and when generating URLs to that route we can always inject the token. That makes it a one-line task for developers.
This issue depends on:
#1606794: Implement new routing system
#1874500: CMF-based Routing system
and (maybe) #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence
Once we have a generator, this should be super easy to do:
1) Add an "xsrf" => TRUE option to a Route.
2) In the Generator, check and see if the route has an xsrf option set. If so, add a token=[token value goes here] GET parameter to it.
3) Add a new request listener (after the matcher listener) that checks if a route requires an XSRF token. If so, check if the incoming request has one and a valid one. If not, throw a 403 exception.
3) Add a new Access Checker that runs if the route has an xsrf tag, and if so check that the incoming request has a valid one. If not, return FALSE to let the access system throw a 403.
Remaining tasks
Get the above issues committed, then implement this. :-)
User interface changes
None.
API changes
Adds a new option to Route object definitions.
Comment | File | Size | Author |
---|---|---|---|
#104 | 1798296-typo.patch | 489 bytes | andypost |
#96 | interdiff-93-96.txt | 781 bytes | David_Rothstein |
#96 | 1798296-96.patch | 36.29 KB | David_Rothstein |
#93 | interdiff-1798296-93.txt | 7.59 KB | damiankloip |
#93 | 1798296-93.patch | 36.28 KB | damiankloip |
Comments
Comment #1
sunPrior art: #755584: Built-in support for csrf tokens in links and menu router
Related: #961508: Dual http/https session cookies interfere with drupal_valid_token()
I also think you wanted to say CSRF wherever you said XSRF. While dictionaries surprisingly yield it as a synonym, CSRF is the proper industry term.
Comment #2
Crell CreditAttribution: Crell commentedI knew there was an existing issue, but couldn't find it. Thanks, sun. (Also, I've heard the terms interchangeably. Meh.)
Comment #3
Crell CreditAttribution: Crell commentedTagging
Comment #4
Gaelan CreditAttribution: Gaelan commentedPossible duplicate of #755584: Built-in support for csrf tokens in links and menu router.
Comment #5
Crell CreditAttribution: Crell commentedI think that issue should close in favor of this one, which we should be able to work on as soon as #1874500: CMF-based Routing system happens. (That introduces the generator, which is where we would do this sort of work.
Comment #6
XanoThe patch implements Crell's original idea, with the addition that it allows routes to use a custom parameter that is passed on to drupal_get_token(). This value may be left empty. The access check simply responds to the presence of a _csrf key in the route requirements.
Comment #8
Crell CreditAttribution: Crell commentedWe've standardized to CsrfAccessCheck, I believe.
The token system needs to become a service that we can inject.
To really do this right, though, we should also implement an OutboundPathProcessor, perhaps one for just routes, that will add a token to an outgoing link automagically. Then there's no involvement from the user.
That requires OutboundPathProcessors, which are coming as part of #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence. Although, that may cause problems for #1965074: Add cache wrapper to the UrlGenerator. Hm...
Comment #9
XanoUh, we don't properly write abbreviations in all caps? Alright.
Shouldn't we do that in a follow-up?
I'm not familiar with those, but how do CSRF tokens apply to outbound links only?
I just found Drupal\rest\Access\CSRFAccessCheck, which does a similar job, although it is more restrictive in terms of when it applies.
Comment #10
andypostWould like to test in current masquerade 8.x-2.x module we need this to get rid of hook menu
Comment #11
Crell CreditAttribution: Crell commentedandypost: grammar parse error. Wha?
Comment #12
andypost@Crell, I mean that this hook_menu implementation needs token but the way Xano pointed looks nice.
So probably deal with injection in follow-ups?
Comment #13
andypostThis kind of access is needed for #1978948: Convert comment_approve() to a Controller
Actually there's no need to implement token-service here because we already have the same implementation and scrope is different
Comment #14
XanoFYI: the token API now is a service, so another name should probably be used for CSRF tokens to prevent confusion.
Comment #15
andypostSo how to make progress here? Maybe open new issue to bikeshed about name for service to replace
drupal_get_token()
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedi dont think we have to do this here..but if we really have to,
session_token
should be called, no need to bikeshed it, its pretty obviousComment #17
XanoI'll reroll the patch. #2036351: Convert CSRF tokens to a service was made to convert CSRF token generation and validation to a service, so let's not do that here.
Comment #18
XanoI changed the GET parameter name from
token
tocsrf
as to prevent confusion with the token API.Comment #20
dawehnerJust fixed a couple of c&p problems.
Comment #21
Crell CreditAttribution: Crell commentedThis still needs to have an outbound path processor that adds the token to the URL.
Comment #23
damiankloip CreditAttribution: damiankloip commentedOops.
Comment #24
damiankloip CreditAttribution: damiankloip commentedSorry, completely posted this on the wrong issue!! This was meant for #2042487: Token checking for Views enable/disable is missing. Sorry for un-needed noise.
Comment #25
andypostNow we have service for token #2036351: Convert CSRF tokens to a service so needs re-roll
Suppose all usages of token in controllers should be fixed in the issue as well
Comment #26
XanoComment #28
damiankloip CreditAttribution: damiankloip commentedThis can reference the new CSRF generator service too.
This can implement StaticAccessCheckInterface.
Access checkers are services, so we can inject this instead.
I guess we don;t want this anymore :)
This will have to change per comment abot about interface change.
Comment #29
andypostIs there ability to inject the service?
should be
Comment #30
Crell CreditAttribution: Crell commentedYes it should be injected, but no we don't set the request on it directly. The request should be implicit in the service as a dependency, or else provided directly in the validate call. Services should be treated as stateless. Vis, #28 is correct, #29 is not. :-)
This is entirely wrong. We only tag a route as needing CSRF. We should NOT put a static token on the route. That's a security hole.
Rather, we need an outbound path processor that will add a token to the path when it gets generated, and *that* is the value that gets checked by the access checker.
Comment #31
XanoNo. The csrf_token service uses the request for the account (which is set when instantiating the service), but the token needs to be passed every time it is validated.
This is not the token, but the value (like a salt) used to generate the actual token. See \Drupal::csrfToken()->get().
Could you rephrase this in layman's terms?
Comment #32
XanoComment #34
Crell CreditAttribution: Crell commentedPer #28, Do not put \Drupal::csrfToken() in a class. Ever. Instead, pass that service in via the constructor of the object using the appropriate services.yml file.
Comment #35
damiankloip CreditAttribution: damiankloip commentedDoing a bit of work on this.
Comment #36
damiankloip CreditAttribution: damiankloip commentedok, did a couple of things:
- Added $route parameter to the outboundPathProcessorInterface
- Added a check for query in the options in UrlGenerator::generateFromRoute() (same as generateFormPath() does)
There is possibly a big issue though, If we add a token to the query string for a link, the access will be checked to determine whether to show the link, but this will fail as in the access check, $request->attributes->get('csrf') will not return anything as it's not for that request.
Haven't worked on the tests yet. So will still be very broken.
Comment #38
XanoComment #40
damiankloip CreditAttribution: damiankloip commentedThis really has no chance of passing yet. There is a much bigger problem before this can can work as things like menu links will always be access false :/
Are you going to be in Prague? Maybe we could talk about it there?
Comment #41
XanoYes. I am at the extended sprints right now.
Comment #42
damiankloip CreditAttribution: damiankloip commentedI was speaking to klausi yesterday, and the discussion turned away from access checks to just having an event subscriber that does this for us instead. This was it is more 'baked in' to the routing system, kind of. But also gets around the fact that things like menu links use access checkers with a mocked request object etc...
Thoughts on this approach instead?
Comment #43
damiankloip CreditAttribution: damiankloip commentedMeh.
Comment #45
damiankloip CreditAttribution: damiankloip commentedLet's try this.
Comment #47
damiankloip CreditAttribution: damiankloip commentedOk, after more discussion, the access checker could be the better plan, as it doesn't run on every request (even though the outbound processor would anyway). I've added a RouteProcesserManager, OutboundRouteProcessorInterface and changed the previous implementation to use that. So this basically allows you to alter the route BEFORE it's compiled and the path is created etc...
This patch is still a work in progress, I will work some more on it later on.
Comment #48
damiankloip CreditAttribution: damiankloip commentedAfter talking to dawehner a bit, we were thinking something along the lines of this; So just for access checkers we can have a way of determining if the request is for reals of not.
Comment #49
damiankloip CreditAttribution: damiankloip commentedAnd we need to take into account the access mode there so we don't get unwanted access disasters happening.
Comment #50
damiankloip CreditAttribution: damiankloip commentedLogic was totally the wrong way round.
Comment #51
BerdirRe last interdiff: Is this still a problem with https://drupal.org/node/2063401?
Seems that when the default is ALL, this works as expected, and if the csrf requirement and another one and set the access mode to ANY, your fault?
Or is there some magic that applies that to routes automatically? I haven't looked at what all the other parts of the patch are doing...
Comment #53
damiankloip CreditAttribution: damiankloip commentedHere are some unit tests. Getting there...
Comment #55
damiankloip CreditAttribution: damiankloip commentedFix the urlGeneratorTest. Should this test use a mock instead and set expectations that processRoute() is called?
Comment #56
dawehnerI would say so, otherwise you should not call it unit test.
Can we please catch exceptions and remove the request variable in there and throw the exception again so it does not leak into 403/404 pages?
Can you tell me more?
I am wondering whether we should try to keep the route processors inside the path processor manager maybe and just add another interface. Having two separate systems feels kind of confusing to be honest.
What about adding @group Drupal and @group Routing or something like that? This test is so nice separated etc.
Comment #57
damiankloip CreditAttribution: damiankloip commentedThanks Daniel, how about something like this?
Comment #58
dawehnerOne important question though which is remaining is whether it is a good idea to have a separate routeProcessmanager ... you know removing features is harder than not adding them.
Nice!!
Comment #59
Crell CreditAttribution: Crell commentedBased on the comment, shouldn't the remove call happen before the throw AccessDeniedHttpException?
(This is exactly the use case that 'finally' is for in PHP 5.5... *sigh*)
Comment #60
damiankloip CreditAttribution: damiankloip commentedYes, this is a perfect use case for finally... :/
You're right, moved the removal to above the check for the $access variable.
Comment #61
damiankloip CreditAttribution: damiankloip commentedComment #62
dawehnerGood catch in #59. Bringing back to RTBC.
Comment #63
David_Rothstein CreditAttribution: David_Rothstein commentedAre there performance concerns with adding this? The possible performance impact of doing this for all generated URLs was a serious concern in #755584: Built-in support for csrf tokens in links and menu router, but when that issue was closed in favor of this one the discussion was not picked up here.
The current de facto standard is to use 'token' as the URL query parameter; I don't think this issue should change it to 'csrf'. People will see these tokens in the printed URL and 'token' is a lot less technical.
Plus there is code (for example, in the Overlay module, overlay_overlay_child_initialize()) that currently assumes 'token' as the de facto standard (actually one of the nice followups of this issue is that hopefully that code could be changed to not have to assume anything anymore, since it could check the route instead).
Comment #64
damiankloip CreditAttribution: damiankloip commentedYes, that is the plan. I'm hoping almost all usages of setting the token query string param and checking for a valid token can be removed in favour of a route requirement.
Shortcut is a good example to convert, looks like it's basically done in your patch, so thanks for that. Looks like it just needs a csrf requirement for the delete route too?
I'm happy to change this back to 'token' it was just csrf for so long I haven't thought about it :)
Hmm, performance...definitely worth looking into.
I can't see this overhead really being much in the scheme of things. One thing worth mentioning though is that this patch adds functionality to the routing system that is (IMO) very much needed. As well as the massive DX improvement for having CSRF protection on routes.
This use case shows it well, but currently you cannot alter a route before if it converted to a link/url etc... You can only use the outbound path processors. Which are basically a port of the old hook_url_outbound_alter() and is very limited. For example, without being able to alter the route before it is processed, you can not really ever add anything to the query string in an outbound processor - which is a major downfall.
Comment #65
damiankloip CreditAttribution: damiankloip commentedHere is a new patch; rolling in David's conversion in shortcut module and changing 'csrf' to 'token' for the query string.
Comment #66
Crell CreditAttribution: Crell commented"Token" was renamed because it was so easily confused with the Token API string mangling system. This is specifically a CSRF check, so let's keep it at that.
I like David's update. It shows exactly the simplification we're going for. :-)
Comment #67
David_Rothstein CreditAttribution: David_Rothstein commentedI wonder if "_csrf_token" in the route and "token" in the URL would be the best combination. But in any case, #65 already started the latter conversion... It looks good but I noticed a couple places it missed (just in code comments) so I made those fixes in the attached.
Nope, delete has a confirmation form. (I only converted it to a route in this patch too since it shares the link-generation code with the other one.)
That's a good point; it is opening up additional functionality besides the CSRF check.
I think the performance issue might come with the number of function calls involved in this (it really takes you several steps down the function stack to invoke this new processor), combined with the fact that there can easily be hundreds of URLs on a page. In addition, because core is shipping with an implementation of the system, that adds some overhead too (compared for example to hook_url_outbound_alter() which never had an implementation in core, so that made it more lightweight on many sites).
Anyway, my guess is @catch would want to see some benchmarks before committing this :) I can imagine some ways to improve performance if it becomes necessary (e.g. the CSRF check could be inlined in the generate() method, as well as a few other things).
Otherwise this is looking great.
Comment #68
damiankloip CreditAttribution: damiankloip commentedYes, I realised that after the comment. If you notice, the patch in #65 doesn't add that.
Seems like a bad idea, we don't hard code any other access checks or outbound processing in the urlGenerator. This is functionality we need either way so moving any stuff inline will not help with that. This is more a patch for route processing functionality, we just haven't needed until until this use case. We certainly can't ship without being able to do this though I don't think.
Comment #69
Crell CreditAttribution: Crell commentedI just realized something else worrisome... We were planning to cache all generated URLs for performance: #1965074: Add cache wrapper to the UrlGenerator
What does this do to cacheability? (And is it any worse than CSRF tokens already?)
Comment #70
damiankloip CreditAttribution: damiankloip commentedThat's a good question indeed. I would say we are no worse off than we are now with tokens? Well, maybe better, as we have a better system for doing it, plus we DO need to be able to alter outbound routes :)
Comment #71
dawehnerIs there any reason for the really high priority here? Note this ensures that it comes first, ... I don't see a point for that.
Let's just @inheritdoc it.
new line missing.
I guess we don't need this change anymore.
Comment #72
damiankloip CreditAttribution: damiankloip commentedthanks Daniel.
Comment #73
dawehnerTogether with the potential cleanups in follow ups this seems to is ready to fly.
Comment #74
ParisLiakos CreditAttribution: ParisLiakos commentedshould we also do those here?
Comment #75
damiankloip CreditAttribution: damiankloip commentedI'll open a follow up for conversions if that's ok? This patch is getting big enough :)
Comment #76
ParisLiakos CreditAttribution: ParisLiakos commentedsure. i opened #2109303: Convert CSRF checks in controllers to the routing system
they are just 3 of them, so one issue should be enough
Comment #77
Xano#72: 1798296-72.patch queued for re-testing.
Comment #78
catchIt'd be good to see what impact the extra processing has with some profiling before this goes in. Haven't reviewed the patch yet.
Comment #79
damiankloip CreditAttribution: damiankloip commentedSee xhprof output from comparing before/after patch, to see the effect of the route processor, this is just using the generateFromRoute() method, as this is the only one that will actually use the route processor:
Comment #80
damiankloip CreditAttribution: damiankloip commentedWe have talked about adding the path and route processors as a single processor, this should make things much better. I would prefer to do that as a critical follow up, how does that sound? If not, I can reroll and do it in this patch. I just don't want this patch to get too large.. and merging the processors will no doubt spark a whole discussion of its own.
Comment #81
damiankloip CreditAttribution: damiankloip commentedHere is a patch with the outbound route processor merged into the path processor instead.
Comment #82
damiankloip CreditAttribution: damiankloip commentedOops, this patch please. Ignore the xhprof hunk I accidentally included in that last one..
Comment #83
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #84
jibranBack to @catch after #79.
Comment #85
katbailey CreditAttribution: katbailey commentedCan we call this addOutboundRouteProcessor? I know it's a bit long but this method is not adding a route, it's adding a processor. Same with getOutboundRoute and the sortedOutboundRoute property. It sounds like they're acting on the same thing as processOutboundRoute but they aren't.
It seemed odd to see this shoved into the PathProcessorManager, but if it really makes a difference to performance then fair enough. I wonder should we change the name though? Not here necessarily... but change it to something like RoutingProcessorManager. Seems more generic.
Anyway, these are minor points so leaving it RTBC.
Comment #86
katbailey CreditAttribution: katbailey commentedwat.
Comment #87
msonnabaum CreditAttribution: msonnabaum commentedTotally agreed with #85.
It struck me as very odd to see PathProcessorManager handling both. The design in #72 looks much better to me, and if performance was the reason we changed that, lets please change it back.
We need to stop considering the performance impact of method calls and calling "new". It's never worth compromising the design.
Comment #88
damiankloip CreditAttribution: damiankloip commentedAfter talking to katbailey and msonnabaum about this, having two managers, and therefore better interfaces etc.. is a better win. Merging the managers gives us nothing from a performance point of view.
This is a reroll of the patch from #72.
Comment #89
msonnabaum CreditAttribution: msonnabaum commented#88 looks much better to me.
Comment #90
katbailey CreditAttribution: katbailey commentedThis looks great to me and the tests are lovely :)
Comment #90.0
katbailey CreditAttribution: katbailey commentedUpdate for related developments.
Comment #91
David_Rothstein CreditAttribution: David_Rothstein commentedThe patch no longer applied, so here's a reroll. I couldn't generate a good interdiff due to the conflicts, but the attached diff-of-a-diff shows the changes (which look pretty trivial).
The performance numbers in #79 look really scary (~50 millisecond performance hit for a page with 100 URLs on it!?). However, I couldn't reproduce anything that bad. Is there some caching that causes the first call to take a lot longer? What I tested was 10,000 consecutive calls to
Drupal::url('user.logout')
, which came out to an average of 110 microseconds per Drupal::url() call before the patch and 120 microseconds after. So a ~10% hit rather than a ~25% hit, but more important, the baseline number was low enough that the total effect would be very small (only a ~1 millisecond performance hit for a page with 100 URLs on it). Perhaps I did something wrong too though. We should probably get to the bottom of it.What was more "interesting" was that I ran the equivalent code in Drupal 7, and the average came out around 22 microseconds per url() call. Which means that with or without the patch, URL generation is 5 times faster in Drupal 7 than Drupal 8?! Wow.
Comment #92
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, per #67 I still think we should do something to standardize the terminology and improve the developer experience ("_csrf" on the route is not precise, and doesn't match "token" in the URL in any way).
I would prefer "_csfr_token" or even "_csrf_token_value" on the route, to be more explicit about what you're actually supposed to provide there.
Comment #93
damiankloip CreditAttribution: damiankloip commentedYou are surprised that D8 is slower than D7?! ;) Things like this are many times slower at the moment I'm afraid :/ I guess if you went back to D6, it would be faster than D7 too :) The bottom line is that we have to be able to alter outbound routes in this way (Sorry, starting to sound like a broken record). The routing system in general need a performance boost, so this getting in I think is an essential part of that - so we can see where we are. What do you think?
What did you use to benchmark? Here is an xhprof summary of what you described above (calling \Drupal::url('user.logout') 100 times):
I have changed this patch to use '_csrf_token' as the requirement instead, I think '_csrf_token_value' is a bit too far IMO.
That above patch should (I hope anyway) certainly fail, as the unit tests are not using the new access() parameter signature. The reroll contains a fix for that too.
Hopefully you're next reply won't take a month, like the frequency between the last one ;) ha. This does make core dev slightly more painful tbh.
Comment #94
catchActually no. I did quite a bit of work on url generation (path alias caching and various other things) to make it faster in Drupal 7.
Related to the caching of the CSRF tokens, this patch doesn't make it worse, it might potentially make it better if it's possible to replace the token handling with something similar to https://drupal.org/project/cacheable_csrf. For that I'd need to be able to swap out the csrf token generator when creating links to one that adds a placeholder, then remove the CSRF check that core does and insert a different one in the routing system. Not sure if that approach is at all viable for core though.
It's not great having both this patch and #2019123: Use the same canonical URI paths as for HTML routes adding overhead to inbound/outbound link generation respectively. We may need to bump #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees and #1965074: Add cache wrapper to the UrlGenerator to critical - although those only address caching not raw performance at all.
Comment #95
damiankloip CreditAttribution: damiankloip commentedSorry, I stand corrected on the D6 > D7 url handling :)
They are all services, so you could just swap them out, you would switch the outbound csrf route processor, as well as the access checker. That would be job done? unless I missed something.
As mentioned up somewhere, we basically need to be able to alter the outbound route in this way, so I think it would be good to get this in, then atleast we know where we stand and what we need to try and optimise more. Thoughts on that?
Comment #96
David_Rothstein CreditAttribution: David_Rothstein commented#93 looks good to me after fixing the remaining instances of _csrf (they were in documentation) - see attached. Thanks for catching the remaining access() instances.
The numbers in #93 look closer to mine although still fairly different (~6 millisecond performance hit per 100 URL calls, rather than ~1 millisecond). Some of that could be computer speed, although percentage-wise they're also quite different (20% vs 10%). For what it's worth, I did my testing like this:
And then compared the numbers.
I'm putting this to RTBC on the theory that a few milliseconds isn't too terrible of a hit for a security boost (especially compared to the overall situation), and that @catch is going to look more at this with a performance eye anyway. Beyond that it seems ready to go.
Comment #97
David_Rothstein CreditAttribution: David_Rothstein commentedHm, that's weird... it already was back to RTBC. But I don't see which comment changed it to that.
Comment #98
dawehnerPlease try to do the following:
Comment #99
David_Rothstein CreditAttribution: David_Rothstein commentedTried that and it sped things up a tiny bit (though that's actually the wrong direction in terms of reconciling with #93).
Results: ~100 microseconds per generate() call before the patch, ~113 microseconds after
(compared to 110 and 120 during previous test with Drupal::url())
Overall makes sense to me that it's similar (since Drupal::url() and $url_generator->generate() are both wrappers around basically the same thing).
Comment #100
David_Rothstein CreditAttribution: David_Rothstein commentedOh also, all my tests were done on a fresh install using the Minimal profile, if it matters.
Comment #101
catchOK I'm still not happy about the performance hit here, but since this is going to end up contributing to making the CSRF tokens (and hence the HTML for those links) cacheable that seems a reasonable trade-off - I'm about to implement that for a 7.x site and there's no clean way to do it.
Committed/pushed to 8.x. Will need a change notice.
Comment #102
damiankloip CreditAttribution: damiankloip commentedCreated a change notice for the csrf token usage, shall we create a new notice or add to existing path subscriber ones for the outbound route subscriber stuff?
https://drupal.org/node/2133117
Comment #103
jibranI think we should add example of dynamic value token as well.
Comment #104
andypostBetter to fix the typo
Comment #105
webchickCommitted and pushed to 8.x. Thanks!
I think "needs review" is still appropriate for the change notice..?
Comment #106
xjmComment #107
damiankloip CreditAttribution: damiankloip commentedI created one of the CNs, but would like #2133439: Dynamically create token value string based on route path to get committed first, as that will change it slightly anyway.
Comment #108
webchickLet's make sure we also make sure to document when to use this new property and why in the routing system docs
Comment #109
damiankloip CreditAttribution: damiankloip commentedAdded something for that to the bottom of https://drupal.org/node/2122195, check it out.
Comment #110
andyposthttps://drupal.org/node/2133117 should be updated too
Comment #111
damiankloip CreditAttribution: damiankloip commentedUpdated that.
Comment #112
Crell CreditAttribution: Crell commentedI added a note to the docs about the token only working when using the generator. Otherwise I think we're done. Thanks all!
Comment #113
damiankloip CreditAttribution: damiankloip commentedOh yeah, that.. Good call!