[wp-trac] [WordPress Trac] #56335: use hash_equals to check password hash

[WordPress Trac]

#56335: use hash_equals to check password hash
-------------------------+------------------------------
 Reporter:  hanshenrik   |       Owner:  (none)
     Type:  enhancement  |      Status:  new
 Priority:  normal       |   Milestone:  Awaiting Review
Component:  Security     |     Version:  trunk
 Severity:  trivial      |  Resolution:
 Keywords:  has-patch    |     Focuses:
-------------------------+------------------------------
Description changed by SergeyBiryukov:

Old description:

> today in wp-includes/class-phpass.php under function CheckPassword we
> find
>
> ```
>                 # This is not constant-time.  In order to keep the code
> simple,
>                 # for timing safety we currently rely on the salts being
>                 # unpredictable, which they are at least in the non-
> fallback
>                 # cases (that is, when we use /dev/urandom and bcrypt).
>                 return $hash === $stored_hash;
> ```
> and while i agree that a constant-time comparison is probably not needed,
> it's a trivial change to fix it, and better safe than sorry. I suggest
> changing it to
> ```
>                 if(PHP_VERSION_ID >= 50600){
>                         return hash_equals($stored_hash, $hash);
>                 } else {
>                         # This is not constant-time.  In order to keep
> the code simple,
>                         # for timing safety we currently rely on the
> salts being
>                         # unpredictable, which they are at least in the
> non-fallback
>                         # cases (that is, when we use /dev/urandom and
> bcrypt).
>                         return $hash === $stored_hash;
>                 }
> ```
>
> PHP_VERSION_ID was introduced in 5.2.7, and i doubt WordPress still need
> to support PHP5.2. Unsure if WordPress still support 5.5? if the answer
> is no, the entire PHP_VERSION_ID can be removed.

New description:

 today in wp-includes/class-phpass.php under function CheckPassword we find

 {{{
                 # This is not constant-time.  In order to keep the code
 simple,
                 # for timing safety we currently rely on the salts being
                 # unpredictable, which they are at least in the non-
 fallback
                 # cases (that is, when we use /dev/urandom and bcrypt).
                 return $hash === $stored_hash;
 }}}
 and while i agree that a constant-time comparison is probably not needed,
 it's a trivial change to fix it, and better safe than sorry. I suggest
 changing it to
 {{{
                 if(PHP_VERSION_ID >= 50600){
                         return hash_equals($stored_hash, $hash);
                 } else {
                         # This is not constant-time.  In order to keep the
 code simple,
                         # for timing safety we currently rely on the salts
 being
                         # unpredictable, which they are at least in the
 non-fallback
                         # cases (that is, when we use /dev/urandom and
 bcrypt).
                         return $hash === $stored_hash;
                 }
 }}}

 PHP_VERSION_ID was introduced in 5.2.7, and i doubt WordPress still need
 to support PHP5.2. Unsure if WordPress still support 5.5? if the answer is
 no, the entire PHP_VERSION_ID can be removed.

--

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/56335#comment:3>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform