Core has no valid use-case for mt_rand() or uniqid()

Updated: Comment #5

Problem/Motivation

Drupal core has occurrences of mt_rand() function

Crypt::hashBase64(uniqid(mt_rand(), TRUE) . mt_rand())
Crypt::hashBase64(uniqid(mt_rand(), TRUE))
Crypt::hashBase64(uniqid(mt_rand(), TRUE) . Crypt::randomString())

The initial reasoning was to have "safe" and "unsafe" random ids: "safe" are generated from the strong random source, "unsafe" are generated from mt_rand() directly, with some slight (but completely undefined) entropy added by uniqid().

Proposed resolution

Crypt::hashBase64(uniqid(mt_rand(), TRUE) . mt_rand())
Crypt::hashBase64(uniqid(mt_rand(), TRUE))
Crypt::hashBase64(uniqid(mt_rand(), TRUE) . Crypt::randomString())

can be changed to

Crypt::randomStringHashed()

The reasons for this change are

• In PHP, mt_rand() Is a very weak RNG, mostly because its state is not *that* hard to guess (the generator is seeded from values - the request time, the PID of the PHP process
• Neither mt_rand() nor uniqid() should reasonably be used in a context where security is expected. Check out this blog post for more information.

Remaining tasks

The remaining tasks are

• The patch is ready for security team to RTBC

Original report by [username]

Any reason:

Crypt::hashBase64(uniqid(mt_rand(), TRUE) . mt_rand())
Crypt::hashBase64(uniqid(mt_rand(), TRUE))
Crypt::hashBase64(uniqid(mt_rand(), TRUE) . Crypt::randomString())

Couldnt turn to

Crypt::randomStringHashed()

amd just work? or objections?

The initial reasoning was to have "safe" and "unsafe" random ids: "safe" are generated from the strong random source, "unsafe" are generated from mt_rand() directly, with some slight (but completely undefined) entropy added by uniqid().

This reasoning doesn't really hold: in PHP, mt_rand() Is a very weak RNG, mostly because its state is not *that* hard to guess (the generator is seeded from values - the request time, the PID of the PHP process, etc. - that are partially observable and not very random). We should (but we don't) seed the generator with strong values before using it. And at this point, the performance difference between generating random bytes from mt_rand() and reading them (in a 8k batch) directly from /dev/urandom is not going to make a difference for us.

So, I would recommend getting rid of all the mt_rand() and the uniqid() in core, and be done with it.