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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi’s picture

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

klausi’s picture

Issue tags: +Security improvements, +WSCCI

Tagging.

klausi’s picture

Status: Active » Needs review
FileSize
7.64 KB

Preliminary patch that tries to replicate the fixes from RESTWS in D7. Not finished yet, so this will fail.

Status: Needs review » Needs work

The last submitted patch, csrf-token-1891052-3.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
Issue tags: +Stalking Crell
FileSize
7.75 KB
13.38 KB

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?

Crell’s picture

Status: Needs review » Needs work
+++ 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

klausi’s picture

I don't believe we're using dots in route machine names.

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.

klausi’s picture

Status: Needs work » Needs review
FileSize
7.17 KB
17.41 KB

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.

klausi’s picture

FileSize
15.92 KB

Rerolled, now that the AccessManager issue was committed. No other changes.

Status: Needs review » Needs work
Issue tags: -Security improvements, -WSCCI, -Stalking Crell

The last submitted patch, csrf-token-1891052-9.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
Issue tags: +Security improvements, +WSCCI, +Stalking Crell

#9: csrf-token-1891052-9.patch queued for re-testing.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I think we're good here. Mr. Testboto can disagree if he wants.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.