Node: Use system random generator in crypto

[speakeasypuncture]

randomBytes uses OpenSSL as its random number generator. It would be wiser and less errorprone to use a system RNG like urandom on Unix platforms, getrandom syscall on Linux and CryptGenRandom on Windows.

[bnoordhuis]

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

[ChALkeR]

Perhaps @joepie91 or @paragonie-scott would be interested to elaborate that here.

[paragonie-scott]

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

[bnoordhuis]

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.

[ChALkeR]

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

[joepie91]

@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.

[ChALkeR]

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?

[ChALkeR]

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.

[ChALkeR]

/cc @nodejs/crypto.

[ChALkeR]

@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.

[joepie91]

@ChALkeR Your summary seems accurate to me.

[paragonie-scott]

Also, random number generators are hot-swappable without compatibility concerns, only security concerns.

[indutny]

I'm -1 on this, no compelling reasons for me.

[joepie91]

@indutny Please refer back to this comment in particular, plus several others in the thread. There is a lack of reasons _not_ to do this (insofar anybody has stated them, that is), while there are documented reasons to do it.

If you feel that there is a reason _not_ to do it, then please share it - but "no compelling reasons for me" really isn't a sufficient argument for a security-related matter. Even a small defect can have disastrous consequences.

[indutny]

@joepie91 sure, sorry for too short reply.

What about systems with "good" PRNG? How many of them are there? Do we have to carry both implementations to support them?

[joepie91]

@indutny Thanks for the elaboration.

As I understand it (and please correct me if not), OpenSSL depends on the system PRNG to begin with. I'm not aware of any platforms (of those supported by Node, that is) where OpenSSL can provide a better PRNG than the one that the system offers natively.

It should thus be possible to just remove OpenSSL's PRNG from the equation entirely, and rely purely on the system PRNG, as PHP has done.

[indutny]

@joepie91 do you suggest to use this randomness for TLS protocol as well? I'm not sure if it is possible, though.

[joepie91]

While that would probably be nice (albeit requiring more investigation), I don't think that's doable. As far as I know, OpenSSL's other functionality relies internally on its own PRNG with no ability to change that - unless we want to get rid of OpenSSL entirely, which would be a separate proposal (and likely not viable at this stage, given the lack of well-tested alternatives).

So, this specific proposal would concern the "user-facing" randomBytes method only.

[indutny]

Ok, considering arguments it probably make more sense now.

[bnoordhuis]

Here's a reason not to switch: OpenSSL's PRNG is a known quantity, the strength of platform-specific PRNGs is not. If the next Windows or FreeBSD release has a flawed PRNG, that will compromise node.js.

If (generic) you think moving to platform-specific solutions is the way forward, get OpenSSL to adopt your approach and node.js will automatically go along. I believe @paragonie-scott is volunteering? He sure seems to feel strongly about it.

if fork-safety might start to matter due to future changes, at which point people will likely have forgotten about this specific issue.

@joepie91 The bucket list of fork-safety issues that would have to be addressed is so long that I think it's safe to say that node.js will never be fork-safe. There are many things that keep me awake at night but this is not one of them.

[alfiepates]

If the next Windows or FreeBSD release has a flawed PRNG, that will compromise node.js.

I don't believe this argument holds any merit, honestly.

~FreeBSD are not just going to push broken code~~ EDIT: FreeBSD are not going to push broken code to _the -STABLE kernel branch_, especially when they have a perfectly functional CSPRNG already, and _especially_ as they have a security team of their own who would definitely catch any changes to the CSPRNG which would lead to a vulnerability in the OS.

I'm going to be frank, here: Attempting to justify away security issues is downright irresponsible, and possibly dangerous. You _need_ to strive for bulletproof security, or you may as well not implement any security at all (And no, don't twist this to mean "Don't bother with security", because that's just lazy.)

I appreciate it's "extra work" to make a change like this, but considering you're using a PRNG which has _actually caused security issues in the past_, I'd err on the safe side and move to something more provably secure.

[bnoordhuis]

as they have a security team of their own who would definitely catch any changes to the CSPRNG which would lead to a vulnerability in the OS

Besides being an appeal to authority, you're asking (generic) me to trust several teams of implementors (the platforms) instead of just one (OpenSSL) to get their implementation right.

considering you're using a PRNG which has actually caused security issues in the past

Are you saying you feel OpenSSL's PRNG is insecure? If so, why aren't you taking that up with the OpenSSL team? Griping about it here isn't going to do any good.

I'll repeat what I've said above: get the upstream project to move over, and we as downstream consumers will automatically move along with it.

[joepie91]

Here's a reason not to switch: OpenSSL's PRNG is a known quantity, the strength of platform-specific PRNGs is not. If the next Windows or FreeBSD release has a flawed PRNG, that will compromise node.js.

Yet OpenSSL's PRNG relies on these platform-specific PRNGs, and mixing in one broken PRNG can weaken the entire (combined) PRNG. How does relying on OpenSSL fix the issue you've described?

The bucket list of fork-safety issues that would have to be addressed is so long that I think it's safe to say that node.js will never be fork-safe. There are many things that keep me awake at night but this is not one of them.

That is a very dangerous assumption to make.

f so, why aren't you taking that up with the OpenSSL team?

I think the existence of this project should give an indication. At this stage, there's a fairly wide consensus around the security community that OpenSSL is awful software, and the only real reason it is still being recommended is because it's what has been tested in the real world for so long. Not because it is of high quality or well-maintained.

[paragonie-scott]

@bnoordhuis:

Here's a reason not to switch: OpenSSL's PRNG is a known quantity, the strength of platform-specific PRNGs is not. If the next Windows or FreeBSD release has a flawed PRNG, that will compromise node.js.

This is a common argument that people make, but it's ultimately invalid.

Even if you avoid depending on the operating system's PRNG, the rest of your system definitely depends on it for security. Node.js will be compromised regardless of what Node.js does.

[indutny]

Just a bit of FYI for everyone here:

http://lwn.net/Articles/633805/rss

"FreeBSD random number generator broken for last 4 months"

[ChALkeR]

@indutny Note that it also affects keys generated by Node.js on FreeBSD (using crypto.randomBytes()):

This includes, but not limited to, ssh keys and keys generated by openssl.

[ircmaxell]

@bnoordhuis

Here's a reason not to switch: OpenSSL's PRNG is a known quantity, the strength of platform-specific PRNGs is not. If the next Windows or FreeBSD release has a flawed PRNG, that will compromise node.js.

Actually, if Windows's CSPRNG is compromised, then Node.JS will be compromised. This is because OpenSSL relies on the system CSPRNG to seed it. There is no other source of high-quality random data in the system.

This means that the OpenSSL CSPRNG can by definition not be any stronger than the system's CSPRNG. However, it can be weaker (as has been seen several times).

The reason @paragonie-scott and others (including myself) are anti-userspace-csprng is that they provide no possible security gain, but introduce several security risks (by increasing attack surface area, by increasing bug surface area, etc).

There really is no benefit to _not_ switching (other than not making a change at all). However, as has been demonstrated in this thread already, there are several advantages to switching.

My suggestion would be to switch to the kernel-space CSPRNG.

[indutny]

@ChALkeR this was a reply to:

FreeBSD are not just going to push broken code, especially when they have a perfectly functional CSPRNG already

[ChALkeR]

@indutny yes, this assumption by @alfiepates is incorrect:

FreeBSD are not just going to push broken code, especially when they have a perfectly functional CSPRNG already, and especially as they have a security team of their own who would definitely catch any changes to the CSPRNG which would lead to a vulnerability in the OS.

Everyone makes mistakes, you can't say «{*} are not just going to push broken code» or «this lib is magical, shining, and will never be broken».

But what should be actually considered here is the fact that OpenSSL PRNG depends on system PRNG. So, it seems like under no circuimstances you could trust OpenSSL PRNG more than system PRNG, which makes «we can't trust system PRNGs, so let's use OpenSSL» argument invalid.

[alfiepates]

@indutny

I will retract some of my comment, it was far too absolute, but I don't believe it's wrong.

It's disingenuous to link this article without stating the fact that this occured on the FreeBSD -CURRENT branch, as opposed to the -STABLE branch. The -CURRENT branch is the bleeding-edge branch, and therefore bugs do happen, and in this case the bug was caught before the it made it to -STABLE.

Besides, I feel you're missing the point of this discussion. OpenSSL's CSPRNG is a security risk. EDIT: Need to pay more attention when tired.

[indutny]

@ChALkeR of course, I'm not saying that this is a counter-argument. Just wanted to make sure that everyone on the same page and does not trust OS vendors blindly.

[indutny]

@alfiepates hey, I'm not missing the point here! :wink: In fact, I'm almost convinced that this should happen. See https://github.com/nodejs/node/issues/5798#issuecomment-198833767

[alfiepates]

@indutny Ahh, wonderful. I'll retract that part of my comment too :P (Please forgive me, I'm one cup of coffee behind right now)

[indutny]

There is however a risk of incorrect implementation on our side, when we will write that new code to support OS-level PRNG. Just some food for thoughts.

[ChALkeR]

@indutny True. There is also a blocking-vs-nonblocking argument that would need to be discussed when implementing this (i.e. /dev/random vs /dev/urandom). I don't have an opinion on that yet.

[paragonie-scott]

@indutny Sure. I'm almost certain that the rough draft will be incorrect in some way. That's why peer review matters.

For what it's worth, I did help with PHP's implementation and I work for a company that audits crypto code. There are others in this thread that are often more perceptive than I am.

So as long as we participated it's highly likely that any implementation bugs will be spotted and rectified.

[paragonie-scott]

@ChALkeR: https://blog.cr.yp.to/20140205-entropy.html

