Skip to content

Commit

Permalink
Harden HMAC verification. props duck_. [28053] for 3.8.
Browse files Browse the repository at this point in the history 
Built from https://develop.svn.wordpress.org/branches/3.8@28054


git-svn-id: http://core.svn.wordpress.org/branches/3.8@27884 1a063a9b-81f0-0310-95a4-ce76da25c4cd
  • Loading branch information
@nacin
nacin committed Apr 8, 2014
1 parent ab6a888 commit 78a915e
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion wp-includes/pluggable.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,7 @@ function wp_validate_auth_cookie($cookie = '', $scheme = '') {
$key = wp_hash($username . $pass_frag . '|' . $expiration, $scheme);
$hash = hash_hmac('md5', $username . '|' . $expiration, $key);

if ( $hmac != $hash ) {
if ( hash_hmac( 'md5', $hmac, $key ) !== hash_hmac( 'md5', $hash, $key ) ) {

This comment has been minimized.

Sign in to view

Copy link
@kg

kg Apr 8, 2014

This is really suspect and confusing.

  • $hmac appears to not be defined anywhere in this scope. I assume it is a global set somewhere? Do you handle $hmac not being defined? Is that what you are 'hardening' against?
  • Why are you hashing again when one of the inputs is a MD5 hash? At the very least, $hash is the result of hash_hmac('md5', ...) on the username and expiration, so it is unnecessary to hash it again unless your goal is to somehow 'harden' this test by applying a second level of hashing. It is unclear whether this accomplishes anything since you are doing the second layer for both values at the point of comparison (I can't imagine this does anything). Is the use of hash_hmac on both sides here an attempt to prevent side-channel timing attacks? If so, it is faulty, since you're still doing a string comparison on the end result. If you want to prevent side-channel timing attacks, use a constant-time string comparison, and don't use hash_hmac a second time.
  • Also, one could argue that comparing hashes of the values instead of comparing the values is introducing the possibility of a md5 collision attack here, but since $hash is already an md5 going in to the hash call, I'll assume $hmac is too so the risk is nil.

This comment has been minimized.

Sign in to view

Copy link
@kivikakk

kivikakk Apr 8, 2014

The property of a HMAC is that HMAC(K, x) = HMAC(K, y) if x = y. You've replaced the latter with the former.

(The only thing that should have been done, afaics, is replacing != with !==, and then replacing !== with some constant-time comparison.)

This comment has been minimized.

Sign in to view

Copy link
@nacin

nacin Apr 9, 2014

Author Member

The methodology of this change is described in this post. It's a cheaper fix and isn't prone to implementing an algorithm that looks time-constant but isn't. We may reevaluate this in the future especially as PHP will be introducing a constant-time comparison function in a future version.

This comment has been minimized.

Sign in to view

Copy link
@kivikakk

kivikakk Apr 10, 2014

@nacin: this is very cool: thank you for the link!
do_action('auth_cookie_bad_hash', $cookie_elements);
return false;
}
Expand Down

1 comment on commit 78a915e

@cheald
Copy link

@cheald cheald commented on 78a915e Apr 8, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is ostensibly a guard against a timing attack, why not just use a constant-time comparison to fix it properly?

function secure_compare($a, $b) {
  $len_a = strlen($a);
  $len_b = strlen($b);
  if($len_a == $len_b) {
    $result = 0;
    for($i = 0; $i < $len_a; $i++) {
      $result = $result | (ord($a[$i]) ^ ord($b[$i]));
    }
    return $result === 0;
  } else {
    return false;
  }
}

(Shamelessly adapted from the Rails patch which protected against the same issue)

Please sign in to comment.