Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#32778 closed defect (bug) (fixed)

hash_equals() does not compare strings in constant time

Reported by: nbachiyski's profile nbachiyski Owned by: nbachiyski's profile nbachiyski
Milestone: 4.5 Priority: normal
Severity: normal Version: 3.9.2
Component: General Keywords: has-patch
Focuses: docs Cc:

Description

The phpdoc of hash_equals says: “Compare two strings in constant time.”

The term “constant time” is widely used in computer science and means O(1) time complexity, or its running time not depending o the size of the input data. This is not the case with hash_equals, where we only make sure that input data with the same size takes the same amount of time.

The first time I saw the WordPress code it took me few minutes to understand what was going on.

Is it possible that we steal the description from php.net: “Timing attack safe string comparison” and link to the documentation there at: http://php.net/hash_equals?

Attachments (1)

32778.patch (747 bytes) - added by AramZS 9 years ago.
Clarifying hash_equals in a way consistent with PHP docs.

Download all attachments as: .zip

Change History (7)

#1 @Otto42
9 years ago

Is this a suggestion to change the function, or just the description of it?

Does it perform the same as php's hash_equals? More or less, I mean? If so, then equating the description is fine.

#2 @dd32
9 years ago

Does it perform the same as php's hash_equals? More or less, I mean? If so, then equating the description is fine.

It performs exactly the same as the PHP function - literally a port of that function to pure PHP.

Given there have been several security reports of "hash_equals() is not constant-time when string lengths differ" (which is the intended behaviour - as most string lengths are known inside PHP/WordPress already, so preventing that is mostly pointless and complex) I'd agree that making it clearer in the docs is worthwhile, if it can be done :)

#3 @jeremyfelt
9 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from trunk to 3.9.2

@AramZS
9 years ago

Clarifying hash_equals in a way consistent with PHP docs.

#4 @AramZS
9 years ago

  • Keywords has-patch added; needs-patch removed

Patch attached to clarify it in a way consistent with the documentation listed on PHP.net for the ported function. Should fulfill the clarification needs.

#5 @nbachiyski
9 years ago

  • Owner set to nbachiyski
  • Resolution set to fixed
  • Status changed from new to closed

In 35805:

Docs: clarify inline docs for hash_equals

Before the docs implied the complexity of the function was O(1) by using the term "constant time", now we use the more descriptive term "Timing attack safe".

Props AramZS.
Fixes #32778.

#6 @netweb
9 years ago

  • Milestone changed from Future Release to 4.5
Note: See TracTickets for help on using tickets.