Use system random generator in crypto #5798

Activity

1. mscdex

   added

   cryptoIssues and PRs related to the crypto subsystem.Issues and PRs related to the crypto subsystem.

   feature requestIssues requesting new Node.js features.Issues requesting new Node.js features.

   on Mar 19, 2016
2. bnoordhuis commented on Mar 19, 2016

   

   bnoordhuis

   on Mar 19, 2016

   Member

   More actions

   Instead of linking to a blog post, please summarize the pros and cons here.

   😄React with 😄2Reacted by armine-hayrapetyan-im and lavreh

   ❤️React with ❤️1Reacted by Forest Mars
3. ChALkeR commented on Mar 19, 2016

   

   ChALkeR

   on Mar 19, 2016

   Member

   More actions

   Perhaps @joepie91 or @paragonie-scott would be interested to elaborate that here.
4. paragonie-scott commented on Mar 19, 2016

   

   paragonie-scott

   on Mar 19, 2016

   More actions

   From "a blog post":

   Why not {SecureRandom, OpenSSL, havaged, &c}?

   These are userspace CSPRNGs. You want to use the kernel’s CSPRNG, because:

   • The kernel has access to raw device entropy.
   • It can promise not to share the same state between applications.
   • A good kernel CSPRNG, like FreeBSD’s, can also promise not to feed you random data before it’s seeded.

   Additionally, OpenSSL's userspace PRNG has caused issues in other languages (PHP under Apache for sure) because it's not fork-safe, which caused random numbers to repeat. (Combine this with the consequences of nonce reuse for most stream ciphers and boom your cryptosystem loses all privacy.)

   A safer bet would be to adopt what PHP 7's random_bytes() does:
   https://github.com/php/php-src/blob/e8f056e535813bf864743626c3a208ceafee70b9/ext/standard/random.c#L83-L186

   👍React with 👍15Reacted by Stephen Oliver, Ondrej Mosnáček, Quinn Wilton, Dennis de Greef, 0xBADF00D, Tobias Feller, Stephen Walker-Weinshenker, Jarek Samic, Kyhwana Pardus, Jamie and 5 more
5. bnoordhuis commented on Mar 19, 2016

   

   bnoordhuis

   on Mar 19, 2016

   Member

   More actions

   OpenSSL's PRNG is seeded from /dev/{,s,u}random, can get entropy from entropy-collecting daemons, and forking is not an issue for node.js. So far I'm not seeing a compelling reason to switch.
6. ChALkeR commented on Mar 19, 2016

   

   ChALkeR

   on Mar 19, 2016

   Member

   More actions

   @paragonie-scott That's does not look like a good reasoning. Could you please be more calm? ;-)

   👍React with 👍1Reacted by Jens

   👎React with 👎23Reacted by rc4, Ackermann Yuriy, Lura Skye, Mae Kennedy, 0xBADF00D, Christopher Davis, Kevin Mark, Rajmund Csombordi, Nick, Stephen Walker-Weinshenker and 13 more

   😄React with 😄8Reacted by kelvin knighton, Pete Nykänen, Phileas Lebada, Ian Stapleton Cordasco, tetyys, Marcos Bérgamo, Jarek Samic and Artur Ergashev
7. joepie91 commented on Mar 19, 2016

   

   joepie91

   on Mar 19, 2016

   Contributor

   More actions

   @bnoordhuis Trying to find specific reasons to explain away potential issues is not a good approach to security-related matters. The issues exist, and there's no way to predict how Node will evolve - for example, if fork-safety might start to matter due to future changes, at which point people will likely have forgotten about this specific issue.

   We should be striving for the optimally secure implementation (within technical constraints), instead of attempting to 'defend' the current implementation when there are known edge cases / issues with it.

   This post addresses that further.

   👍React with 👍22Reacted by Jan Gerlinger, Logan Bell, Pierre-Yves Gérardy, Ondrej Mosnáček, Peter Faiman, Justin Barber, Quinn Wilton, Jason Axley, Oliver Bestwalter, Dennis de Greef and 12 more
