Insecure random values should be opt-in

[joepie91]

Right now, this module will automatically fall back to Math.random() in browser environments that don't support CSPRNG use, essentially failing open.

To operate securely, it should instead fail hard with an error, and only allow falling back to an insecure random source if the developer explicitly indicates that this is acceptable.

[defunctzombie]

While I understand your point, I think the current behavior is more developer friendly and since this module isn't even aiming for strong crypto I feel less strongly that this is a real problem.

[broofa]

I actually feel like this is a pretty legitimate request. Most devs aren't aware of the security issues of less-than-crypto-quality PRNGs. Requiring them to opt-into the existing possibly-insecure behavior is probably a good thing.

... in the next major version, though.

Reopen?

[defunctzombie]

Sure, if you want to reopen go for it.

My thinking was that even tho it isn't crypto quality, this lib isn't that hurt by not being crypto quality as fallback since it isn't a crypto library itself. But if you have a good idea for the API change we can do another major bump.

[pgaubatz]

Hi @joepie91

You might have a look at my implementation that doesn't fallback to Math.random: https://github.com/pgaubatz/node-uuid

Cheers,
Patrick

[broofa]

For the record, crypto.getRandomValues is broadly supported these days.

[willsr]

I think this a serious issue. We have had issues with UUID collisions, specifically in test suites. The whole point of a UUID is to be universally unique...

[broofa]

@willsr, can you elaborate? UUID collisions are always a concern. Specifically, what platform are you running on? What code are you running to generate UUIDs? How often are you seeing collisions? etc...

[Edit: Also, given that getRandomValues is broadly supported, what platform are you running on where this module is falling back to Math.random()?]

[broofa]

For the record, the CVE for this issue: http://www-01.ibm.com/support/docview.wss?uid=swg21982849

[luxalpa]

If you look at this article, you'll notice that it is a very severe problem as some browsers (most notably the google webcrawlers) fall back to a very weak implementation in Math.Random which guarantees collisions after just a few thousand invocations. This causes some severe problems in some applications and as such needs to be fixed. Snowplow has big problems because of this.

[anagytherealone]

How about using Mersenne Twister pseudorandom number generator when the Cryptograpgy API is not available?
So here: https://github.com/kelektiv/node-uuid/blob/master/lib/rng-browser.js#L26 instead of using Math.random() it would use the Mersenne Twister algorithm to generate the random numbers.

[nagyadam2092]

A simple solution would be to use chancejs's implementation of the Mersenne Twister algorithm:
https://github.com/chancejs/chancejs/blob/master/chance.js#L7076

[joepie91]

The issue here is that insecure random values - ie. any random values that do not originate from a CSPRNG - should be opt-in, rather than an automatic fallback.

The addition of a Mersenne Twister implementation, while a possible improvement over plain Math.random in some environments, does not solve this issue; MT is still not a CSPRNG, and should still not be an automatic fallback.

[broofa]

I don’t believe embedding a PRNG like mersenne (or other algo) is the right solution. Instead, this module should throw an Error in environments where no crypto-PRNG is available, and provide an API for devs to inject an PRNG of their choosing. (And provide a a doc example showing how to do that using Mersenne?)

That way we avoid adding a dependency that most folks won’t need, and allow devs to provide whatever algo best suits their needs.

[broofa]

BTW, the reason I haven't acted on this (aside from just generally being busy with other stuff), is that the PRNG feature-detection is done at module load-time currently. So there's a race condition of sorts around how a dev would inject a custom RNG function before uuid decides to throw for lack of a viable native PRNG.

Resolving this probably isn't a big deal, but it's enough of a change that I haven't had the time to sit down and sort through it. If someone wants to suggest an approach/put up a PR, that would be welcome.

[ghost]

@broofa You can do something like this:

uuid/lib/rng.js -> uuid/lib/default-rng-algorithm
uuid/v1.js -> uuid/lib/v1.js

uuid/lib/rng.js