Is there any serious argument that adding new entropy all the time is a good thing? The Linux /dev/urandom manual page claims that without new entropy the user is "theoretically vulnerable to a cryptographic attack", but (as I've mentioned in various venues) this is a ludicrous argument—how can anyone simultaneously believe that

• we can't figure out how to deterministically expand one 256-bit secret into an endless stream of unpredictable keys (this is what we need from urandom), but
• we can figure out how to use a single key to safely encrypt many messages (this is what we need from SSL, PGP, etc.)?

There are also people asserting that it's important for RNGs to provide "prediction resistance" against attackers who, once upon a time, saw the entire RNG state. But if the attacker sees the RNG state that was used to generate your long-term SSL keys, long-term PGP keys, etc., then what exactly are we gaining by coming up with unpredictable random numbers in the future?

Also http://www.2uo.de/myths-about-urandom/

/dev/urandom is what you want here.

[ChALkeR]

It looks like /dev/random is too slow on some systems, so using it would be a semver-major change (and perhaps even a regression). Using /dev/urandom wouldn't be a semver-major change.

[ojss]

Not sure about windows and linux but OSX and FreeBSD use Yarrow, I think FreeBSD switched to Fortuna. Both the algorithms are made by the same person and are quite reliable.
Better than user space generators

interesting link:
Linux random number generator

[MylesBorins]

Chiming in from the rafters... would there be a reason this would not make sense to implement in LibUV and be a downstream consumer to their implementation?

[paragonie-scott]

would there be a reason this would not make sense to implement in LibUV and be a downstream consumer to their implementation?

If you can guarantee rapid Node.js adoption of this (apparently new?) API in LibUV, this would probably be _better_, as there may be other applications that depend on it.

[bnoordhuis]

This means that the OpenSSL CSPRNG can by definition not be any stronger than the system's CSPRNG.

That's not true. /dev/urandom is not the only source of entropy, especially when you have egd installed. Even when you don't, there's still a few extra bits of entropy gained by mixing in the results from getpid(), gettimeofday(), etc.

Also, and I'm repeating myself for the third time now, if you all feel OpenSSL's PRNG is so horribly broken, then _why are you not taking that up with the OpenSSL project_? If OpenSSL switches, we switch - what's so difficult to understand here?

[alfiepates]

Also, and I'm repeating myself for the third time now, if you all feel OpenSSL's PRNG is so horribly broken, then why are you not taking that up with the OpenSSL project?

That's already been addressed. OpenSSL is a generally mediocre piece of software that's only really stuck around because we're already all using it and it's slightly less awful than rolling our own code in most cases... it's not a fantastic codebase at all and where we already have great CSPRNG (/dev/urandom) there's no point using code that has caused security issues in the past.

We can sit around poking OpenSSL until they do something (which is not going to happen in a reasonable timeframe) or we can say "hey, here's an issue" and fix what we have the power to fix.

[bnoordhuis]

So what you're suggesting is that, contrary to decades of good software engineering practices, we fix problems at the leafs instead of the root? I think you can guess from my tone how I feel about that.

[paragonie-scott]

Also, and I'm repeating myself for the third time now, if you all feel OpenSSL's PRNG is so horribly broken, then why are you not taking that up with the OpenSSL project?

Okay, here you go: https://github.com/openssl/openssl/issues/898

They can read this thread for context.

[bnoordhuis]

It's considered good form to give a summary in the bug report but thanks, I appreciate you took the time to open one.

[paragonie-scott]

It's considered good form to give a summary in the bug report

Rest assured, they're _well_ aware of the deficits in their userspace RNG. It's criticized quite frequently. I anticipate it will be swiftly closed as WONTFIX.

[joepie91]

there's still a few extra bits of entropy gained by mixing in the results from getpid(), gettimeofday(), etc.

Neither of those seem particularly random or unpredictable to me?

So what you're suggesting is that, contrary to decades of good software engineering practices, we fix problems at the leafs instead of the root?

Those same decades of good software engineering practices recognize that if the root is unwilling to fix, you don't leave the problem lingering around, but go fix it at the leafs instead.

[Knighton910]

+1

[co60ca]

There is no compelling argument against using kernel sources since OpenSSL is already seeding from it. As per: https://github.com/nodejs/node/issues/5798#issuecomment-198932579 +1

[MylesBorins]

I am particularly enjoying the idea that this issue is bubbling all the way down to making (u) random faster in the Linux kernel

[ircmaxell]

@bnoordhuis

That's not true. /dev/urandom is not the only source of entropy, especially when you have egd installed. Even when you don't, there's still a few extra bits of entropy gained by mixing in the results from getpid(), gettimeofday(), etc.

First off, getpid() and gettimeofday() do not provide sufficient randomness for CS purposes. And they only provide one-time entropy for seeding purposes (and like 4 bits of entropy at that).

As far as EGD, that gets its entropy from the kernel. And yes, /dev/urandom is not the only device to access kernel managed entropy. But the fact that other methods exist does not mean that relying on a user-space CSPRNG is a good thing. Especially when you consider that a compromise of the kernel is by definition a compromise of the child processes.

So while there are benefits to kernel space CSPRNG (no userland compromise will leak state), there are no benefits of user space CSPRNG (since any compromise it would defend against by definition it would be exposed to, therefore it can't defend against them).

If OpenSSL switches, we switch - what's so difficult to understand here?

If that's the position of the project, that's your prerogative (all we can do is recommend and try to show you why we believe that stance is wrong). However, based on this thread, it seems like that's not really the position of the project but just your personal view (unless I'm missing something).

[ChALkeR]

Ok, so, one of the possible drawbacks that has been mentioned in https://github.com/openssl/openssl/issues/898#issuecomment-199355257 is /dev/urandom being about 50x slower than AES-128-CTR_DRBG (numbers mentioned there are ~160 Mbps and ~8 Gbps). Not sure now how does it perform compared to whatever OpenSSL uses internally, though.

Is that critical for us? If yes, how would this proposal deal with it?

[paragonie-scott]

Is that critical for us?

No.

how would this proposal deal with it?

If OpenSSL 1.1 (or later) adopts a RAND_sys_bytes() function as described in the other thread, use that instead of RAND_bytes() and call this closed.

[ChALkeR]

Update: actually, crypto.randomBytes() gives 175 Mbps on my PC, while /dev/urandom gives 157 Mbps.

Those are pretty close, so _perhaps_ switching to /dev/urandom internally won't have a significant performance impact. This has to be tested when implemented, though, and on various platforms. If we care for crypto.randomBytes() throughput, that is.

[paragonie-scott]

If we care for crypto.randomBytes() throughput, that is.

Within reason, you should. If you only need to grab 32 bytes in under a millisecond, an upper limit on bandwidth measured in MB/s vs GB/s isn't likely to make a difference. Conversely, a difference that causes an app to take an extra 0.5s per execution would be a dealbreaker for most people.

[trevnorris]

I anticipate it will be swiftly closed as WONTFIX.

You must feel silly.

Update: actually, crypto.randomBytes() gives 175 Mbps on my PC, while /dev/urandom gives 157 Mbps.

Can you share how you performed this test? e.g. dd vs node. As I note below, my throughput numbers are very different.

If you only need to grab 32 bytes in under a millisecond, an upper limit on bandwidth measured in MB/s vs GB/s isn't likely to make a difference. Conversely, a difference that causes an app to take an extra 0.5s per execution would be a dealbreaker for most people.

If you need a one-off then grabbing 32 bytes every now and again is fine. My machine can do that in a matter of microseconds using randomBytes(), but as soon as I ask it to grab 512MB of consecutive 32 byte performance drops to over 4ms for every 32 byte packet. I've profiled it to make sure node wasn't the cause and sha1_block_data_order_ssse3 was eating up 25% of performance. Haven't taken the time to dig deep and fully assess where every tick is being spent.

Point here is that throughput is important, and that getting consistent high throughput is more complex than it appears. Relying solely on /dev/urandom would cause a huge performance regression on my machine. Currently I can do ~300 Mbps using randomBytes() gathering 1 Mb chunks, but running:

$ time sudo < /dev/urandom dd bs=1048576 count=10 > /dev/null

and I get a mere 96 Mbps (yes I attempted variations and this was the max throughput I could get).

So can we provide a solid argument for switching implementations if the performance impact would be this great (as in can anyone empirically prove that security will improve with the new implementation, or is it mixed hypothetical at this point)? Or should this discussion be tabled until /dev/urandom performance has been improved?

---

Here's the quick benchmark I threw together that has several variations:

'use strict';

const crypto = require('crypto');
const print = process._rawDebug;
const SIZE = 1024 * 1024 * 512;

var t = process.hrtime();
crypto.randomBytes(SIZE);
printTime();

//const CHUNK = 32;
//var t = process.hrtime();
//(function runner(n) {
  //if ((n + CHUNK) >= SIZE)
    //return printTime();
  //crypto.randomBytes(CHUNK, () => runner(n + CHUNK));
//}(0));

//var t = process.hrtime();
//for (var i = 0; i < 16777216; i++) {
  //crypto.randomBytes(32);
//}
//t = process.hrtime(t);
//print(((t[0] * 1e3 + t[1] / 16777216) / 1e4).toFixed(3) + ' ms/32 bytes');

function printTime() {
  t = process.hrtime(t);
  const mbps = (SIZE / 1024 / 1024) * 8 / (t[0] + t[1] / 1e9);
  print(mbps + ' Mbps');
}

[ChALkeR]

@trevnorris

1. Just use dd if=/dev/urandom of=/dev/null status=progress. No sudo, no time. It prints the results in MB/s, also bytes and seconds.
2. Your test shows 155 Mbps here. I used a similar one, but callback-based, 100 chunks, each 1 MiB in size, directly in repl:

js var start = process.hrtime(); var next = (i) => crypto.randomBytes(1024 * 1024, () => { if (i <= 0) { var end = process.hrtime(start); console.log(8 * 100 / (end[0] + end[1] * 1e-9)); } else { next(i - 1); } } ); next(100);

[alfiepates]

_(Note: A previous partial version of this was sent out by email, please ignore that. Fuckin' GitHub.)_

Hey, I'm gonna chime in for the last time on this thread:

I'm not going to name any names, but the tone of discourse in this thread (and in some cases externally, I've seen some... interesting tweets) risks becoming less than civil. While I'm not trying to derail the conversation towards "how2discuss", I think it's important we keep in mind that we (in my opinion) should have the same common goal: ensuring node is as secure as possible.

Security is in the fairly unique position that compromises are rarely acceptable; While mediocre UI decisions will just lead to a bad user experience, or scrappy optimisation can cause a process to run longer or peg some cores, bad security leads to _real life risk_. To be frank: Shit security gets people killed. Node.js is deployed in situations where security is important, and therefore Node.js developers have a responsibility to ensure that security is up to scratch.

This isn't about egos, this isn't about "I don't like how you're doing XYZ", this isn't about any of that. This is about risks that come with real-life consequences. There are people commenting in this thread who do security as their primary full-time employment, and who are damn good at it. I'd encourage all of you to look into the backgrounds of those in this thread making suggestions, not as an excuse to discredit their suggestions, but to put a human being behind these words.

I'm gonna duck out of this thread since I don't have all too much to add implementation-wise, but I am more than willing (at a later date) to talk about how to make sure these discussions stay on track in the future.

I appreciate what all of you are doing.

[bnoordhuis]

Update: actually, crypto.randomBytes() gives 175 Mbps on my PC, while /dev/urandom gives 157 Mbps.

@ChALkeR Try it with 3 or 4 instances running concurrently. A big issue with /dev/urandom is that it's a shared resource; it can produce output at a certain rate but that rate gets divided by the number of consumers.

Also, /dev/urandom has seen some scalability improvements over the years but we support kernels all the way back to 2.6.18.

[trevnorris]

@ChALkeR

Just use dd if=/dev/urandom of=/dev/null status=progress

Unfortunately status=progress isn't available. What kernel/dd version you running?

Your test shows 155 Mbps here. I used a similar one, but callback-based, 100 chunks, each 1 MiB in size

I'm not doubting you do, but I'm also letting you know that on my box I'm able to generate at ~300-400 Mbps. Demonstrating it'll be more complex to generate reliable throughput benchmarks, and that we may run the risk of making it slow enough that it won't be used.

When users read blog posts stating Math.random()'s PRNG has been fixed they may instead start doing things like the following:

const buf = Buffer(SIZE);
var idx = 0;
while ((idx = buf.writeUInt32LE(Math.random() * (-1 >>> 0), idx)) < SIZE);

Which, by the way, generates data at 1300 Mbps.

[trevnorris]

Ironically the above while loop tests well on randomness. Using dieharder on a 512MB file generated from the while loop above, and from output of /dev/urandom. Following is a table with both results. The first set is from the while loop above. The second is from /dev/urandom.

#=============================================================================#
#            dieharder version 3.31.1 Copyright 2003 Robert G. Brown          #
#=============================================================================#
   rng_name         ||     filename                                  ||           filename
 file_input_raw     ||      /tmp/random_data.bin                     ||            /tmp/urandom_data.bin
#==============================================================================================================================#
        test_name   ||ntup| tsamples |psamples|  p-value |Assessment ||ntup| tsamples |psamples|  p-value |Assessment
#==============================================================================================================================#
     dab_bytedistrib||   0|  51200000|       1|0.09731279|  PASSED   ||   0|  51200000|       1|0.19394655|  PASSED
             dab_dct|| 256|     50000|       1|0.76785422|  PASSED   || 256|     50000|       1|0.70144164|  PASSED
       dab_filltree2||   0|   5000000|       1|0.21736330|  PASSED   ||   0|   5000000|       1|0.58799138|  PASSED
       dab_filltree2||   1|   5000000|       1|0.94496905|  PASSED   ||   1|   5000000|       1|0.97227096|  PASSED
        dab_filltree||  32|  15000000|       1|0.49546955|  PASSED   ||  32|  15000000|       1|0.62787597|  PASSED
        dab_filltree||  32|  15000000|       1|0.91901131|  PASSED   ||  32|  15000000|       1|0.80891168|  PASSED
        dab_monobit2||  12|  65000000|       1|0.69102119|  PASSED   ||  12|  65000000|       1|0.05164031|  PASSED
    diehard_2dsphere||   2|      8000|     100|0.45060252|  PASSED   ||   2|      8000|     100|0.45652347|  PASSED
    diehard_3dsphere||   3|      4000|     100|0.27961199|  PASSED   ||   3|      4000|     100|0.49351845|  PASSED
   diehard_birthdays||   0|       100|     100|0.48110599|  PASSED   ||   0|       100|     100|0.19910323|  PASSED
   diehard_bitstream||   0|   2097152|     100|0.99536763|   WEAK    ||   0|   2097152|     100|0.79115717|  PASSED
diehard_count_1s_byt||   0|    256000|     100|0.20484695|  PASSED   ||   0|    256000|     100|0.78195702|  PASSED
diehard_count_1s_str||   0|    256000|     100|0.75812248|  PASSED   ||   0|    256000|     100|0.54848822|  PASSED
       diehard_craps||   0|    200000|     100|0.23976205|  PASSED   ||   0|    200000|     100|0.24913827|  PASSED
       diehard_craps||   0|    200000|     100|0.97521323|  PASSED   ||   0|    200000|     100|0.99211351|  PASSED
      diehard_operm5||   0|   1000000|     100|0.75459685|  PASSED   ||   0|   1000000|     100|0.87415332|  PASSED
 diehard_parking_lot||   0|     12000|     100|0.72516127|  PASSED   ||   0|     12000|     100|0.51744758|  PASSED
  diehard_rank_32x32||   0|     40000|     100|0.04089185|  PASSED   ||   0|     40000|     100|0.55049357|  PASSED
    diehard_rank_6x8||   0|    100000|     100|0.76009470|  PASSED   ||   0|    100000|     100|0.86409689|  PASSED
        diehard_runs||   0|    100000|     100|0.83761343|  PASSED   ||   0|    100000|     100|0.08393408|  PASSED
        diehard_runs||   0|    100000|     100|0.96494601|  PASSED   ||   0|    100000|     100|0.79795717|  PASSED
     diehard_squeeze||   0|    100000|     100|0.21902504|  PASSED   ||   0|    100000|     100|0.45034347|  PASSED
 marsaglia_tsang_gcd||   0|  10000000|     100|0.00061302|   WEAK    ||   0|  10000000|     100|0.00000000|  FAILED
 marsaglia_tsang_gcd||   0|  10000000|     100|0.74541958|  PASSED   ||   0|  10000000|     100|0.91843104|  PASSED
     rgb_kstest_test||   0|     10000|    1000|0.55923481|  PASSED   ||   0|     10000|    1000|0.01354922|  PASSED
      rgb_lagged_sum||   0|   1000000|     100|0.36751921|  PASSED   ||   0|   1000000|     100|0.93757630|  PASSED
rgb_minimum_distance||   0|     10000|    1000|0.00000000|  FAILED   ||   0|     10000|    1000|0.00000000|  FAILED
    rgb_permutations||   5|    100000|     100|0.30443228|  PASSED   ||   5|    100000|     100|0.64178140|  PASSED
         sts_monobit||   1|    100000|     100|0.85916197|  PASSED   ||   1|    100000|     100|0.98321093|  PASSED
            sts_runs||   2|    100000|     100|0.96656242|  PASSED   ||   2|    100000|     100|0.37000645|  PASSED
          sts_serial||  10|    100000|     100|0.17108872|  PASSED   ||  10|    100000|     100|0.78291929|  PASSED
          sts_serial||  10|    100000|     100|0.43352488|  PASSED   ||  10|    100000|     100|0.80215761|  PASSED
          sts_serial||   1|    100000|     100|0.85916197|  PASSED   ||   1|    100000|     100|0.98321093|  PASSED
          sts_serial||  11|    100000|     100|0.56250044|  PASSED   ||  11|    100000|     100|0.46751929|  PASSED
          sts_serial||  11|    100000|     100|0.96681711|  PASSED   ||  11|    100000|     100|0.72513734|  PASSED
          sts_serial||  12|    100000|     100|0.80290859|  PASSED   ||  12|    100000|     100|0.71128922|  PASSED
          sts_serial||  12|    100000|     100|0.97395930|  PASSED   ||  12|    100000|     100|0.84596100|  PASSED
          sts_serial||  13|    100000|     100|0.05460794|  PASSED   ||  13|    100000|     100|0.61265757|  PASSED
          sts_serial||  13|    100000|     100|0.06000085|  PASSED   ||  13|    100000|     100|0.68475462|  PASSED
          sts_serial||  14|    100000|     100|0.24213451|  PASSED   ||  14|    100000|     100|0.22612266|  PASSED
          sts_serial||  14|    100000|     100|0.43303295|  PASSED   ||  14|    100000|     100|0.34819106|  PASSED
          sts_serial||  15|    100000|     100|0.70772349|  PASSED   ||  15|    100000|     100|0.38347196|  PASSED
          sts_serial||  15|    100000|     100|0.94036403|  PASSED   ||  15|    100000|     100|0.79931573|  PASSED
          sts_serial||  16|    100000|     100|0.18413544|  PASSED   ||  16|    100000|     100|0.21397317|  PASSED
          sts_serial||  16|    100000|     100|0.51409929|  PASSED   ||  16|    100000|     100|0.28934397|  PASSED
          sts_serial||   2|    100000|     100|0.86549774|  PASSED   ||   2|    100000|     100|0.34830172|  PASSED
          sts_serial||   3|    100000|     100|0.61116626|  PASSED   ||   3|    100000|     100|0.55936667|  PASSED
          sts_serial||   3|    100000|     100|0.81261243|  PASSED   ||   3|    100000|     100|0.91339662|  PASSED
          sts_serial||   4|    100000|     100|0.04777919|  PASSED   ||   4|    100000|     100|0.10701052|  PASSED
          sts_serial||   4|    100000|     100|0.67123232|  PASSED   ||   4|    100000|     100|0.14444634|  PASSED
          sts_serial||   5|    100000|     100|0.17241503|  PASSED   ||   5|    100000|     100|0.03073699|  PASSED
          sts_serial||   5|    100000|     100|0.73980051|  PASSED   ||   5|    100000|     100|0.86069674|  PASSED
          sts_serial||   6|    100000|     100|0.61426714|  PASSED   ||   6|    100000|     100|0.73662496|  PASSED
          sts_serial||   6|    100000|     100|0.61541617|  PASSED   ||   6|    100000|     100|0.74861872|  PASSED
          sts_serial||   7|    100000|     100|0.37869368|  PASSED   ||   7|    100000|     100|0.18601122|  PASSED
          sts_serial||   7|    100000|     100|0.47181049|  PASSED   ||   7|    100000|     100|0.86052350|  PASSED
          sts_serial||   8|    100000|     100|0.00960323|  PASSED   ||   8|    100000|     100|0.55658027|  PASSED
          sts_serial||   8|    100000|     100|0.34149403|  PASSED   ||   8|    100000|     100|0.99697626|   WEAK
          sts_serial||   9|    100000|     100|0.03105746|  PASSED   ||   9|    100000|     100|0.04533251|  PASSED
          sts_serial||   9|    100000|     100|0.93545391|  PASSED   ||   9|    100000|     100|0.12295836|  PASSED

Even tested well on cacert.org's rng results (search for "v8 4.9.385 Math.random()"). Maybe we should just switch to that. :-P

[ChALkeR]

@trevnorris That would be just another userspace PRNG which would be an extra signle point of failure on top of the system PRNG…

I have not analyzed your dieharder results yet.

dd --versions says «dd (coreutils) 8.25». If you don't have status=progress, just launch watch killall -USR1 dd in a separate tab — it will send USR1 signals to dd every 2 seconds, and dd prints progress when receives an USR1 signal.

[trevnorris]

@ChALkeR I'm using 8.23, and nice trick.

That would be just another userspace PRNG which would be an extra signle point of failure on top of the system PRNG…

Sorry. :-P was meant to imply /s or :trollface: and that I was surprised Math.random() would have scored such high marks on a rigorous randomness analysis test. And to suggest people may see results like this and feel justified choosing the above while loop over randomBytes() because our implementation is so much slower.

[joepie91]

It's not uncommon for non-CS PRNGs to score well on randomness analysis.
That doesn't make them suitable for crypto however - for that, they don't
just have to be indistinguishable from randomness, but also unpredictable
and not subvertible.
On Mar 23, 2016 05:50, "Trevor Norris" [email protected] wrote:

@ChALkeR https://github.com/ChALkeR I'm using 8.23, and nice trick.

That would be just another userspace PRNG which would be an extra signle
point of failure on top of the system PRNG…

Sorry. :-P was meant to imply /s or [image: :trollface:] and that I was
surprised Math.random() would have scored such high marks on a rigorous
randomness analysis test. And to suggest people may see results like this
and feel justified choosing the above while loop over randomBytes()
because our implementation is so much slower.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/issues/5798#issuecomment-200192891

[paragonie-scott]

Humor is good, but unless the calendar reads April 1, please let's keep the discussion of "let's use Math.random() for crypto" to a minimum.

[trevnorris]

Humor is good, but unless the calendar reads April 1, please let's keep the discussion of "let's use Math.random() for crypto" to a minimum.

Loosen up. There was no serious conversation about using it in core. My point is that if our implementation is so slow it's unusable then users will start resorting to methods like this. That I'm serious about, and history has proven users will take this type of action if our API doesn't work as needed and there's no alternative. Don't rebuttal that it's on them, because it's our responsibility to make sure our API is acceptable.

It's not uncommon for non-CS PRNGs to score well on randomness analysis.That doesn't make them suitable for crypto however - for that, they don't just have to be indistinguishable from randomness, but also unpredictable and not subvertible.

Then please share how randomness can be properly analyzed to see if it's suitable for cryptographic use.

[technion]

It's worth referring to LibreSSL's rewrite of OpenSSL's functionality. The first thing their getentropy() function tries (under Linux) is to pull from /dev/urandom:

https://github.com/libressl-portable/openbsd/blob/37759485f52993eb812898ccd70fc42cd92cdad5/src/lib/libcrypto/crypto/getentropy_linux.c#L114

[paragonie-scott]

Apparently OpenSSL bit Android too.

[joepie91]

Looks like a new paper was released regarding OpenSSL's PRNG, a few days ago:

In this work we demonstrate various weaknesses of the random number generator (RNG) in the OpenSSL cryptographic library. We show how OpenSSL's RNG, knowingly in a low entropy state, potentially leaks low entropy secrets in its output, which were never intentionally fed to the RNG by client code, thus posing vulnerabilities even when in the given usage scenario the low entropy state is respected by the client application. Turning to the core cryptographic functionality of the RNG, we show how OpenSSL's functionality for adding entropy to the RNG state fails to be effectively a mixing function. [...]

[bnoordhuis]

@joepie91 I've scanned through the paper. The main premise seems to be that starting from a low entropy state reduces the efficacy of the PRNG? That seems self-evidently true. Node.js calls RAND_poll() at start-up so I think we're immune from that (provided system entropy is reliable.)

I don't really understand his other finding on reducing the entropy from 256 to 240 bits. It relies on the attacker being able to observe the PRNG's intermediate states?

EDIT: Or is the issue that RAND_bytes() uses the pre-existing contents of the output buffer as input for the entropy pool? We disable that in our builds.

[paragonie-scott]

I see this didn't make into Node 6. Maybe we could push for a true CSPRNG in Node 7?

[ChALkeR]

@paragonie-scott

Btw, this comment by @trevnorris:

My point is that if our implementation is so slow it's unusable then users will start resorting to methods like this.

is similar to reasons of using /dev/urandom instead of /dev/random, see http://www.2uo.de/myths-about-urandom/ «What's wrong with blocking?».

The reasons not to make it slower here are pretty similar. While 20% difference would likely be acceptable, making it several times slower would probably be viewed as a regression, and users would resort to _various hacks_.

[ChALkeR]

@paragonie-scott

I see this didn't make into Node 6. Maybe we could push for a true CSPRNG in Node 7?

I don't think platform detection here for randomness source will be agreed on, so this should probably be first pushed up into one of the deps that handles platform abstraction for similar things — e.g. OpenSSL or libuv.

[trevnorris]

I'm still waiting on a response of how to determine if data is "suitable for crypto". Since passing marks on randomness tests apparently aren't good enough.

[alfiepates]

@trevnorris The NIST has published a paper on this very topic, and maintains a somewhat easy-to-digest webpage covering CSRNG.

But, in my opinion, this should not concern you, since (sensibly implemented) system randomness is designed specifically to abstract this away.

However yes, I would somewhat agree with @ChALkeR in that the platform abstraction should probably be handled by a separate dependency, but that's also something I'm not entirely qualified to comment on so this is where my input ceases.

[trevnorris]

@alfiepates Thanks for the links. The point is that I'm genuinely curious how Math.random() scores. Nothing actually important. :)

[indutny]

@trevnorris Math.random() is absolutely predictable after observing several values.

[trevnorris]

@indutny sure, i'm just interested to see that for myself after it scored so high on the dieharder tests.

[ircmaxell]

There is no test that will prove if a given function is random. There are only tests that prove it is not random.

Meaning, no test will tell you if you are OK. That's important to keep in mind when talking about crypto and random numbers (specifically CSPRNG).

[joepie91]

@trevnorris It's important to realize that a CSPRNG shouldn't just be resistant to analysis, but also to targeted attempts at manipulation (eg. by 'poisoning' source entropy). No randomness test will show this. Determining whether random data is 'good enough' is largely a matter of theory.

@bnoordhuis I haven't actually read the paper yet due to time constraints, rather I just figured I'd post it in this thread for its relevance.

[carlos8f]

Just a thought, if someone wanted to be malicious and had publish access to someone's dependency, they could quietly monkey patch:

require('crypto').randomBytes = () => Buffer('pawnd')

and ruin everyone's crypto in one line. maybe there should be a protection against this?

[joepie91]

@carlos8f Hmm. If that works, that'd work with _any_ dependency, and you wouldn't even need to compromise a third-party one (due to the require cache).

[indutny]

@carlos8f what about running require('child_process').spawn from dependency? Seems to be quite a serious security compromise too. IMO, there is certain trust that people put into dependencies that they install, this is not something that core is responsible for.

[carlos8f]

@indutny true, but that would be more noticeable of an attack. monkey patching core crypto module shouldn't be allowed imo. at the very least a warning should spit out to the console.

[indutny]

@carlos8f well, having access to require('fs') is enough to patch node.js binary...

[carlos8f]

@indutny yep, I tried to release a crypto node program the other day and just got laughed at. no one takes node security seriously, especially coming from the security community.

PS i'm trying to revive the conversation on package signing, but seeing crap like this i quietly walk away....

[indutny]

@carlos8f It's not just about node, it is about java, go, python, ruby and almost anything that has a package managers. Any code that wasn't written by company's members should be considered as insecure, unless proven otherwise.

[carlos8f]

A safer bet would be to adopt what PHP 7's random_bytes() does:

LOL that we're in the year 2016, and PHP is what we're aspiring to be like. And at least in PHP, you can't randomly redefine mission-critical parts of the system like random_bytes() without getting a proper exception in production. node _could_ have better security, but it choses not to, because security is always someone else's problem. sadly companies don't audit their code, but that's why we hackers stay in business muahaha.

[bnoordhuis]

monkey patching core crypto module shouldn't be allowed imo.

There are legitimate use cases, like instrumentation and performance monitoring.

I tried to release a crypto node program the other day and just got laughed at. no one takes node security seriously, especially coming from the security community.

I wouldn't call /r/crypto a security community. Maybe "a bunch of of idiots that know just enough to be dangerous."

In fairness, that applies to most technology-related subreddits with more than a few hundred subscribers. The larger they get, the more they regress to the mean.

[trevnorris]

No randomness test will show this. Determining whether random data is 'good enough' is largely a matter of theory.

This nags at me. Having a proper random generator is _so_ important, yet I've not yet been able to find a way to solidly empirically test an implementation. Which then makes the "good enough" line subjective and never ending (e.g. this thread).

[llafuente]

monkey patching core crypto module shouldn't be allowed imo.

There are legitimate use cases, like instrumentation and performance monitoring.

Anyone can freeze that object, so like @bnoordhuis I consider this a job for the user.

I think node could have something to freeze all it's security-concern-objects/modules/*, and prevent unwanted modifications...
process.freeze()?

[alfiepates]

@trevnorris

This nags at me. Having a proper random generator is so important, yet I've not yet been able to find a way to solidly empirically test an implementation. Which then makes the "good enough" line subjective and never ending (e.g. this thread).

Which is why I linked the NIST paper. It's quite hard to tell if a set of numbers is random if all you have is that set of numbers, but if you give the method by which the set of numbers is generated, you _can_ start to decide if you're getting randomness or not.

Sure, there's no "magic randomness test box", but the NIST paper does define a set of rules to test CSPRNG implementations.

[joepie91]

@trevnorris: This nags at me. Having a proper random generator is so important, yet I've not yet been able to find a way to solidly empirically test an implementation. Which then makes the "good enough" line subjective and never ending (e.g. this thread).

Aside from what @alfiepates said, this is really not any different from security in general, however much it sucks (and yes, I agree that this is irritating). In real-world code, there are no proofs that code is secure, definitely not through empirical testing - you can only keep trying to find weaknesses, and fix them as soon as you find them, even if they seem small or insignificant. "Vulnerability scanners" are famously useless for this reason.

It's the same reason why for both CSPRNGs and other tooling, the general advice is "use the battle-tested thing that isn't known to be broken", as it is more likely to have approached 'perfect security' than the non-battle-tested options, assuming no explicitly contradictory information is available. It's all you can do, really, and it has to be done rigorously.

[joepie91]

@llafuente: Anyone can freeze that object, so like @bnoordhuis I consider this a job for the user.

How do you ensure that this happens _before_ anything else gets a chance to tamper with it, though? You'd have to expect the user to explicitly do this at the top level of their application code, and that is going to inevitably be forgotten.

I'd rather see it frozen by default (at least to the point of not allowing to modify _existing_ properties, since eg. promisifying might be necessary), with a runtime flag to allow certain files or modules to bypass this. That would cover instrumentation and performance monitoring, both of which are things that are typically explicitly enabled on runtime in a specific environment.

[bnoordhuis]

That's fake security. One obvious way to circumvent it is to spawn gdb or open /proc/self/mem and flip the protection bits. I'm sure you can think of more.

We're getting off-topic, though.

[azet]

FYI: similar discussion in Ruby - https://bugs.ruby-lang.org/issues/9569

Userspace RNGs are dangerous. Let me reiterate what a couple of people already stated: on Linux you want to seed from /dev/urandom! Ideally you'd go the way e.g. libsodium handles RNGs on different platforms, but I'm not going to dictate anything, reading this thread I wasn't sure if I should laugh or cry.

One core maintainer even suggested "adding entropy" via egd. Something I can only describe as "voodoo security". I actually had to Google egd, because I didn't remember:

A userspace substitute for /dev/random, written in perl.
[...]
egd-0.6 had a major security problem that caused it to only use a tiny fraction of the entropy it gathered. All users should upgrade immediately.

Then I remembered why it exists: a long time ago -- before single threaded frontend languages were hip to write security sensitive "microservices" in -- SunOS (i.e. Solaris) lacked a kernel supplied random number generator. I remember I couldn't compile OpenSSH on this thing because of it and had to resort to some hacked-up rng/entropy gathering daemon back then. I decided to use telnet(1) instead. Which in retrospect was a wise choice, because I would have only added security by obscurity. And didn't connect the machine to the Internet. I was ~14 back then and I liked SPARC a lot. There's one in Rexx as well if you still run OS/2 or a Mainframe to develop useless node dependencies: http://r6.ca/RexxEGD/

As several people have already told you: the RNG in OpenSSL isn't fork safe, since this language depends on forking for performance, it's rather unwise to use a Userspace RNG that has known weaknesses. The recent paper on RNG weaknesses in OpenSSL was linked to in the thread already, but again: https://eprint.iacr.org/2016/367.pdf as it seems @bnoordhuis didn't read beyond the abstract or just didn't grasp it's meaning. _Yes that'll affect node security_. I think it's been fixed (which doesn't mean this release won't be around for ages in legacy systems,..) - yet it demonstrates how a Userspace RNG can fail to provide proper security for a language as a whole. Suggesting user-land entropy gathering daemons along is just pure insanity. There're so many attack vectors. So you either believe the Kernel you're running on is working properly (otherwise why would you run language X on it anyway - remember Ken Thompson's Turing Award Lecture? If you develop on e.g. a garbage collected, dynamically interpreted language - the same holds true for the OS you're running on. If you compile, in this case via multiple steps, you don't only have to trust your compilers and linkers, you have to trust the operating system they're running on and it's libc as well [btw also the klibc]) and _may_ think about using additional security fallbacks like implemented by libsodium or you're relying on voodoo-security for your entire ecosystem.

To be honest: I don't really care about node.js and every time I need to install some service that depends on five different module/package managers and installs more than 200 dependencies I'm asking myself why I'm even bothering. But I know people run node in production, so it may be wise to think about security sensitive issues in the core language and library.

[bnoordhuis]

A userspace substitute for /dev/random, written in perl.

I think you are mistaking the program for the protocol.

the RNG in OpenSSL isn't fork safe, since this language depends on forking for performance

What on earth makes you say that? Please get your facts straight. The only time node forks, it's to call execve() immediately afterwards.

The recent paper on RNG weaknesses in OpenSSL was linked to in the thread already, but again: https://eprint.iacr.org/2016/367.pdf as it seems @bnoordhuis didn't read beyond the abstract or just didn't grasp it's meaning.

How about you explain it instead of making disparaging comments? Besides the tone of your post, the factual errors make me reluctant to take it on face value.

[azet]

I think you are mistaking the program for the protocol.

Which protocol? Are you referring to https://www.openssl.org/docs/manmaster/crypto/RAND_egd.html? If so, again; why would you want to add entropy from a userspace source? What exactly are the use-cases?

What on earth makes you say that? Please get your facts straight. The only time node forks, it's to call execve() immediately afterwards.

Well. It's not a traditional fork. child_process has the following methods:

• .fork():

It is important to keep in mind that spawned Node.js child processes are independent of the parent with exception of the IPC communication channel that is established between the two. Each process has it's own memory, with their own V8 instances. Because of the additional resource allocations required, spawning a large number of child Node.js processes is not recommended.

• .spawn(): similar to popen(3) - opens a bidirectional pipe to a subshell, correct? I'm aware how this works in libuv but am rather unsure if it's handled exactly the same in node.js; libuv uses threads and thread pools. mind: the OpenSSL PRNG is also not thread safe: https://wiki.openssl.org/index.php/Random_Numbers#Thread_Safety
• .send(): a developer may be incorrectly sharing state between childs via IPC.

These seem to be tracked via PIDs (are these internal to node.js or actual OS PIDs?). I'd assume the latter given a subshell is openend from another process, but I'm certainly not as familiar with node.js internals as you are. libuv uses OS PIDs if I remember correctly.

The documentation also contains a waitpid method.

Further down the line, I read a bit of nodejs core code:
https://github.com/nodejs/node/blob/master/src/node_crypto.cc#L234-265

// Ensure that OpenSSL has enough entropy (at least 256 bits) for its PRNG.
// The entropy pool starts out empty and needs to fill up before the PRNG
// can be used securely.  Once the pool is filled, it never dries up again;
// its contents is stirred and reused when necessary.
//
// OpenSSL normally fills the pool automatically but not when someone starts
// generating random numbers before the pool is full: in that case OpenSSL
// keeps lowering the entropy estimate to thwart attackers trying to guess
// the initial state of the PRNG.
//
// When that happens, we will have to wait until enough entropy is available.
// That should normally never take longer than a few milliseconds.
//
// OpenSSL draws from /dev/random and /dev/urandom.  While /dev/random may
// block pending "true" randomness, /dev/urandom is a CSPRNG that doesn't
// block under normal circumstances.
//
// The only time when /dev/urandom may conceivably block is right after boot,
// when the whole system is still low on entropy.  That's not something we can
// do anything about.
inline void CheckEntropy() {
  for (;;) {
    int status = RAND_status();
    CHECK_GE(status, 0);  // Cannot fail.
    if (status != 0)
      break;

    // Give up, RAND_poll() not supported.
    if (RAND_poll() == 0)
      break;
  }
}

If you remember the abstract of the paper referenced earlier, that exactly the issue at hand:

Abstract: In this work we demonstrate various weaknesses of the random number generator (RNG) in the OpenSSL cryptographic library. We show how OpenSSL's RNG, knowingly in a low entropy state, potentially leaks low entropy secrets in its output, which were never intentionally fed to the RNG by client code, thus posing vulnerabilities even when in the given usage scenario the low entropy state is respected by the client application. Turning to the core cryptographic functionality of the RNG, we show how OpenSSL's functionality for adding entropy to the RNG state fails to be effectively a mixing function. If an initial low entropy state of the RNG was falsely presumed to have 256 bits of entropy based on wrong entropy estimations, this causes attempts to recover from this state to succeed only in long term but to fail in short term. As a result, the entropy level of generated cryptographic keys can be limited to 80 bits, even though thousands of bits of entropy might have been fed to the RNG state previously. [...]

https://github.com/nodejs/node/blob/master/src/node_crypto.cc#L268-275:

bool EntropySource(unsigned char* buffer, size_t length) {
  // Ensure that OpenSSL's PRNG is properly seeded.
  CheckEntropy();
  // RAND_bytes() can return 0 to indicate that the entropy data is not truly
  // random. That's okay, it's still better than V8's stock source of entropy,
  // which is /dev/urandom on UNIX platforms and the current time on Windows.
  return RAND_bytes(buffer, length) != -1;
}

https://www.openssl.org/docs/manmaster/crypto/RAND_bytes.html says:

RAND_bytes() returns 1 on success, 0 otherwise. The error code can be obtained by ERR_get_error. RAND_pseudo_bytes() returns 1 if the bytes generated are cryptographically strong, 0 otherwise. Both functions return -1 if they are not supported by the current RAND method.

So I think the check should be return RAND_bytes(buffer, length) == 0; - /dev/urandom is certainly a better option than a broken OpenSSL PRNG, don't you think so? I don't know if node cares about Windows but RAND_poll is incredibly slow due to a heap walk bug: https://rt.openssl.org/Ticket/Display.html?id=2100&user=guest&pass=guest

Let me conclude saying that every cryptographer and security software engineer I've talked and most languages to prefer to seed from urandom directly. Possibly making use of safe and portable constructs like that of libsodium: https://github.com/jedisct1/libsodium/blob/master/src/libsodium/randombytes/sysrandom/randombytes_sysrandom.c

In future versions of the Linux Kernel /dev/urandom will feature a ChaCha20-based DRBG construct similar to that of OpenBSD's arc4random. But these patches are just a few weeks old and there are a couple of people working on further improvements. Anyway. It's safe to use and seed from, may I ask why you're specifically using OpenSSL's RNG? Even some of their maintainers told me "do not use it". It's RNG is also not general-purpose, it's intended to be used for SSL/TLS specific protocol internals.

Recommended reading material: https://emboss.github.io/blog/2013/08/21/openssl-prng-is-not-really-fork-safe/

[bnoordhuis]

Which protocol? Are you referring to https://www.openssl.org/docs/manmaster/crypto/RAND_egd.html? If so, again; why would you want to add entropy from a userspace source? What exactly are the use-cases?

EGD the protocol is implemented by several programs that collect or distribute entropy, e.g., low-entropy VMs that get their initial state from a hardware RNG on the network.

Well. It's not a traditional fork.

Indeed it is not. OpenSSL not being fork-safe is not relevant because node doesn't fork.

If you remember the abstract of the paper referenced earlier, that exactly the issue at hand:

See https://github.com/nodejs/node/issues/5798#issuecomment-209326548 - if you think my understanding of the paper and why I think node is unaffected is wrong, please explain so in detail.

/dev/urandom is certainly a better option than a broken OpenSSL PRNG, don't you think so?

Context is everything. The code you pasted seeds V8's Math.random() and the hash table seed. If you thought it had anything to do with the crypto module - it does not.

I assume you are aware of this but Math.random() is not cryptographically strong, it's just a simple PRNG. The hash seed is to mitigate hash table collision attacks, primarily for string key collisions in JS objects in dictionary mode.

If node didn't override the defaults, V8 will try to open /dev/urandom but happily continues with almost zero entropy if it fails - which, per the contract of Math.random(), is perfectly acceptable.

The situation on Windows was even worse at the time of writing - processes spawned within one second of each other would generate the same random numbers - and is still not great: it uses rand_s / RtlGenRandom, which does not claim to be strongly random. Again, perfectly acceptable but not exactly great.

Knowing what you know now, wouldn't you agree that OpenSSL's RNG is better?

Let me conclude saying that every cryptographer and security software engineer I've talked and most languages to prefer to seed from urandom directly.

Seeding from /dev/urandom (and /dev/random and /dev/srandom) is exactly what OpenSSL does. Why then do you consider it so broken?

In future versions of the Linux Kernel /dev/urandom will feature a ChaCha20-based DRBG construct similar to that of OpenBSD's arc4random.

I've commented on that before but to reiterate: that doesn't help while we still support 2.6.18 and 2.6.32. Performance regressions on older kernels are not acceptable.

may I ask why you're specifically using OpenSSL's RNG? Even some of their maintainers told me "do not use it".

I'd appreciate it if you could back up that claim with names and places. I've never heard any of the OpenSSL developers publicly state that.

[azet]

EGD the protocol is implemented by several programs that collect or distribute entropy, e.g., low-entropy VMs that get their initial state from a hardware RNG on the network.

Can you please add a reference to that statement? What are you exactly referring to?

Indeed it is not. OpenSSL not being fork-safe is not relevant because node doesn't fork.

Have you read further down? My point being node builds on libuv, libuv uses threads, OpenSSL is not thread safe. Also it's rather easy to produce a fork bomb with node using spawn() in detached mode.

Context is everything. The code you pasted seeds V8's Math.random() and the hash table seed. If you thought it had anything to do with the crypto module - it does not.

Context? The code I pasted has a comment above it saying it uses /dev/urandom why would it seed from Math.random()? That's insane given it's your core crypto interface. I'm referring to https://github.com/nodejs/node/blob/master/src/node_crypto.cc#L268-275 - if this is not your entropy source, where is it?

If node didn't override the defaults, V8 will try to open /dev/urandom but happily continues with almost zero entropy if it fails - which, per the contract of Math.random(), is perfectly acceptable.

I do not understand this sentence. And /dev/urandom does not block nor does "run out of entropy" in any sense that's relevant to security for nodejs.

Knowing what you know now, wouldn't you agree that OpenSSL's RNG is better?

No. Take a look at libsodium.

Seeding from /dev/urandom (and /dev/random and /dev/srandom) is exactly what OpenSSL does. Why then do you consider it so broken?

Because it does not stop there, it seeds from it then mixes in "entropy" from user-land that can easily be tampered with, hence producing predictable output. See the very last link in my previous comment. There're many other examples.

I've commented on that before but to reiterate: that doesn't help while we still support 2.6.18 and 2.6.32. Performance regressions on older kernels are not acceptable.

I absolutely agree there. These are patches being worked on right now, they won''t be around for a while. Can you explain which performance regressions you're experiencing on older kernels specifically?

I'd appreciate it if you could back up that claim with names and places. I've never heard any of the OpenSSL developers publicly state that.

I have not done so on purpose, I thought that was obvious. Why do you think both LibreSSL and BoringSSL (now powering all of Google, including desktop applications [e.g. Chrome] et cetera) have switched to other RNG designs?

[bnoordhuis]

OpenSSL is not thread safe

This is wrong.

Also it's rather easy to produce a fork bomb with node using spawn() in detached mode.

Irrelevant. Why do you even bring it up?

Context? The code I pasted has a comment above it saying it uses /dev/urandom why would it seed from Math.random()?

I think you should go back and read what I wrote again (hint: not 'from'.) Please put more effort in your replies, will you?

Because it does not stop there, it seeds from it then mixes in "entropy" from user-land that can easily be tampered with, hence producing predictable output.

Can you explain? If by 'tamper' you mean 'attach with ptrace', then predictable output is the least of your worries. If you mean 'exploitable program bugs', the kernel is no stranger to that either.

Can you explain which performance regressions you're experiencing on older kernels specifically?

Try reading from /dev/urandom from multiple processes simultaneously and you'll see what I mean. It's slow - very slow - and can have a dramatic impact on overall system performance, not just the readers.

I have not done so on purpose, I thought that was obvious.

I hope you understand I won't be able to just take your word for it.

[paragonie-scott]

This is getting nonproductive, fast.

I have a lot of work on my plate right now, but once it's done, expect a patch so we can stop discussing the theoretical realm.

The patch I'm envisioning will do exactly what libsodium 1.0.11 and newer (not yet released) does:

• Windows:
  
  
  
  • CryptGenRandom
  
  
• OpenBSD:
  
  
  
  • getentropy()
  
  
  • arc4random_buf()
  
  
• Linux:
  
  
  
  • If getrandom(2) is available, use that
  
  
  • Otherwise:
  
  
  • Poll /dev/random until it's available to be read. This tells us that urandom is seeded, and therefore safe to use with no regards for "entropy running out".
    
    The only reason I'm bothering with this is the nonzero chance of someone writing Node.js apps that can race against the Linux boot process. If this isn't a concern, I'll nix this step.
  
  
  • Read from /dev/urandom
  
  
• Other BSDs (also OS X):
  
  
  
  • Just read /dev/urandom, because it blocks until it's seeded (like Linux should but doesn't)
  
  

