WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 7 days ago

Last modified 7 days ago

#43771 closed enhancement (fixed)

use wp_rand instead of mt_rand()

Reported by: BjornW Owned by: johnbillion
Milestone: 4.9.7 Priority: normal
Severity: normal Version:
Component: Security Keywords: needs-patch
Focuses: Cc:

Description

wp_rand() should be used instead of mt_rand() according to the docs:

"WordPress uses wp_rand() in order to create hashes, passwords, and nonces that are far less predictable than the similar PHP native functions like rand() and mt_rand()." Source: https://developer.wordpress.org/reference/functions/wp_rand/

I wonder if it would be better to use SHA1 instead of MD5 as well?

Attachments (2)

43771.patch (439 bytes) - added by BjornW 2 months ago.
Replace mt_rand() with wp_rand()
43771-2.patch (865 bytes) - added by BjornW 3 weeks ago.

Download all attachments as: .zip

Change History (11)

@BjornW
2 months ago

Replace mt_rand() with wp_rand()

#1 @BjornW
2 months ago

  • Keywords has-patch reporter-feedback added

#2 @BjornW
2 months ago

  • Keywords reporter-feedback removed

#3 @BjornW
2 months ago

  • Keywords 2nd-opinion added

This ticket was mentioned in Slack in #core by bjornw. View the logs.


6 weeks ago

#5 follow-up: @johnbillion
4 weeks ago

  • Keywords needs-patch added; dev-feedback has-patch 2nd-opinion removed
  • Milestone changed from Awaiting Review to 4.9.7
  • Owner set to johnbillion
  • Status changed from new to reviewing
  • Version trunk deleted

Thanks for the report and the patch @BjornW!

mt_rand() is also used in update_option_new_admin_email() for the same purpose, so this instance will need to be changed too.

md5() is only used here as a hashing function, not for a cryptographic purpose. The randomness comes from (mt|wp)_rand() and md5() just converts the result into a user-facing, URL-safe format. If the hash was successfully reverse engineered it wouldn't expose any information that isn't already stored along side it in the adminhash option.

#6 @johnbillion
4 weeks ago

I should say at this point that in the future it would be great if you could report security-related issues to the project's HackerOne program instead of here on the public issue tracker. This particular issue is just hardening, but it's always better to be safe than sorry.

@BjornW
3 weeks ago

#7 in reply to: ↑ 5 @BjornW
3 weeks ago

Replying to johnbillion:

Thanks for the report and the patch @BjornW!

mt_rand() is also used in update_option_new_admin_email() for the same purpose, so this instance will need to be changed too.

md5() is only used here as a hashing function, not for a cryptographic purpose. The randomness comes from (mt|wp)_rand() and md5() just converts the result into a user-facing, URL-safe format. If the hash was successfully reverse engineered it wouldn't expose any information that isn't already stored along side it in the adminhash option.

Hi @johnbillion, Thanks for your feedback.

I have updated the patch so it includes update_option_new_admin_email() in wp-admin/includes/misc.php as well.

I'm sorry about posting this here, but after reading WordPress' Policy on HackerOne I got the impression not adhering to the guidelines over there would be considered a very 'Bad Thing™'... Since I couldn't produce a Proof-of-Concept I assumed Trac would suffice instead of HackerOne. Next time I'll err towards HackerOne ;)

PS: I've also done a quick grep (grep -nir 'mt_rand' --include \*.php --exclude .svn) on trunk to see the other uses of mt_rand() instead of wp_rand(), but I'm not ready yet with my assessment of these to see if these need changes as well. I'll update this ticket if any (IMHO) need to be changed.

Thanks

#8 @johnbillion
7 days ago

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

In 43367:

Security: Harden the random aspect of the hash used for user profile and admin email address changes.

Props BjornW

Fixes #43771

#9 @johnbillion
7 days ago

In 43368:

Security: Harden the random aspect of the hash used for user profile and admin email address changes.

Props BjornW

Fixes #43771

Merges [43367] to the 4.9 branch.

Note: See TracTickets for help on using tickets.