Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[11.x] Use secure randomness in Arr::random() and Arr::shuffle() #49642

Merged
merged 4 commits into from
Jan 11, 2024

Conversation

valorin
Copy link
Contributor

@valorin valorin commented Jan 10, 2024

This is my long-awaited second attempt at #46105.

The implementations of Arr::random() and Arr::shuffle(), which are also used by Collection, currently use insecure randomness from array_rand() and shuffle(). This replaces those insecure functions with the secure Randomizer::pickArrayKeys() and Randomizer::shuffleArray().

As far as I can tell, these are the last two "random" functions in the framework that aren't using secure randomness. Str::random() was already random, and Str::password() suffered from an unsecure shuffle which this PR fixes.

I know there was an idea to introduce a standalone Random class into Laravel - which is what I've made with https://github.com/valorin/random. Making something like that as part of the Laravel core is still an option, if that is worth doing? This would allow for custom Randomizer Engines to support seeding, etc.
(Note, my package supports older versions of PHP, while a Laravel core version would just need to be 8.2+.)

Additionally, as part of this change, I removed the $seed functionality from shuffle(). If you're relying on seeding randomness, you should be directly implementing seeded randomness functions to ensure nothing changes code wise - rather than relying on third party functions that aren't guaranteed to not change. The old PR failed because a bunch of folks were seeding randomness outside these functions while also expecting specific results from the different functions.

For folks who don't want to implement their own seeded random functions, my Random package offers a good solution to seeded randomness: https://github.com/valorin/random.

If seeds are being used for tests - we could look at replicating the random string factory in use for Str::random() within Arr as a testing helper?

This is targeted at v11, since it contains backwards compatibility breaks.

@driesvints driesvints changed the title Use secure randomness in Arr::random() and Arr::shuffle() [11.x] Use secure randomness in Arr::random() and Arr::shuffle() Jan 11, 2024
@Jubeki
Copy link
Contributor

Jubeki commented Jan 11, 2024

I think it is create to add secure randomness. But it might not be needed in all cases. Therefor the question: Are there performance implications for example with the array shuffle method?

@valorin
Copy link
Contributor Author

valorin commented Jan 11, 2024

I just ran the following test on my laptop, comparing shuffle() and (new Randomizer)->shuffleArray():

use Illuminate\Support\Benchmark;
use Random\Randomizer;

$list = range(1, 1_000_000);

Benchmark::dd([
    'randomizer' => fn () => (new Randomizer)->shuffleArray($list), 
    'shuffle' => fn () => shuffle($list),
]);
array:2 [
  "randomizer" => "453.126ms"
  "shuffle" => "58.990ms"
]

So it does have an impact, however I'd argue that secure by default is far more important. Folks assume methods like Arr::shuffle() and Arr::random() are secure and can be used for any purpose. If performance becomes an issue, they can implement alternate methods and take the risks associated with those methods.

@Jubeki
Copy link
Contributor

Jubeki commented Jan 11, 2024

I am on the same page with you. I just wanted people to be aware that security comes with performance tradeoffs and how that might impact their decision for which method to use for the right circumstances.

@valorin
Copy link
Contributor Author

valorin commented Jan 11, 2024

Ah, good point, thanks! 🙂

@taylorotwell taylorotwell merged commit d8e5ec4 into laravel:master Jan 11, 2024
29 checks passed
@taylorotwell
Copy link
Member

Thanks 👍

Comment on lines +914 to +917
$this->assertNotEquals(
$input,
Arr::shuffle($input)
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW: This assertion is going to fail in 1 of 720 cases.

Comment on lines +919 to 922
$this->assertNotEquals(
Arr::shuffle($input),
Arr::shuffle($input)
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also going to fail in 1 of 720 cases. Combined this gives a failure rate of 1439/518400 for this testcase, which is roughly 1 in 360, which is too high of a false-positive rate for my taste.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants