Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dubious rng #225

Closed
wants to merge 7 commits into from
Closed

Dubious rng #225

wants to merge 7 commits into from

Conversation

broofa
Copy link
Member

@broofa broofa commented Oct 1, 2017

#173 points out that quietly falling back to Math.random raises security concerns. This diff causes uuid to throw an error if/when Math.random would be used to create UUIDs, provides a hook for suppressing that error, and links to a wiki page (https://goo.gl/vz3J87) that explains the error, the hook, and a possible workaround for generating high-quality UUIDs using SJCL crypto library.

@broofa
Copy link
Member Author

broofa commented Oct 1, 2017

BTW, I'm thinking we bump the major version for this, since it may throw in cases where it didn't previously.

rnds[i] = r >>> ((i & 0x03) << 3) & 0xff;
}
function mathRNG() {
if (!rng._dubious) throw new Error('No good RNG available. See https://goo.gl/vz3J87');
Copy link
Collaborator

Choose a reason for hiding this comment

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

lint?

@defunctzombie
Copy link
Collaborator

I would probably consider allowing the user to specify the crypto, or honestly, maybe recommend they polyfill the crypto functions the library needs if their browser does not have them? I do like the idea of a library that ships with the support it needs so another option is to not change anything and simply let the user polyfill the crypto function to throw if they don't want to suppose "weak" crypto browsers.

@broofa
Copy link
Member Author

broofa commented Oct 2, 2017

let the user polyfill the crypto function

Oh, I like that idea. Can't really think of any downsides to this at the moment.

another option is to not change anything and simply let the user polyfill the crypto function to throw

This is basically asking users to fix the problem themselves. I don't think we can say we've fixed this bug if naive users are still vulnerable to the issue. I think the spirit of this change is to complain loudly if there's an issue, and then inform the user about the problem and what options are available for fixing it.

@broofa
Copy link
Member Author

broofa commented Oct 2, 2017

[rant]npm is filled with prng modules that are just wrappers around window.crypto || window.msCrypto. Boo![/rant]

'Would really like to have a code snippet demonstrating the preferred method for shimming crypto in legacy browsers.

@broofa broofa changed the base branch from eslint to master November 28, 2017 12:18
@vvo
Copy link

vvo commented Jan 15, 2018

I don't have the required knowledge to review this sorry, maybe @defunctzombie can do another pass?

@broofa
Copy link
Member Author

broofa commented Jan 15, 2018

@vvo: No worries - I'm not sure we've resolved how best to address this. As @defunctzombie says, asking the user to find a polyfill may be the best solution here, but it's up for debate.

@broofa
Copy link
Member Author

broofa commented Jun 7, 2019

Apparently I don't have time/interest to push this through.

@broofa broofa closed this Jun 7, 2019
@broofa broofa deleted the dubious_rng branch June 7, 2019 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants