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

Hardening #20

Open
Ninos opened this issue Oct 13, 2021 · 3 comments
Open

Hardening #20

Ninos opened this issue Oct 13, 2021 · 3 comments

Comments

@Ninos
Copy link

Ninos commented Oct 13, 2021

Type checking

See:

keypair/index.js

Line 1020 in 9596418

if(crypto) {

Please change from:

if(crypto) {

to

if(null !== crypto) { # Yoda-Condition not needed

For such critical lib there are too less type checkings in this lib, normally such critical libs should only work in strict mode! :-)

Fallbacks

Currently you're using too many fallbacks to prevent errors. Each fallback/weakening) should be optional. Examples in your code:

  1. crypto-lib is optional
  2. weak entropy (see
    /* Mozilla claims getRandomValues can throw QuotaExceededError, so
    ).

Recommendation:
In future versions such fallbacks can be added, but just as option. E.g. if crypto is not available you could output an error with a security warning, how to disable required usage of crypto. Same with weaker entropy...

@juliangruber
Copy link
Owner

Thank you for the recommendations!

The type check is fine, if the return value of require('crypto') can't be trusted to be an object or falsy, then the environment is broken altogether.

What do you think about instead of throwing a warning, we add an option like { weak: true } which makes everything work even if crypto is unavailable or throws?

@Ninos
Copy link
Author

Ninos commented Oct 13, 2021

What do you think about instead of throwing a warning, we add an option like { weak: true } which makes everything work even if crypto is unavailable or throws?

Sounds good. You could also add something like:

{
    weak: {
        disableEntropyException: true,
        disableCrypto: true,
}

So setting weak.disableCrypto should also return true for if( weak ) (not tested!). Then you have a bit more flexibility for individual options:
if( weak && false !== weak.disableCrypto )

@mhardeman
Copy link

What do you think about instead of throwing a warning, we add an option like { weak: true } which makes everything work even if crypto is unavailable or throws?

What is the runtime environment where that happens and that you care about? The odds are fair that it's an environment that is unsuitable for generating key material.

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

3 participants