[ChALkeR]

@azet
Several corrections, to clear misunderstanding here:

1. Not OpenSSL is not thread-safe, but some parts of it (including the random number generator).
2. The fact that node uses libuv, that libuv has a thread pool, and that node uses OpenSSL RAND_bytes() for crypto.randomBytes() _does not imply_ that calls to crypto.randomBytes() are performed from the thread pool.

_Update:_ I have made a mistake here, see explanation below: https://github.com/nodejs/node/issues/5798#issuecomment-222373530.

[ChALkeR]

@paragonie-scott Thanks. I'm not sure though about pulling in the platform abstraction here instead of a dependency (openssl or libuv, for example). Also, what you proposed is going to affect speed, and may cause significant performance drop on Linux, which is bad even from the security side. Perhaps we will need tests, though.

[azet]

@bnoordhuis

This is wrong.

No it is not. Which version of OpenSSL is nodejs using? git-master? Take a look at the commit history in general from the RNG in question: https://github.com/openssl/openssl/commits/master/crypto/rand/md_rand.c

The new threading API was added to 1.1.0-pre5 on 8th of march this year: https://github.com/openssl/openssl/commit/8eed7e873bb54ab46b15e6efa3aff416e02f4d7f

Before that you had to explicitly call CRYPTO_set_locking_callback something I'm unaware node is doing w.r.t. to the OpenSSL RNG. Maybe I'm missing something?

