crypto: warn if counter mode used in createCipher

[shigeki]

crypto.createCipher() sets the fixed IV derived from password and it
leads to a security risk of nonce reuse when counter mode is used.
A warning is emitted when CTR, GCM or CCM is used in
crypto.createCipher() to notify users to avoid nonce reuse.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

crypto

Fixes: #13801

CC @bnoordhuis, @indutny

[shigeki]

CI is running on https://ci.nodejs.org/job/node-test-pull-request/8760/.

[iangcarroll]

Is it possible to put the documentation warning in a box like one of the alerts for stability notices? This really needs to be more prominent, and the wording should be more direct, i.e replace the entire paragraph with something like:

When using ciphers that require an initialization vector, createCipher will insecurely derive a non-random initialization vector from the key you provide. Messages with the same plaintext will always have the same ciphertext. Never use createCipher with counter modes (e.g. CTR, GCM or CCM), as reusing initialization vectors may result in no encryption at all. A warning will be emitted if you attempt to use createCipher with one of these modes.

Though if it goes in an alert box it'll obviously need to be more concise. Maybe just "Don't use this function".

[tniessen]

Maybe just "Don't use this function".

See #13801 (comment), there are justified use cases of createCipher.

[tniessen]

I think the problems and security implications of using createCipher are not limited to counter modes, but FWIW, this won't make it worse.

[iangcarroll]

there are justified use cases of createCipher.

"Don't use it" was mostly a joke, but still holds true even for ECB. ;)

but FWIW, this won't make it worse.

Well, I mean, sure. I'd still say the documentation needs to be stronger here before it's merged. Especially if people come across the warning and the documentation says it's a "recommendation".

[indutny]

LGTM

[indutny]

Nitpicking: could you please surround if's body with braces {...}?

[shigeki]

Thanks. Fixed.

[cbarcenas]

Thanks for getting the ball rolling on a fix for this, @shigeki! Some thoughts:

crypto.createCipher() sets the fixed IV derived from password and it leads to a security risk of nonce reuse when counter mode is used.

"Leads to a security risk" is a huge understatement... in common block cipher modes (e.g. CTR), IV reuse is catastrophic to confidentiality. Observing just a few ciphertexts is sufficient in many cases to derive the underlying key and/or plaintexts, compromising the encryption scheme entirely. (See here for a good explanation.)

Reiterating my concerns from #13801, I think a much better change is for createCipher to fail when called with a block mode that requires an IV. Yes, this is a API-breaking change, but it's much better than the current behavior. The current behavior is completely broken in a cryptographic sense once the same password is used for more than one encryption operation!

If Node were to adopt this fail-fast behavior, the legacy key/IV generation should be exposed in a public immediately-deprecated API (e.g. generateLegacyKey/generateLegacyIV), so existing projects that rely on the old behavior of createCipher could easily generate the same key/IV data from their preexisting passwords. Because such an API call would be deprecated, it would serve as good impetus to migrate behavior away from the broken key/IV-generation scheme and move toward proper use of these block cipher modes via createCipheriv.

What are your thoughts about this?

I must admit, I have never contributed to this project before, so I'm not sure of the feasibility of these changes and I may not have the sufficient context on this project to make the changes myself, but I'm happy to hear what veterans of this project might think about this proposal.

[bnoordhuis]

I think I'm +1 on @cbarcenas's proposal. It's been proven time and time again that users simply ignore or mute warnings so I don't have much hope for this PR.

If we're going to make a hard break, are there other things that should go?

[stouset]

@cbarcenas That is an improvement over the current situation, but a better approach in my opinion is for crypto.createCipher to generate IVs for the end-user, and return them with the output of cipher.final(). Or alternatively, if you don't want to change semantics of existing functions, deprecate the existing function and replace it with a newly-named one.

Users should not, in the general case, be asked to deal with details like IV generation. Modes like CTR have different security requirements (uniqueness) than CBC (unpredictability), and users will not consistently get this right. Similarly, modes like GCM only provide an advantage when the auth tag is actually provided during decryption. Naïve searching shows that only about 80% of users actually take this step when using GCM. Including the auth tag (when relevant) in the output when using crypto.createCipher would also eliminate this entire class of weakness.

If a user does want to take things into their own hands, they can use the existing createCipheriv.

[cbarcenas]

@stouset I agree with you in principle that letting Node handle IV generation would improve usability and security. However, something to consider here is making these changes as backwards-compatible as possible, and giving projects an easy path to remediate this issue without relying these projects' developers to have cryptographic expertise.

My main concern is that including the IV in the output of cipher.final() will break decryption in projects that take ciphertext output from cipher.final() and blindly pass it into decipher.update() without preprocessing.

