Posted by sun on March 28, 2010 at 8:06pm
Jump to:
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
| Issue tags: | Security improvements |
Issue Summary
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
#1
#144538-26: Image with /logout URL as source 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.
#2
subscribe
#3
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').
#4
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.
#5
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 :)
#6
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:<?phpif ($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?#7
This would be very nice although it's probably too late for D7...
#8
+1. I'd sure like to see this in D7.
#9
+1
#10
I began work on a patch, and it's small, but got hung up on this question:
How does
menu_get_item()know to calldrupal_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.
#11
I was also tinkering with this for a bit... and yes I added a column to menu_router.
#12
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).
#13
IMO this is in fact a feature and is probably too late for D7.
#14
#15
subscribe
#16
I just stumbled across this issue and hope this can be resolved for D8 with a possible backport to D7.
#17
FYI I've started working on a sandbox to support this in D7 (and possibly backport to D6): http://drupal.org/sandbox/davereid/1332490
#18
Better searchability: they are csrf tokens, not security.
#19
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.
#20
@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.
#21
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.