I think you should go back and read what I wrote again (hint: not 'from'.) Please put more effort in your replies, will you?

So it seeds from urandom and mixes into Math.random()? That's still a completely unacceptable design.

Try reading from /dev/urandom from multiple processes simultaneously and you'll see what I mean. It's slow - very slow - and can have a dramatic impact on overall system performance, not just the readers.

I'm aware /dev/urandom is currently slow. That's not the point. As suggested previously: take a look at libsodium it only uses random/urandom as a seed. Newer OpenSSL releases have improved a lot, but a lot of sytems do not ship bleeding edge releases.

I hope you understand I won't be able to just take your word for it.

Sure. Again: look at their commit log, those of BoringSSL et cetera. I'm not going to name devs. - it doesn't even matter, development and changes are public.

BTW: I'm still waiting for an answer on EGD - I'm not sure what you mean.

[azet]

@ChALkeR

The fact that node uses libuv, that libuv has a thread pool, and that node uses OpenSSL RAND_bytes() for crypto.randomBytes() does not imply that calls to crypto.randomBytes() are performed from the thread pool.

Thanks for the information. So how does it work if multiple child processes want to read from crypto.randomBytes()?

[indutny]

@azet each child process starts from scratch without sharing anything except an possibly std input/outputs. So it just seeds the entropy from the kernel's RNG.

