Originally identified in #66264-38: Remove CSRF vulnerability from comment module

+++ modules/comment/comment.module	12 Mar 2010 07:14:21 -0000
@@ -992,6 +992,7 @@ function comment_links($comment, $node) 
           'title' => t('approve'),
           'href' => "comment/$comment->cid/approve",
           'html' => TRUE,
+          'query' => array('token' => drupal_get_token("comment/$comment->cid/approve")),

+++ modules/comment/comment.pages.inc	12 Mar 2010 07:14:21 -0000
@@ -107,6 +107,9 @@ function comment_reply($node, $pid = NUL
 function comment_approve($cid) {
+  if (! isset($_GET['token']) || !drupal_valid_token($_GET['token'], "comment/$cid/approve")) {
+    return MENU_ACCESS_DENIED;
+  }

Ideally, we'd do the following:

1) Enhance l() + url() to support $options['token'] = TRUE, i.e. automatically take the $href as value for drupal_get_token().

2) Enhance hook_menu(), so that an item can specify 'requires token' => TRUE, which makes http://api.drupal.org/api/function/menu_execute_active_handler/7 resp. http://api.drupal.org/api/function/menu_get_item/7 take the registered path (with dynamic arguments replaced from map) and automatically checks the token against that.

Comments

sun’s picture

#144538-26: User logout is vulnerable to CSRF already contains a patch that implements the menu router support to some extent.

I'd prefer to use a query parameter for the token though, as that would make usage/conversion a bit easier, I think.

andypost’s picture

subscribe

grendzy’s picture

We'd have to do some benchmarks, but I think it might be possible to even eliminate the extra parameter to url(), and have it automatically detect when the token is required. (Basically, url() would look up the router path, and check for 'requires token').

coltrane’s picture

For new readers, implementing this would hopefully help decrease the number of cross site request forgery vulnerabilities that are created by developers who use links for actions like deleting content or voting something up.

While this would still have to be an opt-in fix, it's just a bit easier than writing the token set and check yourself, so hopefully adoption would be good.

David_Rothstein’s picture

Subscribing. Does this really still have a shot at Drupal 7? I guess as a security improvement it might, but otherwise probably Drupal 8.

This is going to be a great feature though - people have a bad tendency to throw up a confirm form in front of everything even when there is no good reason to, and this will make it a lot easier for them to safely skip that. A nice long-term dream for Drupal 8 would be to see the confirm_form() function used a lot less and a new undo_link() function (based on this, among other things) used a lot more :)

coltrane’s picture

I agree, a query parameter would be better than part of the path. It has the benefit of fitting easily into the definition of url() by doing something like:

if ($options['token']) {
  $options['query']['token'] = drupal_get_token($path);
}

and using query parameter building.

But, wouldn't we need to alter the menu_router table to store if token was set on the menu definition so menu_get_item() knows to check the validity?

meba’s picture

This would be very nice although it's probably too late for D7...

rfay’s picture

+1. I'd sure like to see this in D7.

matt2000’s picture

+1

coltrane’s picture

I began work on a patch, and it's small, but got hung up on this question:

How does menu_get_item() know to call drupal_valid_token() when a request comes in? Should the menu_router table be amended to store this?

Once we get some working code we can create a variant for grendzy's idea of removing the url() option and benchmark the two.

grendzy’s picture

I was also tinkering with this for a bit... and yes I added a column to menu_router.

sun’s picture

Version: 7.x-dev » 8.x-dev
Assigned: sun » Unassigned

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

rfay’s picture

IMO this is in fact a feature and is probably too late for D7.

Dave Reid’s picture

Category: task » feature
joachim’s picture

subscribe

Lars Toomre’s picture

I just stumbled across this issue and hope this can be resolved for D8 with a possible backport to D7.

Dave Reid’s picture

FYI I've started working on a sandbox to support this in D7 (and possibly backport to D6): http://drupal.org/sandbox/davereid/1332490

greggles’s picture

Title: Built-in support for security tokens in links and menu router » Built-in support for csrf tokens in links and menu router

