Page MenuHomePhabricator

Use "random_bytes()" under newer PHP, and introduce "Filesystem::readRandomInteger()"

Authored by epriestley on Jan 16 2019, 5:14 PM.
Referenced Files
Unknown Object (File)
Sun, May 28, 8:51 AM
Unknown Object (File)
Sat, May 27, 3:32 AM
Unknown Object (File)
Wed, May 3, 8:53 AM
Unknown Object (File)
Feb 17 2023, 12:57 PM
Unknown Object (File)
Feb 15 2023, 9:00 PM
Unknown Object (File)
Jan 28 2023, 5:23 AM
Unknown Object (File)
Jan 7 2023, 2:48 PM
Unknown Object (File)
Dec 13 2022, 12:12 PM



Ref T13222. Mostly, I need to generate SMS codes for MFA, e.g. 12345678.

I'm planning to use numeric codes for general consistency/user expectation, since most (all?) other SMS MFA systems I interact with regularly use numeric codes. (We could use alphanumeric codes instead fairly easily, but we'd probably want at least some handling for visually similar glyphs like: "i" vs "I" vs "l" vs "1" vs "|". readRandomCharacters() attempts to limit the fallout here but can still generate "i" and "o", which users might enter as "1" and "0", especially if we happen to send them a code like "635o29".)

PHP 7.2.0 and newer provider random_int() to do this. They also provide random_bytes(), which is approximately a builtin version of our readRandomBytes().

Expose random_int(), falling back to mt_rand() if it's not available (this should be fine for SMS MFA codes). Make readRandomBytes() use random_bytes() if available.

Test Plan
  • Called random_int() and random_bytes() a bit, output sure looked random!
  • This stuff is notoriously hard to verify and I think we kind of just have to trust that these functions (which are intended for this purpose) are our best option until there's evidence they aren't.

Diff Detail

rPHU libphutil
Lint Not Applicable
Tests Not Applicable

Event Timeline

amckinley added inline comments.

FWIW, the PHP docs explicitly say "Caution! This function does not generate cryptographically secure values, and should not be used for cryptographic purposes." I agree with the thrust of your comment, but maybe we should call this readSortaRandomIntegerSuperDangerousDontUse or similar.


Also, the return value is defined as "...or FALSE if max is less than min", so maybe we should check for that condition and throw instead of potentially returning not-an-int.

This revision is now accepted and ready to land.Jan 16 2019, 6:00 PM
This revision was automatically updated to reflect the committed changes.