8. ChALkeR commented on Mar 19, 2016

   

   ChALkeR

   on Mar 19, 2016

   Member

   More actions

   I do not yet have a strong opinion on this. I called @joepie91 and @paragonie-scott here because they expressed similar considerations as @speakeasypuncture in an IRC chat earlier.

   @bnoordhuis, as I understand their points, the reasons here are:

   1. System PRNG is good for this, is known to be stable and hasn't caused many security issues.
   2. Adding a user-space PRNG of top of that doesn't make it more secure, but it makes it more prone to errors.
   3. OpenSSL isn't actually a very high code quality product.
   4. Using OpenSSL PRNG caused serious security issues for some projects, namely for Debian, Android and PHP (that latter is still not fixed, btw), though that was mostly their fault. Those issues are not directly applicable to Node.js, but this gives an uneasy feeling.
   5. There seems to be no reason not to ditch OpenSSL PRNG and reduce the possible errors.

   Everyone — did I miss anything?

   👍React with 👍5Reacted by Jan Gerlinger, Brian M Hunt, Ackermann Yuriy, Jason Axley and Linus Unnebäck

   ❤️React with ❤️1Reacted by Extarys
9. ChALkeR

   added

   securityIssues and PRs related to security.Issues and PRs related to security.

   on Mar 19, 2016
10. ChALkeR commented on Mar 19, 2016

    

    ChALkeR

    on Mar 19, 2016

    Member

    More actions

    Again: I do not yet have a strong opinion on this.

    I would like to hear what would the drawbacks of this change be, and is there something where (or why) OpenSSL PRNG could be better than the system PRNG.
    Excluding the fact that it's already used, of course — that also isn't a good enough argument, this won't even be a semver-major change. It's not documented that crypto.randomBytes() uses OpenSSL.
11. ChALkeR commented on Mar 19, 2016

    

    ChALkeR

    on Mar 19, 2016

    Member

    More actions

    /cc @nodejs/crypto.
12. ChALkeR commented on Mar 19, 2016

    

    ChALkeR

    on Mar 19, 2016

    Member

    More actions

    @mscdex I'm not sure if this is a «feature request», it looks like a proposal to change the implementation of crypto.randomBytes() to me.
13. joepie91 commented on Mar 19, 2016

    

    joepie91

    on Mar 19, 2016

    Contributor

    More actions

    @ChALkeR Your summary seems accurate to me.
14. paragonie-scott commented on Mar 19, 2016

    

    paragonie-scott

    on Mar 19, 2016

    More actions

    Also, random number generators are hot-swappable without compatibility concerns, only security concerns.
15. 147 remaining items

    Load more
16. saghul

    added a commit that references this issue on Aug 15, 2017

    unix,win: add uv_randbytes

    ...

    4f35250
17. Trott

    removed

    discussIssues opened for discussion and feedback.Issues opened for discussion and feedback.

    on Mar 11, 2018
18. davisjam commented on Mar 13, 2018

    

    davisjam

    on Mar 13, 2018

    Last edited by davisjam

    Contributor

    More actions

    [Edited arguments for and against based on feedback]

    I'm going to attempt to summarize the conversation and arguments that have happened on this issue. Sorry if I missed your post, I can amend.

    Preliminaries

    @speakeasypuncture initially opened this issue.
    Per @joepie91, this issue is about Node's crypto.randomBytes. (Statement).

    Issue statement

    Node's crypto.randomBytes may not be cryptographically secure.

    • Operating systems expose a "system" RNG (Cryptographically Secure Pseudo-Random Number Generator or CSPRNG) which offers the best source of randomness on the system.
    • Rather than use the system RNG, Node.js relies on OpenSSL for crypto.randomBytes.
    • OpenSSL can be no more secure than the system RNG, and may be less secure. OpenSSL's use of a user-space CSPRNG on top of the kernel's CSPRNG is unnecessary and simply adds a layer of vulnerability.

    I do not believe anyone has claimed that OpenSSL's CSPRNG is currently broken, although @ChALkeR pointed out some reasons why it might not be trustworthy.

    Proposal

    Node should consider an alternative implementation for random numbers that is definitely derived securely.

    1. @azet says that libsodium is a better option for random numbers than OpenSSL is, because libsodium underwent a review.
    2. @FiloSottile here and @ircmaxell here add that avoiding reliance on a user-space CSPRNG, whether derived from a kernel CSPRNG or not, would reduce Node's attack surface.

    Arguments against

    The major arguments from the Node maintainers (and I may be reading between the lines a little bit) appear to be:

    • @bnoordhuis here and @sam-github here: if Node's security is broken because of issues in OpenSSL, it seems like the right answer is to pursue enhancements to OpenSSL. @shigeki here says that efforts along these lines are underway.
    • Code inertia. The OpenSSL-based implementation works. Changing the implementation to one derived from another library means investing time and energy into development and maintenance.

    Peer pressure

    • @azet said Python uses the system CSPRNG rather than a user-space CSPRNG.
    • @paragonie-scott pointed to PHP's implementation (link), and @azet pointed to Ruby's.
    • @bnoordhuis indicated that implementing PRNG logic in Node directly seemed undesirable.

    My impressions

    • This proposal seems unlikely to result in motion from the Node maintainers until the OpenSSL API is available.
    • It seems like adopting libsodium would lead to unnecessary code churn because the OpenSSL API is on its way, albeit perhaps slowly.
    • In the interim, if security-conscious developers don't trust OpenSSL, they can use an npm module or an add-on that accesses platform-specific sources of randomness directly.

    ❤️React with ❤️7Reacted by Forest Mars, Tony Quetano, Nicholas Sherlock, Robert Kieffer, b1ue, Kevin Brown and Soroush
