Remove CSRF vulnerability from comment module

This feature request is still valid in Drupal 6 beta2. To approve an individual comment you still have to click the "Edit" link, then click on the "Administration" fieldset title to open it, click the "Published" radio button, then scroll to the bottom of the page and click the "Submit" button. Phew!

So a one-click approval link would really be handy, if it's possible.

Redirecting to the comment.module issue queue. I guess this is for 7.x-dev because of the code freeze?

After using my Drupal blog I have noticed that this is a feature that would be very useful. I searched and found this issue so I have attached a patch to it rather than starting a new issue.

This is my first patch related to the comment module. It may be a bit rough around the edges.

Advised by chx to use comment_load.

chx reviewed in IRC.

This includes a modified test to take this into account.

Excellent patch. With tests and everything. :-)

Great stuff!

+  $items['comment/approve'] = array(
+    'title' => 'Approve to comment',

sounds strange, like granting a permission to a user. Isn't it 'Approve a comment' ?

Yes, I think I was trying to write "the."

still applies with offset.

Committed to CVS HEAD. Marking fixed ... thanks.

Sorry, this is a CSRF vulnerability. Either we need a confirm form or some sort of cron_key-like mechanism to allow this sort of one-click convenience to work without opening a security hole.

Not following you.

Need to be logged in with correct privileges:

$items['comment/approve'] = array(
    'title' => 'Approve a comment',
    'page callback' => 'comment_approve',
    'page arguments' => array(2),
    'access arguments' => array('administer comments'),
    'type' => MENU_CALLBACK,
  );

And link won't even be displayed unless you can administer comments anyway as they are considered unpublished.

If anything now that I look at it should the url be 'comment/approve/%' ?

#12 - @see http://en.wikipedia.org/wiki/CSRF please. No actions should ever be taken through an insecure GET request.

Believe I have it working, but I have to leave so post later.

This should do it.

Updated test so that it take the confirmation form into account and it passes. Had 1 fail (as it should) before.

Why not use %comment instead of just %? Then you get comment_load() called automatically.

Sure, never used it before. Was just adding % so that it would be consistent and wouldn't allow comment/approve to be called.

Patch includes minor change.

Still applies, ping.

The patch looks good, except that these lines are completely unneeded, because confirm_form() has already set $form['#redirect']:

+  $comment = comment_load($form_state['values']['cid']);
+  $form_state['redirect'] = "node/$comment->nid";

Checked into, but that doesn't seem to be the case.

* @param $path
*   The page to go to if the user denies the action.
*   Can be either a drupal path, or an array with the keys 'path', 'query', 'fragment'.

And it never sets #redirect, as it also doesn't work if I remove the lines in question.

Patch still applies and ran comment test which returned all passes.

Re-rolled, DB TNG was the cause of failed to apply.

"You can unpublish comment" should be "You can unpublish this comment". Otherwise this looks good - except I'd rather see us use tokenised links to avoid CRSF for one click operations rather than confirm forms. That probably needs centralising though.

Addressed #24.

Sending for re-test.

Why:

+  else {
+    drupal_set_message(t('The comment you are approving does not exist.'), 'error');
+    drupal_goto();
+  }

and not a 404?

Otherwise looks good.

@webchick rediscovered the CSRF vulnerability in #606608: Use proper menu router paths for comment/*. We should probably consolidate efforts here, but note there is a more up-to-date patch at http://drupal.org/node/606608#comment-2240294

She also pointed out that the access callback for this link is not correct (you should not be able to approve a comment that is already approved).

I also agree with @catch - it seems like the entire reason this was introduced was to have a convenient one-click approve link, so adding a confirmation form hurts that experience. We don't really need a confirmation form, just a token. Seems like we could get away without centralizing it - this isn't that common of link that people will be generating outside of a few special modules, and it's not hard for them to use the same token generation code as the comment module would use (and they'd learn to do that pretty quickly, since if they didn't, their links wouldn't work :)

Tagging this as a known security issue.

For D7 it would be nice to have some common token-adding code in the API

a number of contrib modules (e.g. fivestar) have ahd to repeatedly implement this for D6.

Here is a copy of the patch that David referenced, so we can keep everything in one place, while we refocus that issue for the performance concerns.

uses drupal_valid_token, instead of a cumbersome confirmation dialog. Also adds a test assertion to verify the XSRF is fixed.

This looks RTBC except for the extra space between ! and isset().

This patch also implies that comment_approve() is not an API function but that it assumes to be called through the browser (for lack of a better word). We should verify that that is a sound assumption. I haven't looked at the surrounding code.

I don't think we should create vital API functions that cannot be used as an API. It shouldn't be too difficult to separate the two into a web wrapper and base API.

+++ 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() so that the value for drupal_get_token() can be specified in $options. EDIT: Actually, it's just $options['token'] = TRUE, i.e. automatically take the $href.

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.

143 critical left. Go review some!

Ok, here's a new patch that fixed the whitespace, and also tests both sides of the token validation code.

Regarding the API function, comment_approve is documented as a menu callback only, is not called elsewhere, and is in a pages.inc file so it's not even callable from other modules.

The API function is comment_save(), which I think is adequate. If you want to add an additional comment API function, that's a separate issue from fixing the security vulnerability IMO.

sun, I was thinking the same thing concerning the url() enhancement. Do you think there's still time to do that in d7? Should it be in this patch, or a different issue? (I would also really like drupal_get_token to use HMAC).

Let's just fix the sec issue here, issue has been around more than a year, no reason to delay it now.

Additional hardening sounds great for a new issue though.

Patch looks RTBC pending test bot.

Patch looks good, and works as intended.

And for anyone who is wondering, no, comment_approve() was not anything like an API function before this patch either. Here is the part of the (current) function that you can't see from the patch file:

    drupal_set_message(t('Comment approved.'));
    drupal_goto('node/' . $comment->nid);
  }
  return MENU_NOT_FOUND;

That's three separate, independent ways in which this is not an API function to begin with :)

So we aren't making it any worse with this patch.

I also created a followup issue for the much less serious bug with the access callback of this function reported in #29:

#748996: Visiting the comment approval link for a published comment causes 403

Patch looks good. Thanks for the hardening tests, too.

Committed to HEAD.

Do we have an issue already for the suggestion in #38, more automated security token handling?

@sun: I don't remember seeing one recently.

@sun, @catch - Ben proposed that as his focus for the Drupalcon Core Sprint - http://www.benjeavons.com/dcsf-core-summit-presentation

coolio, also created an issue: #755584: Built-in support for csrf tokens in links and menu router

I'm looking at using the Comment Moderation module to get the functionality that was originally discussed in this issue for Drupal 6.x. It's a little overkill, but might work for what we need. Since I'm not a programmer, could someone that understands the CSRF problem that was discussed here take a look at the Comment Moderation module to see if it suffers from the same problem? I'm going to cross post this thread to the module maintainer (dag-) as an issue on his module.