I guess an underlying question here is whether the crypto module is intended to provide high- or low-level cryptographic functionality.

[mscdex]

Also, I think having differing .final() return values depending on whether createCipher() or createCipheriv() was used would be unexpected behavior. On top of that, returning objects (plain or otherwise) from C++ land would decrease performance (although passing an object created in JS land to C++ land might be slightly faster -- at least it used to be).

[stouset]

@cbarcenas Yeah, I wouldn't mind such an API having a new name.

@mscdex The return value wouldn't change. It would just be slightly longer to accommodate the IV (and ideally tag, for authenticated encryption). This would be essentially transparent to consumers of the API.

[shigeki]

I agree that nonce reuse in counter mode leads catastrophic. For examples in TLS case of AES-GCM, "Nonce-Disrespecting Adversaries" shows that the MAC key of GHASH can be derived and encrypted message can be tampered by bit flipping.

I wrote a modest description in the doc because the nonce is not always reused in crypto.createCipher if it is used only once or keys are changed in every encryptions.
For example in case of plain text is known, we can do it unless it has hash collision as

const crypto = require('crypto');
const key = 'pass';
const plain = 'hello world';
const cipher = crypto.createCipher('aes-128-gcm', crypto.createHmac('sha256', key).update(plain).digest());
const encrypted = cipher.update(plain);

I also think that we cannot prohibit reused nonce even if we use crypto.createCipheriv. Some people might use a random value as nonce such that

const crypto = require('crypto');
const key = 'pass';
const plain = 'hello world';
const cipher = crypto.createCipheriv('aes-128-gcm', key, crypto.randomBytes(12));
const encrypted = cipher.update(plain);

But it has a possibility of nonce reused when it is called in extremely large number due to birthday paradox. The situation of nonce reused largely depends on how user use it and I think we cannot cover all the cases.
That is why AES-SIV RFC5297) and AES-GCM-SIV are developed. We should encourage to use them against nonce reuse. Unfortunately, they are not yet widely used. AES-GCM-SIV is under standardization and only boringSSL has just supported it recently. I think we can only let users generate IV to meet their use and security requirements for now.

[bnoordhuis]

Needs a rebase but let's land it, it still improves on the status quo.

[bnoordhuis]

Maybe make this warning a little stronger, e.g. "a predictable IV undermines the security of counter-based ciphers."

Would it be worthwhile to mention that this API predates the addition of counter-based ciphers to OpenSSL, hence why it's not an error outright?

(Really, the more I think about it, the more I feel throwing an exception is the right thing to do. If it weren't for backwards compatibility...)

[shigeki]

I changed the doc into a little stronger description as ae7dbb1 and add a reference of Nonce-Disrespecting Adversaries.

Would it be worthwhile to mention that this API predates the addition of counter-based ciphers to OpenSSL, hence why it's not an error outright?

I think it gets more complicated description in the doc. We can make throwing an error some time later after enough time.

[stouset]

A predictable IV does not undermine the security of counter-based ciphers. In fact, a simple incrementing counter is a recommended approach when using counter-based schemes that have IVs too small to reliably be generated uniquely at random.

[bnoordhuis]

@stouset I think you may be misunderstanding how openssl implements them and what "IV" means in that context. The best description I could find:

https://stackoverflow.com/questions/3141860/aes-ctr-256-encryption-mode-of-operation-on-openssl/3146214#3146214

[stouset]

I checked the link, and I might be wrong but I don't think anything in it contradicts my response. I'll do some more research, but at the very least the wording you suggested may be confusing and need clarification if there's a particular reason it's accurate in this case. I'm in an area with poor signal but I should be able to take a better look later tonight.

[shigeki]

I think that IV should be a nonce not a random so that it must not be reused with the same secret key but it can be predictable.

[bnoordhuis]

Yes. I think the confusion is around the fact that the IV to OpenSSL's EVP_CipherInit_ex() is the concatenation of the nonce and the counter as a single bit string.

[stouset]

Right, but it's fine if that's predictable as long as it's never repeated. The nonce in CTR mode can be predictable (and is often recommended to be a simple counter in appropriate contexts), and the counter is itself just an actual counter.

[bnoordhuis]

I don't think we are in disagreement. Is your beef with the wording in this PR? Can you suggest an alternative?

[shigeki]

Rebased against the latest master and CI is running on https://ci.nodejs.org/job/node-test-pull-request/9777/.

[bnoordhuis]

Still LGTM.

[shigeki]

The previous CI had an Jenkins error. A new CI of https://ci.nodejs.org/job/node-test-commit/11951/ is all green. Landed in 9dfb2d1.

@stouset If you have any proposal, please submit a new PR.

Thanks for reviewers.

[MylesBorins]

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

[bnoordhuis]

v6.x: #16583