The ajax flag link uses a token to protect against CSRF attacks.

Currently the token only uses the entity ID. I've discussed this with the security team, whose response is that this is fine, but could do to be hardened:

* It would be great to move to something beyond md5 (since that can lead to false positives about insufficient cryptographic strength in automated scans)
* It would be great to add more data into the token beyond just entity ID

For point two, we can certainly add the flag name, action, and entity type into the token.

For point one, I could use some advice :)

CommentFileSizeAuthor
#11 flag-harden-token-in-flag-link-1751004-11.patch2.16 KBgokulnk
#8 flag-harden-token-in-flag-link-1751004-8.patch2.16 KBgokulnk
#5 flag-harden-token-in-flag-link-1751004-5.patch2.2 KBalexweber

Comments

Scyther’s picture

Scyther commented

Maybe it can use the same function as the user password is using, but maybe it's to complex? See http://api.drupal.org/api/drupal/includes!password.inc/7

joachim’s picture

joachim commented

Do you mean http://api.drupal.org/api/drupal/includes!password.inc/function/_passwor... ?

I am not really sure what to make of that, or which bits of that to use here.

Also, one thing to bear in mind is that password encryption is a relatively rare event, as a response to a form submission. It can afford to be slow. Generating a flag link may have to be done many times on a single page load.

Scyther’s picture

Scyther commented

Long time ago, but I think I meant that function :/

alexweber’s picture

alexweber commented

@joachim how do you want to combine entity id, flag name and action? Simple concatenation?

alexweber’s picture

alexweber commented
Status: Active » Needs review
StatusFileSize
new flag-harden-token-in-flag-link-1751004-5.patch2.2 KB

Attached is a patch which does the following:

  • Adds $flag->name and $action as extra required parameters for both flag_get_token() and flag_check_token()
  • Creates a new token string performing a simple string concatenation of: $entity_id . $flag_name . $action
  • Uses _password_crypt() with sha256 algorithm and a 7-iteration salt (Drupal minimum). We could alternatively use user_hash_password() instead but it uses sha512 and 15 iterations which IMO is a bit excessive here
  • This new hashing algorithm obviously only affects the authenticated user check but anonymous still get the benefit of the new concatenated token string

I realize there might be adjustments given I had to make a couple decisions in order to get a patch. Either way, it should serve as a good starting point for this!

joachim’s picture

joachim commented
Status: Needs review » Needs work

_password_crypt() looks like a pretty heavy function to me, and generating a token is something that has to be done for every single flag JS link. On a page like /node, or a view with flag links, that represents a lot of links. I'm concerned that it could significantly increase page load times.

I think we need input from the security team on what would be better to use than md5, but without sacrificing speed.

alexweber’s picture

alexweber commented

Ok, sounds good! Yeah, I realized that _password_crypy() might be a bit of overkill... maybe just call hash('sha256') instead? As of PHP 5.2 we can always be sure that sha256 is supported...

Can you get someone on the security team to get eyes on this?

gokulnk’s picture

gokulnk commented
Issue summary: View changes
StatusFileSize
new flag-harden-token-in-flag-link-1751004-8.patch2.16 KB

I think this is a good feature to have as it improves the security. I read a few blogs and based on that I feel hash('sha256') is good enough for generating tokens.

I have re-rolled the patch and changes the hashing function as discussed in previous comments.

joachim’s picture

joachim commented
Status: Needs work » Needs review

Let's see if the testbot likes it.

Scyther’s picture

Scyther commented

In patch 8

  $token_string = $entity_id . $flag_name . $action;
  return ($GLOBALS['user']->uid) ? drupal_get_token($token_string) : hash('sha256', drupal_get_private_key() . $token_string);

  // Looks better ?

  $token_string = drupal_get_private_key() . $entity_id . $flag_name . $action;
  return ($GLOBALS['user']->uid) ? drupal_get_token($token_string) : hash('sha256', $token_string);
gokulnk’s picture

gokulnk commented
StatusFileSize
new flag-harden-token-in-flag-link-1751004-11.patch2.16 KB

Scyther,

Agree with you. Moving the drupal_get_private_key() to token_string makes it more readable.

Updated the patch.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work 
  1. +++ b/flag.module
@@ -2368,17 +2368,18 @@ function flag_get_link_types() {
-  return ($GLOBALS['user']->uid) ? drupal_get_token($entity_id) : md5(drupal_get_private_key() . $entity_id);
+  $token_string = drupal_get_private_key() . $entity_id . $flag_name . $action;
+  return ($GLOBALS['user']->uid) ? drupal_get_token($token_string) : hash('sha256', $token_string);

    why do you do your own logic and not just use drupal_get_token()? The comment says to work for anon users, but drupal_get_token() is able to work with anonymous users? drupal_get_token() does all the private key magic for you. 

  2. +++ b/flag.module
@@ -2368,17 +2368,18 @@ function flag_get_link_types() {
+function flag_check_token($token, $entity_id, $flag_name, $action) {
+  return flag_get_token($entity_id, $flag_name, $action) == $token;

    Always use "===" when comparing security related stuff. drupal_valid_token() could be used here if you just rely on drupal_get_token().

joachim’s picture

joachim commented 
-  return ($GLOBALS['user']->uid) ? drupal_get_token($entity_id) : md5(drupal_get_private_key() . $entity_id);

The code is already doing something separate for anon users, so the patch isn't making that worse AFAICT.

The comment there says:

  // Anonymous users get a less secure token, since it must be the same for all
  // anonymous users on the entire site to work with page caching.

So it looks like there's a reason -- not that I understand it completely.

With some git blame & issue queue archaeology we could dig into it more, but it's going to take a bit of work, as a simple git blame of those lines hits the really annoying dump commit in our history:

commit c36124e0e57578a25e775725ff851a261d0e1f6f
Author: Nathan Haug <quicksketch@35821.no-reply.drupal.org>
Date:   Sun May 9 00:07:47 2010 +0000

    Re-adding 6--2 branch to head for Drupal 7 branch.
ivnish’s picture

ivnish
he/him
commented
Status: Needs work » Closed (outdated)

Closed as outdated because Drupal 7 is EOL