Page MenuHomePhabricator

Default to openssl_random_pseudo_bytes instead of /dev/urandom.
ClosedPublic

Authored by gabe on Nov 7 2013, 9:55 PM.

Details

Summary

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.

Test Plan

Add unit test for readRandomBytes() Would be nice to have someone test this on windows. Working on ubuntu at the moment.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

epriestley accepted this revision.Nov 7 2013, 10:08 PM

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
390–391

We could remove this, since we no longer use it.

404–405

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.

415–416

This string -- and thus the error message -- includes a huge amount of whitespace and a newline. Better as:

'a b c '.
'd'

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
gabe updated this revision to Unknown Object (????).Nov 7 2013, 10:28 PM

Diff against master, as requested by @epriestley

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?

gabe updated this revision to Unknown Object (????).Nov 7 2013, 10:33 PM

Take 2. Arc diff upstream/master

epriestley closed this revision.Nov 7 2013, 10:35 PM
epriestley added a subscriber: gabeguz.

Closed by commit rPHU13ad06352d84 (authored by @gabeguz, committed by @epriestley).