Skip to content

random: Use CSPRNG for CombinedLCG seeding #13748

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

Merged
merged 1 commit into from
Mar 19, 2024
Merged

Conversation

TimWolla
Copy link
Member

Now that the CombinedLCG is no longer used within GENERATE_SEED(), we can safely use the CSPRNG with a php_random_generate_fallback_seed() fallback to seed the CombinedLCG.

Now that the CombinedLCG is no longer used within GENERATE_SEED(), we can
safely use the CSPRNG with a php_random_generate_fallback_seed() fallback to
seed the CombinedLCG.
Copy link
Contributor

@zeriyoshi zeriyoshi left a comment

Choose a reason for hiding this comment

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

I believe this is not a problem, but it might be a good idea to interview active extension users to avoid confusion like we had with MT before.

@TimWolla
Copy link
Member Author

to avoid confusion like we had with MT before.

Can you clarify what confusion you are referring to?


For the CombinedLCG: It is not seedable from userland and the seeding strategy in php_random_combinedlcg_seed_default should not be externally observable. It even includes a fallback for the rare case where the CSPRNG fails. So this should be a 100 % safe change.

@zeriyoshi
Copy link
Contributor

to avoid confusion like we had with MT before.

Can you clarify what confusion you are referring to?

For the CombinedLCG: It is not seedable from userland and the seeding strategy in php_random_combinedlcg_seed_default should not be externally observable. It even includes a fallback for the rare case where the CSPRNG fails. So this should be a 100 % safe change.

Sorry. The intonation of the words was incorrect. It was not meant to be a mast, but only a reminder to those who saw my comment.

I don't see a problem either. I searched for such an extension on GitHub just to be sure, but I don't seem to find one. No problem!

@TimWolla
Copy link
Member Author

It was not meant to be a mast, but only a reminder to those who saw my comment.

Sorry, I don't follow here. I don’t believe “mast” is the English word you wanted to use here.


Anyway, merging as you confirmed this looks good to you!

@TimWolla TimWolla merged commit 807524d into php:master Mar 19, 2024
@TimWolla TimWolla deleted the lcg-seed branch March 19, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants