WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#10284 closed defect (bug) (fixed)

hash_hmac implementation does not match PHP hash_hmac

Reported by: jrush_aplus Owned by: mdawaffe
Milestone: 2.9 Priority: normal
Severity: normal Version: 2.8
Component: General Keywords: has-patch tested commit
Focuses: Cc:

Description

The hash_hmac implementation output does not match the native PHP hash_hmac output when using a key longer than 64 characters.

If the key is longer than 64 characters, it is packed. The output of pack may be less than 64 characters, so the key needs to be padded. The current implementation does not pad the key because the key was packed.

The attached patch removes the else that keeps a packed key from being padded. By removing the else, the length of the key is recalculated and will be padded if it is less than 64 characters.

If the key is padded after being packed, the output matches the output of the native PHP hash_hmac function.

Attachments (3)

compat.patch (412 bytes) - added by jrush_aplus 5 years ago.
Patch to allow compat.php hash_hmac implementation to match PHP hash_hmac output
hash_hmac_test.php (1.5 KB) - added by peaceablewhale 5 years ago.
test script
10284.diff (699 bytes) - added by mdawaffe 5 years ago.
correct key padding and add support for raw_output in hash_hmac

Download all attachments as: .zip

Change History (14)

jrush_aplus5 years ago

Patch to allow compat.php hash_hmac implementation to match PHP hash_hmac output

comment:1 peaceablewhale5 years ago

  • Keywords has-patch tested added
  • Milestone changed from Unassigned to 2.8.1
  • Version set to 2.8

peaceablewhale5 years ago

test script

comment:2 markjaquith5 years ago

What implications does this have for existing sites if the patch is committed?

comment:3 mdawaffe5 years ago

Outputs from core uses of hash_hmac() will not change for most blogs. The key length provided by https://api.wordpress.org/secret-key/1.1/ are all 64 characters long. (Under the assumption that most blogs use either random keys from above, the default key, or shorter custom keys).

However, most outputs from core uses of wp_hash() will change on most blogs.

So after patching on most blogs, all users on those blogs will be logged out. Logging back in will not be affected. Also, all nonces will be invalid, but new and valid ones will generate just fine.

A temporary and one time effect (the same effect many upgrades face: cookies and nonces are invalid after upgrade).

+1

comment:4 mdawaffe5 years ago

  • Owner set to mdawaffe
  • Status changed from new to accepted

WordPress' implementation of hash_hmac() also ignores the $raw_output argument.

That argument is never used by core, but plugins and OAuth libraries use it. Attached updates to correct the above bug and add support for $raw_output.

mdawaffe5 years ago

correct key padding and add support for raw_output in hash_hmac

comment:5 mdawaffe5 years ago

  • Keywords commit added

10623.diff can be deleted - wrong patch.
10284.diff is correct.

comment:6 dd325 years ago

10623.diff can be deleted - wrong patch. 10284.diff is correct.

Deleted 10623.diff so as not to confuse anyone, As the filename suggests, it was obviously for another ticket.

comment:7 westi5 years ago

  • Cc westi added

This looks ok.

Working on writing up some test cases for wordpress-tests.

I guess the changes detailed above only affect blogs running on a PHP version which needs this compat function - so anything earlier than 5.1.2

comment:8 westi5 years ago

(In [11920]) Make our hash_hmac compatibility function unit testable even when the real one exists. See #10284.

comment:9 westi5 years ago

  • Milestone changed from 2.8.5 to 2.9

comment:11 westi5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [11921]) Correct key padding and add support for raw_output in hash_hmac. Fixes #10284 props mdawaffe.

Note: See TracTickets for help on using tickets.