[technion]

// The only time when /dev/urandom may conceivably block is right after boot,
// when the whole system is still low on entropy.  That's not something we can
// do anything about.

Regardless of the rest of the discussion, this is objectively wrong. /dev/urandom does not block. I'm loathe to refer to the generally poor man page, but from random(4):

A read from the /dev/urandom device will not block waiting for more entropy.  If there is not sufficient entropy, a pseudorandom number generator is used to create the requested bytes

[paragonie-scott]

/dev/urandom never blocking is only a problem for programs that can run during the Linux kernel's boot process and embedded devices. There are two solutions to this:

1. If getrandom(2) is available, use it and forsake urandom. This does the correct thing: Blocks until there's enough entropy is available, then never blocks again. It's also immune to file descriptor exhaustion, making it the preferred solution.
2. Poll /dev/random until it's available, and then read from /dev/urandom forever more. Linux seeds the unblocking entropy pool (urandom) before the blocking pool. This means once you can successfully poll /dev/random, then /dev/urandom has been seeded with at least 128 bits of entropy. You may then safely read from urandom without a care in the world.

It's never acceptable to read from /dev/random and use something hacky like egd or haveged to "feed entropy" into the system.

[azet]

@indutny

each child process starts from scratch without sharing anything except an possibly std input/outputs. So it just seeds the entropy from the kernel's RNG.

