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

Authored by epriestley on Jan 16 2019, 5:14 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13222: 2018 Week 48-51 Bonus Content

Commits

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

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

epriestley created this revision.Jan 16 2019, 5:14 PM

Harbormaster completed remote builds in B21536: Diff 47683.Jan 16 2019, 5:14 PM

epriestley requested review of this revision.Jan 16 2019, 5:14 PM

amckinley accepted this revision.Jan 16 2019, 6:00 PM

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.