use wp_rand instead of mt_rand()

[BjornW]

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?

[BjornW]

Replace mt_rand() with wp_rand()

[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.

[johnbillion]

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]

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

[johnbillion]

In 43367:

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

Props BjornW

Fixes #43771

[johnbillion]

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.