19. paragonie-scott commented on Mar 13, 2018

    

    paragonie-scott

    on Mar 13, 2018

    Last edited by paragonie-scott

    More actions

    There's an upstream proposal to make OpenSSL create the equivalent of libsodium's randombytes_sysrandom() (which uses the OS's CSPRNG) which Node could then use instead of the userspace PRNG. This isn't close-worthy, it's stalled until OpenSSL does something.

    Swapping out one userspace PRNG for another userspace PRNG is not a fix. Bypassing the userspace and using the OS's CSPRNG is the solution.
20. Trott

    added

    stalledIssues and PRs manually marked as stalled and scheduled for automatic closure.Issues and PRs manually marked as stalled and scheduled for automatic closure.

    and removed

    stalledIssues and PRs manually marked as stalled and scheduled for automatic closure.Issues and PRs manually marked as stalled and scheduled for automatic closure.

    on Mar 13, 2018
21. bnoordhuis commented on Mar 13, 2018

    

    bnoordhuis

    on Mar 13, 2018

    Member

    More actions

    This isn't close-worthy, it's stalled until OpenSSL does something.

    It's not actionable at the moment and won't be for a long time to come. Inactionable items clog up the bug tracker so I'll close this out for now. We can revisit when we upgrade to an openssl version that supports this new API.
22. bnoordhuis

    closed this as completedon Mar 13, 2018
23. ChALkeR commented on Mar 13, 2018

    

    ChALkeR

    on Mar 13, 2018

    Member

    More actions

    @bnoordhuis How are we going to remember to revisit it, then?

    Perhaps introducing a separate label for temporary-inactionable items that were closed just because of that is needed so we don't forget to revisit?
24. bnoordhuis commented on Mar 13, 2018

    

    bnoordhuis

    on Mar 13, 2018

    Member

    More actions

    We go over the changelog with a fine-tooth comb whenever we upgrade (at least I do) so it's unlikely that such a change would go unnoticed.

    Perhaps introducing a separate label for temporary-inactionable items that were closed just because of that is needed so we don't forget to revisit?

    I'm not completely opposed.
25. ChALkeR

    added

    revisit later

    on Mar 13, 2018
26. FiloSottile commented on Mar 14, 2018

    

    FiloSottile

    on Mar 14, 2018

    More actions

    Thanks for summarizing, but I don't think that captures the point. We are not advocating for reimplementing something that's in a library (for whatever reason), but for removing the library entirely. The library implements a user-space CSPRNG on top of kernel entropy, it might be good or bad, doesn't matter, we are arguing for not using one at all.

    What you say is valid about primitives, which have to be implemented somewhere, but here we are talking about removing a layer from a system, where each layer is an independent point of failure.

    As for "they are the ones that know about security", OpenSSL is bound by a lot of legacy, BoringSSL and libsodium are widely regarded as secure modern implementations, and those use the kernel CSPRNG.

    👍React with 👍6Reacted by Scott, James C. (Jamie) Davis, AJ Jordan, Abigail O'Rourke, Eden Tyler-Moss and Charmander
27. davisjam commented on Mar 14, 2018

    

    davisjam

    on Mar 14, 2018

    Contributor

    More actions

    @FiloSottile Thanks, I've amended my post. Holler if you're still unhappy with my representations.

    👍React with 👍1Reacted by Filippo Valsorda
28. atoponce commented on Feb 24, 2022

    

    atoponce

    on Feb 24, 2022

    More actions

    What's the current status with this issue?
29. technion commented on Feb 25, 2022

    

    technion

    on Feb 25, 2022

    More actions

    @atoponce I could well have made a mistake somewhere, but tracing randomBytes appears to land here:

    https://github.com/nodejs/node/blob/f69a4f61b2c133878159a9573ac438d58f8a05ac/src/crypto/crypto_random.cc

    In which we first call RAND_status and then RAND_byes, these being OpenSSL functions.

    👍React with 👍1Reacted by aronis-clearco