var implementation = function (){
	throw new Error("Random number generator is not set.");
};

var abstractRNG = function (){
	return implementation();
};

abstractRNG.use = function (rng){
	implementation = rng;
};

module.exports = abstractRNG;

uuid/v1.js

const uuidv1 = require('./lib/v1');
const rng = require('./lib/rng');
const defaultRngAlgorithm = require('./lib/default-rng-algorithm');
rng.use(defaultRngAlgorithm);

module.exports = uuidv1;

I think this is the best solution if you want to keep the current procedural approach and you want to stay backward compatible. You can decide whether you want to keep the current - crypto.getRandomValues with Math.random fallback - implementation as default or change to crypto.getRandomValues only and increase the major version number. If they want them to use their own implementation, then you should publish the uuid/lib/v1.js and the uuid/lib/rng.js through an uuid/custom.js, so they can write something like the upper uuid/v1.js.

[ctavan]

Suggestion @broofa #337 (comment)

I think we can fix a number of issues here by distilling the implementation of this file down to:

export default function() {
  if (Math.getRandomValues) {
    return Math.getRandomValues(new UInt8Array(16));
  } else if (typeof(crypto) != 'undefined' && crypto.getRandomValues) {
    return crypto.getRandomValues(new UInt8Array(16));
  } else if (typeof(msCrypto) != 'undefined' &&  && msCrypto.getRandomValues) {
    return msCrypto.getRandomValues(new UInt8Array(16));
  }

  // TODO: Add README verbiage describing error and how to resolve by providing user-supplied rng function
  throw Error('Unable to locate getRandomValues() API.  See https://github.com/kelektiv/node-uuid#secure-api');
}

• Fixes Insecure random values should be opt-in #173 by removing dependency on Math.random()
• Forward-compatible with tc39/proposal-uuid#33
• Supports user-supplied rng (replace / supply Math.getRandomValues())

By not caching a reference to getRandomValues it becomes trivial for uses to supply their own implementation as needed. There's probably a modest perf penalty to feature sniffing this on each call but I don't think it's a significant concern.

[ctavan]

@broofa I'm not 100% sure about the idea of forward-compatibility with Math.getRandomValues(). I think the proposal is still in a very early stage and there is not yet any reason to believe that it will really land in the language in the form that is currently being discussed. I would wait for stage-2 ("The committee expects the feature to be developed and eventually included in the standard") before assuming this will ever become available.

Putting Math.getRandomValues() into the wild here and suggesting that people could/should overwrite/polyfill it right now doesn't sound helpful to me with respect to establishing this new feature in the Language. First of all I believe it is generally a bad practice to extend the native prototypes. Second, examples like Promise come to my mind where polyfill-implementations (bluebird) got broad adoption before these features were added to the language (with often a different and partly incompatible API surface) and it took quite a while for the polyfills to vanish in favor of the native implementations.

I would prefer to offer a more explicit way of providing a custom rng.

Other concern: At this point in time I also tend to drop msCrypto support and ask people who want to support IE11 to provide a global alias (window.crypto = window.msCrypto || window.crypto) alias… Where this would be a problem or impossible, people could still continue to use uuid@3.x.

[ctavan]

Fixed in #354.

[ctavan]

We have just released uuid@7.0.0 which removes support for insecure default RNGs. Please feel free to open a new issue if you encounter any issues with the new version.

[neg3ntropy]

@ctavan updating uuid@7.0.0, it breaks for me in AWS Lambda, node 12.x:

 Error: uuid: This browser does not seem to support crypto.getRandomValues(). If you need to support this browser, please provide a custom random number generator through options.rng.
    at rng (/var/task/index.js:1964:7)
    at Module.v1 (/var/task/index.js:1938:59)
    at UuidGenerator.generateTimeUuid (/var/task/index.js:1915:74)

Locally it works, but I cannot understand what would be the difference in AWS.

[ctavan]

@neg3ntropy please check #378 (comment). The issue should be fixed with uuid@7.0.1.