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

Guidance on A6 does not follow Node.js best practices for the Crypto module #105

Closed
jboyer2012 opened this issue Sep 26, 2017 · 2 comments
Closed

Comments

@jboyer2012
Copy link
Contributor

According to Node's Crypto API docs, the default initialization vector used by the createCipher function is not random enough to protect against brute-force attacks.
Here is the appropriate quote from the Crypto API docs:

The implementation of crypto.createCipher() derives keys using the OpenSSL function EVP_BytesToKey with the digest algorithm set to MD5, one iteration, and no salt. The lack of salt allows dictionary attacks as the same password always creates the same key. The low iteration count and non-cryptographically secure hash algorithm allow passwords to be tested very rapidly.

In line with OpenSSL's recommendation to use pbkdf2 instead of EVP_BytesToKey it is recommended that developers derive a key and IV on their own using crypto.pbkdf2() and to use crypto.createCipheriv() to create the Cipher object.

Based on this, we should update the code samples and tutorial for A6 to include this logic. Developers may be looking to simply copy the code there which will result in their encryption not being as strong as expected due to poor implementation. I have submitted PR #104 to resolve this issue.

I updated the A6 tutorial code to create an IV first using the config.cryptoKey and PBKDF2 and then use createCipheriv to create the cipher and decipher objects. See the PR for more details.

@ckarande
Copy link
Member

@jboyer2012 Thanks for proactively reaching out and providing the context for the PR. Really appreciate your contribution. I will review and merge the PR or get back to you if any questions 👍

@ckarande
Copy link
Member

Merged the PR. Thanks.

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