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

don't fallback to PRNG if real CSPRNG is not available #2

Closed
timkuijsten opened this issue Oct 17, 2014 · 3 comments
Closed

don't fallback to PRNG if real CSPRNG is not available #2

timkuijsten opened this issue Oct 17, 2014 · 3 comments

Comments

@timkuijsten
Copy link

As I've pointed out in dcodeIO/bcrypt.js#16 I have the feeling that using a self-supplied RNG is considered insecure and a CSPRNG should always be supplied by the host a program is running on. In case of a browser environment this would be the Web Crypto API.

I can imagine that having a hard fail on older browsers is inconvenient, but not letting a user know that an extremely weak RNG [1] is substituted for a real secure RNG will quite possibly give a false sense of security without anyone noticing it.

I would propose a hard fail like the original bcrypt-nodejs module [2] or at least a way to default to secure behaviour and let the user of the module do extra work if he/she really wants a "convenient" insecure fallback.

[1] https://github.com/fpirsch/twin-bcrypt/blob/master/src/twin-bcrypt.js#L23
[2] https://github.com/shaneGirish/bcrypt-nodejs/blob/master/bCrypt.js#L559

@fpirsch
Copy link
Owner

fpirsch commented Oct 17, 2014

You're right.
Event cryptographically secure generators like Fortuna or Isaac must be seeded, and there's no way to seed them properly in a browser. And as crypto.getRandomValues is widely available, the best option IMO when it's not available is to refuse to generate salts, and suggest the user updates her browser.
This change will not be retro-compatible for older browsers, so a new major version is needed.

@timkuijsten
Copy link
Author

Sounds like a plan and thanks for the clarification.

@timkuijsten
Copy link
Author

Looks like 52933eb fixes this.

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

No branches or pull requests

2 participants