When available, prefer php's built in openssl_random_pseudo_bytes() to /dev/urandom. Test for functionality and fallback to /dev/urandom if necessary to get data. Simplifies the code a bit, and is somewhat more functional as running inside a chroot is now possible.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Commits
- rPHU13ad06352d84: Default to openssl_random_pseudo_bytes instead of /dev/urandom.
Add unit test for readRandomBytes() Would be nice to have someone test this on windows. Working on ubuntu at the moment.
Diff Detail
Diff Detail
- Branch
- master
- Lint
Lint Errors Severity Location Code Message Error src/filesystem/Filesystem.php:398 XHP31 Use Of PHP 5.3 Features Error src/filesystem/Filesystem.php:517 XHP31 Use Of PHP 5.3 Features - Unit
Tests Passed
Event Timeline
Comment Actions
Thanks!
I googled for openssl_random_pseudo_bytes and clicked at least one link without seeing any warnings that it was completely broken.
I also ran it manually a few times on my machine and looked at the output, which sure looked random to me.
These diligent actions of careful research have given me a high degree of confidence that this does not compromise anything.
I'll fix the two tiny whitespace/comment things in the pull.
| src/filesystem/Filesystem.php | ||
|---|---|---|
| 392 | We could remove this, since we no longer use it. | |
| 406–407 | If the caller requests readRandomBytes(0), and the fread fails, the strlen() of false is 0, so this check will pass, and then we'll return false instead of '' (empty string). I think this is fine since readRandomBytes(0) is pathological, I'm just proving that I'm paying attention. The old implementation had the same problem anyway. | |
| 417–418 | This string -- and thus the error message -- includes a huge amount of whitespace and a newline. Better as: 'a b c '. 'd' | |
Comment Actions
Can you give me a diff against origin/master? It looks like there's some phutil_is_windows() removal stuff behind this on your branch. I can manually sort that out but it's probably easier to just have a full patch here.
arc diff origin/master
Comment Actions
Hmm, it looks like that's not origin/master -- the origin has this on 396:
if (phutil_is_windows()) {I'm guessing you have a commit immediately behind this which removed that?