If cookie/session-based authentication is used write operations should require an anti-CSRF token (in e.g. request header) to prevent certain combinations of browser plugins and HTTP redirects that can be used to trick the user’s browser into making cross-domain requests circumventing the protections in place.
Comment | File | Size | Author |
---|---|---|---|
#9 | csrf-token-1891052-9.patch | 15.92 KB | klausi |
#8 | csrf-token-1891052-8.patch | 17.41 KB | klausi |
#8 | csrf-token-1891052-8-interdiff.txt | 7.17 KB | klausi |
#5 | csrf-token-1891052-5.patch | 13.38 KB | klausi |
#5 | csrf-token-1891052-5-interdiff.txt | 7.75 KB | klausi |
Comments
Comment #1
klausiSee also SA-CONTRIB-2013-003 for a similar vulnerability in RESTWS: http://drupal.org/node/1890222
We will need to do something similar: http://drupalcode.org/project/restws.git/commitdiff/dae7e2e
Comment #2
klausiTagging.
Comment #3
klausiPreliminary patch that tries to replicate the fixes from RESTWS in D7. Not finished yet, so this will fail.
Comment #5
klausiFixed the simpletests. I also found the cURL quirk with ->drupalLogin(), that simplifies the simpletests a bit.
Question: do we want to check the CSRF token in the usual controller (as done in this patch) or do we want to implement a routing system access handler?
Comment #6
Crell CreditAttribution: Crell commentedNote to self for later: drupal_get_token() needs to turn into an injectable service.
I don't believe we're using dots in route machine names.
I would say the token checking should be split out to an access checker object. That makes it flexible to be used beyond just rest module's own unified controller. We'll want to do something very similar for #1798296: Integrate CSRF link token directly into routing system
Comment #7
klausiYes we do, REST module uses dots for all of its routes. Route naming conventions should be discussed in another issue.
If we move the check to a routing AccessCheckInterface object how do we implement the applies() method? We could use an _access_rest_csrf requirement on the route. I'm only worried that resource plugins might forget to add that to their routes, so I guess it would be a good idea if the rest module route collector adds it.
Comment #8
klausiWhile developing this I hit a routing system access bug: #1896556: Routing AccessManager does not evaluate access checks correctly.
Here is a patch that does the CSRF validation in a routing system access checker. Includes code from #1896556: Routing AccessManager does not evaluate access checks correctly.
Comment #9
klausiRerolled, now that the AccessManager issue was committed. No other changes.
Comment #11
klausi#9: csrf-token-1891052-9.patch queued for re-testing.
Comment #12
Crell CreditAttribution: Crell commentedI think we're good here. Mr. Testboto can disagree if he wants.
Comment #13
webchickCommitted and pushed to 8.x. Thanks!