Page MenuHomePhabricator

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

Authored by epriestley on Jan 16 2019, 5:14 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 10 2024, 5:51 AM
Unknown Object (File)
Feb 3 2024, 8:46 PM
Unknown Object (File)
Dec 27 2023, 1:01 PM
Unknown Object (File)
Dec 27 2023, 1:00 PM
Unknown Object (File)
Dec 27 2023, 1:00 PM
Unknown Object (File)
Dec 27 2023, 1:00 PM
Unknown Object (File)
Dec 22 2023, 1:13 AM
Unknown Object (File)
Dec 22 2023, 1:13 AM
Subscribers
None

Details

Summary

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

Repository
rPHU libphutil
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

amckinley added inline comments.
src/filesystem/Filesystem.php
560

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.

560

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.