Better searchability: they are csrf tokens, not security.

sun’s picture

btw, recently I wondered why these tokens are specific per operation, if all we need is them to be specific per user/session. Malicious JS is a much larger attack vector these days, and these tokens do not protect at all against that. We should consider to make them as simple as possible, and at the same time, attempt to use them in many more places. IIRC, @Heine was OK with that proposal.

greggles’s picture

@sun are you proposing that we make them less unique (not unique to the action) and only unique to the user/site?

I couldn't come up with an explicit reason they need to be unique to the user, site, and action. The owasp csrf prevention page says a few interesting things. First, they say you could create a csrf token *per session* and use the same one over and over. They then go on to describe several ways - other than XSS - where someone could abuse a token if the token is the same across an entire session. I think we should keep with our current standard of making it unique to user, site, and action.

grendzy’s picture

woah, I'm having flashbacks to index.php?PHPSESSID=5bf28931.... :-)

Anyway, I think there's very little added complexity by keeping the path in the hmac - especially if it's completely abstracted through hook_menu and url().

I guess the benefit is preventing a replay attack when permission to perform a particular action depends on more than just the user's identity... but I'm struggling to think of a good example.

catch’s picture

We'd have to do some benchmarks, but I think it might be possible to even eliminate the extra parameter to url(), and have it automatically detect when the token is required. (Basically, url() would look up the router path, and check for 'requires token').

A menu_get_item() inside url() would be roughly the same as what contextual_links does, see #607244: Decrease performance impact of contextual links for the performance impact of that except that was with a much smaller subset of links - it would be a very significant performance penalty.

tstoeckler’s picture

Status: Active » Needs work

I like the approach. DX++.
Adding a preg_match() to l() requires some benchmarks, though I think.

xjm’s picture

Status: Needs work » Needs review

Sending to the bot.

xjm’s picture

xpost

Crell’s picture

Note that url() and menu_get_item() will both be changing radically in WSCCI; url() will likely be replaced completely by a UrlGenerator object. Given that you'll always be generating links to router items, not to random strings, I think it's completely reasonable to automate token logic in that process.

I don't know if I will do that in the initial router patch but I'll keep that question in mind.

rfay’s picture

Status: Needs review » Needs work

#23 has an endless loop or something in the tests. Current status is "Running tests, 418,232 assertion(s)." after a *long* time.

matt2000’s picture

I don't think there are any loops at all in the patch itself, but I didn't test it locally. I know it's a long way from being ready. I just wanted to express the idea. AFAIC, the test process can be killed.

rfay’s picture

Sorry, deleted #23 to keep it from using up a testbot all the time. We probably need a better way to do this. Happened the other day on another issue.

matt2000’s picture

#23 used to say:

I also think using menu_get_item is unnecessary overkill, since the
proportion of paths needing tokens will probably be relatively small. I
count less than 10 in core:

http://api.drupal.org/api/drupal/core%21includes%21common.inc/function/c...
[1]

Can we just keep a separate list of paths needing tokens in $conf or a
dedicated semi-permanent cache?

To get us started, here's a patch based largely off Dave Reid's sandbox
that uses variable_set.

(Patch to be reposted after further review.)

greggles’s picture

@Crell - What's the status on the UrlGenerator object work? Is it done? Should we still hold off waiting for it?

Crell’s picture

The generator may or may not happen as part of #1606794: Implement new routing system, but if not it's the very next thing on my list. At this point I do plan to try and just bake this functionality in, where it belongs. Reviews of that patch (still very much in progress) or help pushing it forward (contact me) are very much welcome.

If it can be done easily here, as a prototype for the new system, that's fine. But since our links are right now all generated as URL strings, not machine names, I'm not sure how feasible that is.

Crell’s picture

Status: Needs work » Closed (duplicate)
greggles’s picture

(grumble about closing an issue with a bunch of people following it, without justification, when the new issue has no discenarable improvement compared to just updating the issue summary of the original issue that is 2 years older).