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

crypto,doc: update language around key stretching #19748

Closed
bnoordhuis opened this issue Apr 2, 2018 · 11 comments
Closed

crypto,doc: update language around key stretching #19748

bnoordhuis opened this issue Apr 2, 2018 · 11 comments
Labels
crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.

Comments

@bnoordhuis
Copy link
Member

doc/api/crypto.md currently says this:

it is recommended that developers derive a key and IV on their own using crypto.pbkdf2()

That's only sound when the key+IV are used once. Using the same key+IV twice is undesirable in general and downright disastrous with counter mode ciphers:

  1. identical plaintexts encrypt to the same ciphertext (usually not what you want)
  2. leaks information about the initial plaintext block with CBC and CFB ciphers
  3. completely destroys the security of CTR, GCM and OFB ciphers

It would be good to add some guidelines on how to safely create and store IVs. They should be unpredictable but don't need to be kept secret after encrypting. Are there exceptions to this rule?

Refs: indexzero/nconf#299

@bnoordhuis bnoordhuis added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Apr 2, 2018
@tniessen
Copy link
Member

tniessen commented Apr 2, 2018

[IVs] should be unpredictable but don't need to be kept secret after encrypting. Are there exceptions to this rule?

Not that I know of. One can think of a cipher E which uses an IV N as a parameterized algorithm whose only inputs are key and message (c = EN(k, m)), and in this case the IV need not be secret according to Kerckhoff's principle.

it is recommended that developers derive a key and IV on their own using crypto.pbkdf2()

It is not strictly necessary to use a KDF to produce the IV, and it is valid to reuse a key with different IVs to some extent.

@ryzokuken
Copy link
Contributor

@bnoordhuis @tniessen could I pick this up? We could discuss the specifics of the addition.

@ryzokuken
Copy link
Contributor

ryzokuken commented Apr 3, 2018

As per my understanding of IVs,

An initialization vector should be unpredictable; ideally, they will be cryptographically random. They do not have to be secret: IVs are typically just added to ciphertext messages in plaintext. It may sound contradictory that something has to be unpredictable, but doesn’t have to be secret; it’s important to remember that an attacker must not be able to predict ahead of time what a given IV will be.

Let me know if I'm missing something or if something's wrong with my understanding of IVs.

@bnoordhuis
Copy link
Member Author

@ryzokuken Yes, that's correct.

@ryzokuken
Copy link
Contributor

@bnoordhuis in that case, should I add this particular paragraph to the docs to make it explicit? I believe it sums the whole thing up.

@bnoordhuis
Copy link
Member Author

bnoordhuis commented Apr 4, 2018

Sounds good. As long as it doesn't insinuate that IV reuse is okay, like it currently does.

@ryzokuken
Copy link
Contributor

ryzokuken commented Apr 4, 2018

@bnoordhuis Correct me if I'm wrong, but I think this only appears in the documentation for crypto.createDecipher and crypto.createCipher. Aren't they both deprecated anyway?

My point here being: if I add the paragraph, do I add it to crypto.createCipher or crypto.createCipheriv which is not deprecated, but doesn't have this exact text in its description anyway?

@bnoordhuis
Copy link
Member Author

I'd add them to all four, yes.

Aren't they both deprecated anyway?

That's right.

@ryzokuken
Copy link
Contributor

@bnoordhuis making a PR. That said, should I add the exact same text to all four functions?

@ryzokuken
Copy link
Contributor

Oh, wait. createCipher and createDecipher don't actually take an IV as an argument. Do they still need it?

@ryzokuken
Copy link
Contributor

I added the text to createCipheriv and createDecipheriv. If you think it's necessary, I'd add the same to the remaining two functions.

ryzokuken added a commit to ryzokuken/node that referenced this issue Apr 5, 2018
Update the docs to provide clearer instructions regarding the exact scope
of the use (and re-use) of an IV, stating the instructions explicitly with
greater clarity.

Fixes: nodejs#19748
targos pushed a commit that referenced this issue Apr 12, 2018
Update the docs to provide clearer instructions regarding the exact
scope of the use (and re-use) of an IV, stating the instructions
explicitly with greater clarity.

PR-URL: #19810
Fixes: #19748
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants