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

random: Do not trust arc4random_buf() on glibc #10390

Merged
merged 2 commits into from
Jan 23, 2023

Conversation

TimWolla
Copy link
Member

Targeting PHP-8.2, because the the original PR was landed in PHP-8.2 and it does not appear to be useful to allow this for the entirety of 8.2 only, but no other branch.

/cc @crrodriguez


This effectively reverts #8984.

As discussed in #10327 which will enable the use of the getrandom(2) syscall on NetBSD instead of relying on the userland arc4random_buf(), the CSPRNG should prioritize security over speed [1] and history has shown that userland implementations unavoidably fall short on the security side. In fact the glibc implementation is a thin wrapper around the syscall due to security concerns and thus does not provide any benefit over just calling getrandom(2) ourselves.

Even without any performance optimizations the CSPRNG should be plenty fast for the vast majority of applications, because they often only need a few bytes of randomness to generate a session ID. If speed is desired, the OO API offers faster, but non-cryptographically secure engines.

This effectively reverts php#8984.

As discussed in php#10327 which will enable the use of the getrandom(2) syscall on
NetBSD instead of relying on the userland arc4random_buf(), the CSPRNG should
prioritize security over speed [1] and history has shown that userland
implementations unavoidably fall short on the security side. In fact the glibc
implementation is a thin wrapper around the syscall due to security concerns
and thus does not provide any benefit over just calling getrandom(2) ourselves.

Even without any performance optimizations the CSPRNG should be plenty fast for
the vast majority of applications, because they often only need a few bytes of
randomness to generate a session ID. If speed is desired, the OO API offers
faster, but non-cryptographically secure engines.
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Hmm, unsure about this. @crrodriguez commented:

You do not need to revert it. it is safe.. and will later be improved as kernel/libc versions advance.

I think both of you have a point. But regardless which solution we prefer, we should probably add a brief comment in the source code, so we won't switch back and forth over time.

@TimWolla
Copy link
Member Author

we should probably add a brief comment in the source code, so we won't switch back and forth over time.

That comment will be added by @devnexen's #10327 and is already part of that PR. The revert for glibc is also consistent with NetBSD's migration to the syscall. I wanted to get this PR out first, such that devnexen's comment is actually factually accurate. I also hope to clean up the whole CSPRNG implementation a little further, the removal of the HAVE_DEV_URANDOM check and #10392 is a first part of that.

You do not need to revert it. it is safe.. and will later be improved as kernel/libc versions advance.

I believe that any kind of userland solution is unsafe. If glibc is just a thin wrapper around getrandom(2) (as it is now) then it does not bring us any benefit, we need to maintain the getrandom(2) logic anyway. If glibc does more than wrapping getrandom(2), then it is likely unsafe.

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

That comment will be added by @devnexen's #10327 and is already part of that PR.

Ah, nice!

Then I'm fine with this change.

@TimWolla TimWolla merged commit 57b362b into php:PHP-8.2 Jan 23, 2023
TimWolla added a commit that referenced this pull request Jan 23, 2023
* PHP-8.2:
  random: Do not trust arc4random_buf() on glibc (#10390)
@TimWolla TimWolla deleted the csprng-glibc branch January 23, 2023 17:22
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.

4 participants