Thanks for the explanation, and I've tested this on a 12core/2thread, 128GB machine over night. And it seems to be fine given a recent and well maintained OpenSSL release (debian jessie). BTW the only reason this is the case - as far as I can tell - is that commit https://github.com/openssl/openssl/commit/3cd8547a2018ada88a4303067a2aa15eadc17f39 has been added in 2013 to mitigate a problem on Android. With earlier releases it may still have been possible to exploit the PID-overlap issue and non-existent thread safety in the RNG, if I'm not mistaken.

The thing is, as explained before, as per your seeding mechanism, if RAND_bytes returns anything but -1 (i.e. 0 for low entropy or e.g. a broken userland RNG state) - if I understand @bnoordhuis correctly - the fallback would be a seed from urandom that mixes into Math.random() which is a completely unacceptable design, and predictable, it doesn't matter that the seed was of good quality in this case. You just accept the broken RNG state instead, which is completely insane as well.

@technion

Regardless of the rest of the discussion, this is objectively wrong. /dev/urandom does not block.

I agree.

@paragonie-scott

/dev/urandom never blocking is only a problem for programs that can run during the Linux kernel's boot process and embedded devices.

Unless you're planning on runnning node in initramfs there's really no reason to think about that. As for embedded devices: it largely depends, this is usually manufacturer error. They still have noise sources and interrupts, someone just forgot to configure the kernel properly or submit a patch. In any case this is nothing that node itself can do about, it's a proper upstream problem. I don't like the polling idea, getrandom(2) is perfectly fine as it's the new interface on Linux but as such: it's not available everywhere. Instead of writing your own solution I'd suggest importing sysrandom from libsodium. This is vetted code that works.

