Posted by frega on January 17, 2013 at 10:36am
12 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | rest.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Security improvements, Stalking Crell, WSCCI |
Issue Summary
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.
Comments
#1
See 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
#2
Tagging.
#3
Preliminary patch that tries to replicate the fixes from RESTWS in D7. Not finished yet, so this will fail.
#4
The last submitted patch, csrf-token-1891052-3.patch, failed testing.
#5
Fixed 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?
#6
+++ b/core/modules/rest/lib/Drupal/rest/RequestHandler.php@@ -66,4 +71,43 @@ public function handle(Request $request, $id = NULL) {
+ public function csrfToken() {
+ return new Response(drupal_get_token('rest'), 200, array('Content-Type' => 'text/plain'));
Note to self for later: drupal_get_token() needs to turn into an injectable service.
+++ b/core/modules/rest/rest.routing.yml@@ -0,0 +1,6 @@
+rest.csrftoken:
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
#7
Yes 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.
#8
While 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.
#9
Rerolled, now that the AccessManager issue was committed. No other changes.
#10
The last submitted patch, csrf-token-1891052-9.patch, failed testing.
#11
#9: csrf-token-1891052-9.patch queued for re-testing.
#12
I think we're good here. Mr. Testboto can disagree if he wants.
#13
Committed and pushed to 8.x. Thanks!
#14
Automatically closed -- issue fixed for 2 weeks with no activity.