Skip to content

random: whitelist arc4random_buf if glibc #8984

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

Closed
wants to merge 1 commit into from

Conversation

crrodriguez
Copy link
Contributor

Glibc will provide the BSD arc4random API, either on version 2.36 or slightly later.. whitelist its implementation as it uses the variant with modern crypto

Glibc will soon implement the BSD arc4random API. whitelist its
implementation as safe.
Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

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

LCTM

@zeriyoshi
Copy link
Contributor

arc4random is a CSPRNG, but getrandom syacalll may give more favorable results.
On Linux, why not use getrandom syscall only when it is not available?

(I am not familiar with the arc4random implementation in glibc)

Also, getrandom syscall seems to be much faster on latest Linux.
https://www.phoronix.com/scan.php?page=news_item&px=Linux-getrandom-8450p

@devnexen
Copy link
Member

Thanks for the pointers ! Let s leave it as is then. I myself mostly know BSD and macOs implementation (not 100% great thus this PR). Pretty interested by your RFC ;-)

@crrodriguez
Copy link
Contributor Author

crrodriguez commented Jul 13, 2022

It all has to do with eliminating as much as possible the syscall overhead, which can be significant if called in a loop. the glibc implementation currently on last-steps review, uses the thread control block to store the state and a buffer of 256 bytes, it reseeds (so syscalls again only after) 16 MB of entropy is consumed.
This is not like the BSD stuff which is per-process, it has larger locking overhead, fork detection, etc.. it only reseeds the state on fork.

Other advantages are that it never blocks, it never fails, so all the error checking is avoided.. the failure path is again different from the BSD.. if for any reason it fails, the BSD versions abort..the glibc version fallbacks to raw getrandom and if that fails then terminates the program.

@devnexen
Copy link
Member

What about this ... Let's revisit this a bit later, not a final no (from my part at least), but the RFC integration being the main point of focus (and trusting @zeriyoshi expertise on that matter) I do not feel the urgency at the moment, no complain about quality/perf so far.

@devnexen devnexen closed this Jul 14, 2022
@zeriyoshi
Copy link
Contributor

Oops, PR is closed...

@crrodriguez
I see! I will take a look at the glibc implementation later too.
Do you have a link I can refer to?

I think this is a very good improvement if it doesn't affect the random number quality.
The system call overhead is very high, so if you can improve it that would be great.

@devnexen devnexen reopened this Jul 14, 2022
@crrodriguez
Copy link
Contributor Author

Oops, PR is closed...

@crrodriguez I see! I will take a look at the glibc implementation later too. Do you have a link I can refer to?

I think this is a very good improvement if it doesn't affect the random number quality. The system call overhead is very high, so if you can improve it that would be great.

Yes, latest version is here https://sourceware.org/pipermail/libc-alpha/2022-July/140627.html the later emails in the thread have arch-specific optimizations et..

@zeriyoshi
Copy link
Contributor

@crrodriguez
I have confirmed. I think this makes sense. Nice!

@devnexen
You can merge this change (+ macOS CCRandomGenerateBytes). I will deal with it later on my PR.

@devnexen
Copy link
Member

@zeriyoshi Thanks for your input.
@crrodriguez Apologies for having closed your PR earlier.

@devnexen devnexen closed this in 3be9118 Jul 15, 2022
@zeriyoshi
Copy link
Contributor

@crrodriguez @devnexen
I'm not too familiar with the situation, but it seems that xxHash is bundled with PHP. xxHash has ChaCha20, but what do you think about implementing a CSPRNG like arc4random, for example, in PHP?

This could improve performance on Linux distributions that do not have glibc, such as Alpine Linux.

@devnexen
Copy link
Member

This is definitively a topic worth discussing IMHO.

@crrodriguez
Copy link
Contributor Author

This is definitively a topic worth discussing IMHO.

Yes, I refraining from suggesting this because it is for another topic.. as far as I am aware the currently most accepted construction is "A fast key erasure RNG" as suggested by D.J.Bernstein. https://blog.cr.yp.to/20170723-random.html

@zeriyoshi
Copy link
Contributor

This is definitively a topic worth discussing IMHO.

Yes, I refraining from suggesting this because it is for another topic.. as far as I am aware the currently most accepted construction is "A fast key erasure RNG" as suggested by D.J.Bernstein. https://blog.cr.yp.to/20170723-random.html

Yes, I know. This is probably better discussed in the Internals ML.
I will send an email later.

@zeriyoshi
Copy link
Contributor

I have sent it. See you on the Internals ML. 😄

@crrodriguez crrodriguez deleted the arc4random_glibc branch July 15, 2022 16:32
@crrodriguez
Copy link
Contributor Author

glibc now has arc4random but the above mentioned implementation was discarded due to security concerns, for now it calls getrandom with the appropiate error checking and has bsd compatible behaviour.. patch still stands.
in future releases either getrandom will be offered via the VDSO to reduce mostly eliminate the syscall overhead or a safe way to buffer a certain amount of entropy in a safe way with kernel assistance.

@zeriyoshi
Copy link
Contributor

zeriyoshi commented Jul 27, 2022

@crrodriguez
I see, thank you very much.
This PR has already been imported into master, but I think it would be better to revert it.
May I do that work?

Off topic: I was thinking it was not possible to vDSO getrandom for safety reasons, but I am curious what approach you would take.

@crrodriguez
Copy link
Contributor Author

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

TimWolla added a commit to TimWolla/php-src that referenced this pull request Jan 20, 2023
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.
TimWolla added a commit that referenced this pull request Jan 23, 2023
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.
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.

3 participants