Move cryptographic functions to Crypt component

This moves drupal_random_bytes() to a new component and replaces all function calls.

This is presumably redundant now our minimum version is 5.3.5, although this should probably be dealt with separately.

This is simply moving code out of bootstrap.inc and into a component, looks good to me.

And if this is now a component that can be used outside of Drupal, I guess the version_compare() test should stay in place.

removed the comment and rerolled
i think maybe we should keep it like this for now, since this is used in mostly procedural code..and we should also add some unit tests..but lets do the conversion first?

newline is there as far as i can see:)
and did not remove the space, cause there is consistent spacing like that in this function, and its out of scope cleaning it up all

ah, thats what you meant:)

agreed on all points above, i ll try to roll a patch with them later

This is no longer relevant as we don't support that version of PHP.

The last submitted patch, drupal-randomBytes-1929288-16.patch, failed testing.

The last submitted patch, drupal-randomBytes-1929288-18.patch, failed testing.

The last submitted patch, drupal-randomBytes-1929288-20.patch, failed testing.

status

yeah well for this to work Uuid needs to be a service..so we either go with #11 or postpone for #1969572: Make Uuid a service

i have to agree about the complexicity
here is the patch in #11 rerolled, with the phpunit added

and with correct docblocks:P

there:)

Crell asked in IRC if all these uses are security critical.

Looking at the patch, of the calls to get random bytes I see, maybe only the UUID one could get replaced with something like uniqid(mt_rand(), TRUE)

That's probably the main one that's a performance hit anyhow.

rootawc, pwolanin, msonnabaum, and I just had a lengthy conversation in IRC about this issue and we're going to change direction a little.

1) We don't need a RandomBytes class. We need a Cyrpt class, which would include randomBytes() as well as hashBase64() and hmacBase64(). They come as a matched set.

2) The $_SERVER here is not being used in a contextual way, just in a "we need some random crap" way; $_SERVER is available in both HTTP and CLI SAPIs in PHP, so it's safe to use for that without having to inject anything. (It has a completely different set of random crap in each case, but it does have data.)

3) That said... UUID's use of random_bytes() isn't necessary anyway since that doesn't need to be a cryptographically strong string. It can just be uniquid().

rootawc is working on a new patch that does the above 3 items. Coming soon to an issue near you!

I did not touch Uuid, since it does not bring problems now, left it as is per Damien's comment.

Besides improving

Anyway, i think besides those above, patch is in good shape, i cant come up with a way to unit test hmacBase64() and hashBase64()

credits to Crell for the docblock:)

I wouldn't object to a randomHashedString($size) method. Seems a reasonable simplification.

here is a total hack

meh, i keep missing stuff

Let's not try to fix what's not broken:

The function should be called Crypt::randomBytes(), and $count should be mandatory.

Speaking of... I don't understand why we do this to begin with:

Hashing a random string can really only result in losing entropy. We should really just convert the random bytes to base64.

Any reason:

<?php
Crypt::hashBase64(uniqid(mt_rand(), TRUE) . mt_rand())
Crypt::hashBase64(uniqid(mt_rand(), TRUE))
Crypt::hashBase64(uniqid(mt_rand(), TRUE) . Crypt::randomString())
?>

Couldnt turn to

<?php
Crypt::randomStringHashed()
?>

amd just work? or objections?

The initial reasoning was to have "safe" and "unsafe" random ids: "safe" are generated from the strong random source, "unsafe" are generated from mt_rand() directly, with some slight (but completely undefined) entropy added by uniqid().

This reasoning doesn't really hold: in PHP, mt_rand() Is a very weak RNG, mostly because its state is not *that* hard to guess (the generator is seeded from values - the request time, the PID of the PHP process, etc. - that are partially observable and not very random). We should (but we don't) seed the generator with strong values before using it. And at this point, the performance difference between generating random bytes from mt_rand() and reading them (in a 8k batch) directly from /dev/urandom is not going to make a difference for us.

So, I would recommend getting rid of all the mt_rand() and the uniqid() in core, and be done with it.

@rootatwc: I'm looking at the why we did that *even before the patch*. I think it is useful to have a function that gets you a random URL-safe identifier, so I'm not criticizing the existence of randomStringHashed() at all. What I'm discussing is why we are hashing + base64-ing the random bytes instead of just base64-ing them directly. This is not changed by this patch.

We use 55 everywhere, but just because we actually want randomStringHashed() everywhere. The default doesn't make sense outside of this particular use-case.

Ah, ok, sorry for misunderstanding:)

About "String" vs "Bytes": a String in Drupal always implies a UTF-8 string. It would be unwise to break this expectation here.

You are right, i am gonna change the method name

The last submitted patch, drupal-crypt-1929288-50.patch, failed testing.

no longer applies, reroll

There should be also an explanation, why we an't yet.

Should all be proper type hinted, sorry.

I proper to use static if possible, as this allows you to override the class later. (you never know what this might be good for).

Looks fine to me.

Er. Change notice.

followup #1985940: Core has no valid use-case for mt_rand() or uniqid()