It's never acceptable to read from /dev/random and use something hacky like egd or haveged to "feed entropy" into the system.

Exactly. And I'm thus not sure what @bnoordhuis is referring to. All these have been deprecated, the OpenSSL interface is ancient and their support for it has been dropped.

[paragonie-scott]

Instead of writing your own solution I'd suggest importing sysrandom from libsodium. This is vetted code that works.

Agreed. I previously ported a significant chunk of libsodium's sysrandom to PHP 7's random_bytes(). I was intending to do the same thing here.

[ChALkeR]

@azet
Btw, I made a mistake in the comment above. Requests to RAND_bytes are indeed coming from the thread pool if callback is specified. However, OpenSSL rng is not thread-safe only unless thread safety is ensured with CRYPTO_set_locking_callback (see https://wiki.openssl.org/index.php/Random_Numbers#Thread_Safety), and some more bits. Node.js is doing those since https://github.com/nodejs/node/commit/97cada0e6a130791ceafaa750f9009d8fdaaadbb.

Also, OpenSSL RAND_bytes is not fork-safe, but that doesn't affect us, because Node.js doesn't fork, it runs new instances.

[eozu]

A little late to the party, but if this helps, the folks over at Chromium had a similar discussion regarding the CSPRNGs. If I read it correctly, they dropped OpenBSD's RC4/ARC4 for kernel-space sources.

The current implementation (as clarified here) appears to grab hardware-based entropy – if available – before reverting to standalone urandom. Specifically, it tests for the presence of RDRAND Intel's Digital Random Number Generator (DRNG), and if available, combines it with urandom and ChaCha20.

I think this is similar to what @paragonie-scott is aiming to patch, but I'm unsure if the RC4 flaws have been addressed?

Thank you all for doing your best to keep us secure. 👍

Edit: Intel no longer calls it RDRAND for various reasons. ;)

[ChALkeR]

Looks like Linux urandom will now be ChaCha20-based: http://lkml.iu.edu/hypermail/linux/kernel/1607.3/00275.html

[ChALkeR]

(Loosely related): yet another userspace prng has falled down:
https://lists.gnupg.org/pipermail/gnupg-announce/2016q3/000395.html

[ChALkeR]

/cc @saghul — would something like https://github.com/nodejs/node/issues/5798#issuecomment-222329062 make sense in libuv?

[saghul]

@ChALkeR I think it would. Moreover, I was planning on doing it myself, since I implemented a similar thing for another project not so long ago.

The key will be documenting what guarantees we promise. I'll try to make a quick stub this week to ge the conversation started over there.

[saghul]

Not 100% ready yet, but feedback is more than welcome: https://github.com/libuv/libuv/pull/1055

[sam-github]

For those that believe Linux's /dev/urandom can have no security issues: https://www.schneier.com/blog/archives/2013/10/insecurities_in.html (and others).

I haven't seen a single compelling reason in this thread to switch away from OpenSSL's PRNG, its a platform abstraction layer on top of platform-specific entropy sources... seems like exactly what we want.

Also, its easy to complain about some kind of perceived "low quality" of OpenSSL - but suggesting that the Node.js team has a better cryptographic background than their devs, and that we should be writing our own PRNG abstraction layer is a bit tenuous, IMO. We certainly wouldn't get the scrutiny that they do.

[paragonie-scott]

For those that believe Linux's /dev/urandom can have no security issues: https://www.schneier.com/blog/archives/2013/10/insecurities_in.html (and others).

That's not the argument, at all. The argument is this:

1. Your entire operating system already depends on the kernel's CSPRNG (e.g. /dev/urandom) to be secure. If that's insecure, your entire system is hosed.
2. Adding a second userspace PRNG doesn't create defense-in-depth against the failure of the OS's CSPRNG, it just creates an additional point of failure.
3. Given that OpenSSL's userspace PRNG has a lot of problems (i.e. isn't fork-safe, which screwed over the security of PHP applications that depended on its PRNG), there's more urgency to get off of a userspace PRNG in favor of the kernel's CSPRNG than if it were a better-designed userspace PRNG.

That's a very different argument than "there will never be a /dev/urandom flaw".

Several security experts have chimed in on this thread. The consensus is unanimously in favor of using the kernel's CSPRNG over OpenSSL's userspace PRNG.

I haven't seen a single compelling reason in this thread to switch away from OpenSSL's PRNG

I linked several research papers above. Did you read through them all?

its a platform abstraction layer on top of platform-specific entropy sources... seems like exactly what we want.

If that were true, there'd be no complaint. But that's not what's going on under the hood. OpenSSL's RAND_bytes() seeds itself with 32 bytes from urandom, then maintains its own internal PRNG state (mixed with SHA1 by default, but the code supports mixing with MD5) and never touches the kernel's CSPRNG again.

What you want is something like libsodium's sysrandom. @bascule previously wrote a Ruby gem, called sysrandom, which is a secure replacement for Ruby's SecureRandom (misnomer, also uses OpenSSL).

I've written about this in a blog post appropriately titled, How to Generate Secure Random Numbers in Various Programming Languages.

Also, its easy to complain about some kind of perceived "low quality" of OpenSSL - but suggesting that the Node.js team has a better cryptographic background than their devs, and that we should be writing our own PRNG abstraction layer is a bit tenuous, IMO.

You're right. We should just use libsodium's, which does the job better than any other implementation I've seen.

We certainly wouldn't get the scrutiny that they do.

Until last year, I'd have taken this as tongue-in-cheek, with Heartbleed and whatnot.

[sam-github]

Adding a second userspace PRNG doesn't create defense-in-depth against the failure of the OS's CSPRNG, it just creates an additional point of failure.

User-space gives defence against locking problems in urandom, as you have been told, and is one of the objections to OpenSSL itself shifting over to direct read of urandom.

A well-constructed PRNG cannot magically add entropy, but neither does it magically subtract.

Node implementing its own platform abstraction for entropy also adds an additional point of not-well-reviewed failure, so this point is not compelling without demonstrating that OpenSSL currently has security flaws _that effect node_ and need to be fixed by bypassing it.

Given that OpenSSL's userspace PRNG has a lot of problems (i.e. isn't fork-safe, which screwed over the security of PHP applications that depended on its PRNG), there's more urgency to get off of a userspace PRNG in favor of the kernel's CSPRNG than if it were a better-designed userspace PRNG.

Irrelevant, node doesn't fork, why do people keep bringing this up?

Are there other, relevant, criticisms?

Several security experts have chimed in on this thread.

You should keep in mind that it is impossible in this thread to distinguish between security experts, and people just playing them on the internet. Also that security experts _are_ contributors to OpenSSL.

Who do you think are security experts, and why?

[Trott]

Aside: Assuming we're not already doing so, it might be a good idea to add the sodium package to our CITGM runs. Doesn't seem to get a _ton_ of usage, but a decent amount, and it seems like exactly the kind of thing that might help us find some unusual corner cases. (Plus, I'm sure we'd be doing the maintainers a favor if we identified either unusual platforms where the module does not work or notified them ahead of time that an upcoming Node.js release is going to break their package.) /ping @TheAlphaNerd

[paragonie-scott]

Who do you think are security experts, and why?

In no _particular_ order:

• Dan J Bernstein, cryptography professor who helped bring us:
  
  
  
  • Salsa20
  
  
  • ChaCha20
  
  
  • Poly1305
  
  
  • X25519 over Curve25519
  
  
  • SipHash
  
  
• Tanja Lange, whom is the professor spearheading the worldwide post-quantum cryptography research initiatives.
• Thomas Ptacek (@tqbf), who ran an appsec/crypto pen-testing firm for years and based his "use urandom" blog post on the plight and insight of industry experts you and I can't just tap for a quick question without writing up a $X0,000 contract first
• Matthew D Green (@matthewdgreen), cryptography professor known for the iMessage forgery attack, the TrueCrypt audit, and the zcash project
• Daira Hopwood (@daira), because of hir work on projects like SPHINCS and zcash
• JP Aumasson (@veorq), who helped bring us (among other things):
  
  
  
  • BLAKE and BLAKE2
  
  
  • SipHash
  
  
  • NORX (3rd round CAESAR entry, which I believe will be a finalist)
  
  
• Anthony Ferrara (@ircmaxell) whom:
  
  
  
  • Wrote the password hashing library used by PHP
  
  
  • Has contributed a lot of practical security advice to various open source projects, in addition to providing a plethora of correct StackOverflow answers about crypto/security
  
  
• Aaron Zauner (@azet), whose contributions on Github stand alone
• Steve Weis (@sweis), whom I've bumped into reporting chosen-ciphertext attacks on "encrypted messaging" mobile applications before
• Filippo Valsorda (@FiloSottile), whom discovered Heartbleed but has also published a lot of other great crypto research, including a practical Bleichenbacher'06 exploit against an RSA implementation in Python. (EDIT: Was mistaken on this point.)
• Solar Designer (@solardiz), a widely known security researcher

Additionally, there are plenty of people I consider colleagues worth mentioning because:

1. They research these topics
2. If I'm wrong, they'll say so

And so the list expands to include:

• Sven Slootweg (@joepie91)
• Taylor Hornby (@Defuse)
• Adam Caudill (@adamcaudill)
• Aaron Toponce (@atoponce)
• Alfie (@alfiepates)
• Zofrex (@zofrex)

Feel free to ask anyone on that list if they'd prefer OpenSSL's userspace PRNG or the operating system kernel's CSPRNG for cryptography purposes. I guarantee you'll get 100% in favor of urandom (and urandom equivalent).

Feel free to try to find cryptographers and real world security experts to offer a dissenting opinion. By and large, you'll be more successful at finding folks who agree with me than disagree with me (only on _this particular point_).

[paragonie-scott]

(Corrected a mistake in my previous comment and wanted to make it known that there are probably about 50 - 100 other people I could have listed there too, but the list is plenty long enough.)

[sam-github]

OK, I also got the opinions of someone I know to be a security expert (the requirement is a successful track record of _breaking_ systems), and they agree with you (and know you): current best practice is direct read from O/S entropy sources.

Whether that is best for Node.js though, is not as clear, there are other factors to consider.

1. Is it proposed to only modify crypto.randomBytes(), or to also replace the entropy source used during SSL/TLS exchanges by OpenSSL? If we don't make OpenSSL/Node.js's HTTPS implementation bypass the user-space PRNG, then we aren't getting a win for Node.js's common use-cases.
2. Following on to that... if OpenSSL changes upstream to do direct read (as they seem to have an interest in doing)... then we can take advantage of that for both HTTPS, and crypto.randomBytes() with zero work (other than an OpenSSL update).
3. Performance remains a concern. In the absence of a current vulnerability in OpenSSL, we would be gaining a theoretical security advantage, but losing actual performance. This should be addressed. Perhaps by convincing ourselves that HTTPS performance is not impacted, and crypto.randomBytes() isn't used enough for us to care (or notice).
4. Maintenance remains a concern. If Node takes on the burden of maintaining an abstraction layer for system entropy collection, our abstraction code could become the new weakest link in the chain of randomness. Whereas if we push that up to OpenSSL (or even libsodium), we can benefit from their work. Bringing in an entire new sub-dep (libsodium) is a heavy price to just remove a theoretical problem.

[paragonie-scott]

1, 2, and 4: RAND_system_bytes() is not implemented as of 1.1.0 (which was just released), and we'll be waiting for a while until 1.2.0 comes out. But fixing it in OpenSSL rather than in each language would be the ideal solution.

3: If the two are in conflict, security should take precedence over performance when it comes to cryptography. However, a significant performance penalty just invites DoS attacks (which is a security problem). So I agree, this should be addressed.

Fixing the issue upstream would be the best solution (since it'd pay forward to lots of programming languages), but in the absence of an upstream solution, what do we do?

A. Keep using a userspace PRNG and not exposing an alternative CSPRNG interface?
B. Fix it at the interpreter layer, like PHP did?

That's for the Node team to decide. I'm jumping over the OpenSSL repository to discuss getting a patch ready to expose this API in the next version.

[saghul]

I'd like to clarify that while libuv might get this API I didn't implement
it with this issue in mind. Node need not use it.

Moreover, even if the libuv patch lands tomorrow (which will not happen
anyway) it's targeted at master, not v1.x. That means that in principle it
will be part of libuv 2.0. This is intentional, so we have a bit more
leeway to make changes should issues be discovered with the implementation.

On Sep 16, 2016 23:52, "Scott" [email protected] wrote:

1, 2, and 4: RAND_system_bytes() is not implemented as of 1.1.0 (which
was just released), and we'll be waiting for a while until 1.2.0 comes out.
That would be the ideal solution.

3: If the two are in conflict, security should take precedence over
performance when it comes to cryptography. However, a significant
performance penalty just invites DoS attacks (which is a security problem).
So I agree, this should be addressed.

Fixing the issue upstream would be the best solution (since it'd pay
forward to lots of programming languages), but in the absence of an
upstream solution, what do we do?

A. Keep using a userspace PRNG and not exposing an alternative CSPRNG
interface?
B. Fix it at the interpreter layer, like PHP did?

That's for the Node team to decide. I'm jumping over the OpenSSL
repository to discuss getting a patch ready to expose this API in the next
version.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/issues/5798#issuecomment-247717511, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATYGKYZE_uzOGuvyEgLZUs257XOxBJyks5qqw-UgaJpZM4H0YGk
.

[joepie91]

@sam-github: Is it proposed to only modify crypto.randomBytes(), or to also replace the entropy source used during SSL/TLS exchanges by OpenSSL? If we don't make OpenSSL/Node.js's HTTPS implementation bypass the user-space PRNG, then we aren't getting a win for Node.js's common use-cases.

I just want to point out here that there are plenty common _non-HTTPS_ usecases for crypto.randomBytes in Node - from UUID generation, to other (non-OpenSSL) crypto implementations, to lottery-type random-number-picking, and so on. Even _if_ the problem would remain unsolved for HTTPS, there'd still be a clear win in other areas.

[co60ca]

To @joepie91 I've seen a implementation of UUID v4 use math.random so yes, a secure default that pulls from system entropy would be fantastic.

[atoponce]

As another example of using crypto.randomBytes() in a non-HTTPS setting, I am using it in a password generator: https://github.com/atoponce/nodepassgen.

[jasnell]

ping @nodejs/crypto ... should this remain open?

[bnoordhuis]

No, let's close.

[azet]

this issue remains unfixed and there has been valuable discussion on resolving the problem.
similar issues in e.g. ruby proper lead to switch to libsodium-like approaches: https://bugs.ruby-lang.org/issues/9569

OpenSSL's implementation remains, to my knowledge, largely untested and specifically optimized to cater to TLS implementation's needs for fast random numbers. most languages (e.g. python) opt to use the /dev/urandom device on Linux distributions. libsodium is platform aware and spares the overhead of fiddling and maintaining distribution specific code-paths. libsodium has also been reviewed and is used by a lot of people and projects to provide a layer of abstraction for obtaining cryptographically secure pseudo random numbers.

I urge the core team to use libsodium's reviewed, platform-aware way to obtain secure pseudo random numbers: https://github.com/jedisct1/libsodium/blob/master/src/libsodium/randombytes/randombytes.c

please re-open this security relevant issue.

[tarcieri]

What was the rationale for closing this issue?

[Trott]

If I understand correctly (which is a big "if" when it comes to cryptography and related issues):

Especially given activity on https://github.com/openssl/openssl/issues/898 fairly recently, and that progress there could mean a suitable fix here that can still use OpenSSL APIs, I'd be for keeping this open.

[sam-github]

@nodejs/crypto I think we should keep it open until its fixed. Last discussion was that OpenSSL were themselves moving this direction, and we hoped to simply inherit their fix. From https://github.com/openssl/openssl/issues/898, it does indeed look like they are moving along (slowly, but with recent progress). But if OpenSSL 1.1 doesn't change this, we could revisit whether node should use some other way of getting pseudo-random.

[MylesBorins]

As a handful of people (including collaborators) have chimed in about wanting to continue the discussion. I'm reopening

That being said I think this might be part of a larger conversation about openssl... perhaps we can look into improving their implementation on systems that have support?

afaict libsodium is going to be a nonstarter if it doesn't have fips support

[paragonie-scott]

afaict libsodium is going to be a nonstarter if it doesn't have fips support

Replacing OpenSSL entirely with Libsodium might be a nonstarter.

Is FIPS-140-2 compliance an absolute mandatory requirement? Because ECB mode is FIPS compliant, and you sure as hell don't want people encrypting that way.

[technion]

Any changes to OpenSSL, even if they improve in this space, are going to show up on a new major release, which moves much, much slower than Node. If they made a great release tomorrow, it will take years to hit some distributions at all. I don't feel you want to wait on this.

[Trott]

If they made a great release tomorrow, it will take years to hit some distributions at all.

Node.js doesn't use the operating system's OpenSSL. Node.js ships with its own copy of OpenSSL.

[bnoordhuis]

I might be open to bundling libsodium in addition to openssl but that should be its own issue because there will be numerous details to hash out.

We sure as heck are not going to ship something homegrown and until openssl grows the requisite APIs there isn't anything actionable. I'm closing this again and I'm locking it to prevent this already humongous thread from growing bigger.

[ChALkeR]

@MylesBorins Re: FIPS compliance — if libsodium randombytes.c is not compliant and if FIPS compliance is desired, that could be fixed with a runtime flag that switches back the impl to OpenSSL (as we won't stop bundling OpenSSL nevertheless). Also, see this link.

[sam-github]

Note that openssl 1.1 won't be FIPS compliant for quite a while, so FIPS users will need to stick to a node built on 1.0. cf. https://www.openssl.org/blog/blog/2016/07/20/fips/

I'm still hopeful that OpenSSL will deal with this for us, I'm not saying we should be thinking of libsodium for 8.x, but I don't think FIPS would be a blocker _if_ we go looking elsewhere for a PRNG. Also, I'm not sure random seeds are covered by FIPS, anyway, that would take some looking into.

[shigeki]

Note that OpenSSL has just landed a commit to use DRGB with AES-CTR of NIST SP 800-90A as https://github.com/openssl/openssl/commit/75e2c877650444fb829547bdb58d46eb1297bc1a. We can use it with the os-specific seeding source (e.g. /dev/urandom) by a default define flag of OPENSSL_RAND_SEED_OS.
I think it is best for us to wait for the next release of OpenSSL-1.1.1.

[davisjam]

[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.

[paragonie-scott]

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.

[bnoordhuis]

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.

[ChALkeR]

@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?

[bnoordhuis]

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.

[FiloSottile]

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.

[davisjam]

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