crypto.createCipher should not work with AES-CTR

[iangcarroll]

Node should not allow calls to crypto.createCipher to succeed when the AES mode selected is CTR. AES-CTR is fundamentally broken when an initialization vector is used twice, and crypto.createCipher will always generate the same initialization vector for the same key, so crypto.createCipheriv needs to be used.

In the short term, there should probably be a warning about this in the function's documentation.

Posted because of HainaLi/horcrux_password_manager#1.

[tniessen]

cc @nodejs/crypto

[iangcarroll]

It appears this problem is not uncommon. This search gives ~500 JS files. Some large repositories I found that use this unsafe code (haven't checked for exploitability):

• https://github.com/HabitRPG/habitica/blob/release/website/server/libs/encryption.js
• https://github.com/Microsoft/BotBuilder-Samples/blob/master/Node/demo-ContosoFlowers/bot/utils.js#L14

And then there's this blog post: http://lollyrock.com/articles/nodejs-encryption/

[tniessen]

Similar arguments apply to many AES ciphers. It is strongly recommended to always use a random IV, no matter which block cipher mode is used. Even though the problems arising from using a constant IV are more severe for CTR, there are serious implications for other ciphers, e.g. OFB. Non-streaming modes such as CBC are less vulnerable, but still benefit from a random IV.

Maybe @shigeki has some more insights.

[bnoordhuis]

In the short term, there should probably be a warning about this in the function's documentation.

There is one, isn't there? Or am I misunderstanding you?

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

Or do you specifically want to call out AES-CTR? Like @tniessen mentions, it's not specific to that cipher.

[tniessen]

In the short term, there should probably be a warning about this in the function's documentation.

There is one, isn't there? Or am I misunderstanding you?

We might want to visually and verbally highlight the security implications of using createCipher with vulnerable block cipher modes, especially for users without sufficient knowledge about symmetric ciphers.

[iangcarroll]

We might want to visually and verbally highlight the security implications of using createCipher with vulnerable block cipher modes

Yeah, agreed, that was my issue -- "recommended" is a bit far from a warning.

This does become a problem for other modes as well. Maybe it's worth talking about modifying the API to only have createCipheriv as createCipher? I'm not very familiar with Node's processes for that, but the API design seems very prone to misuse, and I'm not sure there's a net benefit for having Node handle this generation insecurely given the limited use cases in which it's "secure". If you must derive the IV from the key, that should probably be left to the user application -- the official API is not doing it safely either.

Of course, there are other issues with people blindly using CTR besides the IV, but this seems like the biggest one.

[indutny]

@bnoordhuis I think we should print a warning similar to what we do for deprecated APIs. This warning could be disabled by either env or command-line arguments.

[tniessen]

@indutny What about ciphers which do not use an IV such as ECB mode? It is perfectly fine to use createCipher for those. This argument applies to many ciphers, but not to all.

[indutny]

@tniessen I absolutely agree, this has to be applied selectively.

[cbarcenas]

If you must derive the IV from the key, that should probably be left to the user application -- the official API is not doing it safely either.

I agree 100% that IV generation should be left to the API consumer, given that this is exposing mostly low-level cryptographic primitives. Furthermore, there should be a strong warning in the API documentation that IVs are nonces and should be nondeterministic and uniformly random.

The IV-less createCipher API should fail on block modes requiring an IV, rather than relying on this dangerous key/IV derivation logic. Somewhat unrelated, but why does this routine use MD5 in its key-derivation function?

[tniessen]

I think we can agree that the behavior of createCipher is slightly outdated, considering both its behavior regarding the IV and key derivation. However, a convenience interface to a slightly more high-level API would not necessarily be bad, I could think of the same function accepting an options object, maybe slightly similar to options used within the web crypto API. On the other hand, this should not necessarily be part of the core as it can be done in a separate module rather easily.

[bnoordhuis]

Printing warnings on unsafe use isn't a bad idea but -- Socratic method follows -- if the motivation is to protect unskilled programmers against themselves, where do we stop? Should ECB ciphers print a warning? What about obsolete ciphers like DES and RC2?

Should we just turn on FIPS mode unconditionally and call it quits? It does all of the above and more.

[stouset]

@tniessen Arguably, you should need to explicitly "break the seal on the warranty" to use something like ECB mode.

Ideally, the APIs here would be designed to guide the user to the right decision by default. Encryption functions should optionally take an IV, and return an [iv, ciphertext, tag] triplet or an { iv: "...", ciphertext: "...", tag: "..." } object. If the IV provided is undefined, generate one randomly for the user, otherwise use the one provided. Allow the user to opt-out by by making the IV explicitly zeroes.

Likewise, AES-128-GCM should be the default should no cipher be explicitly chosen (this may be the case, I'm just an infosec person leaving a drive-by comment). If the user doesn't provide a tag when decrypting authenticated modes like GCM, fail.

It should not be considered acceptable in 2017 to provide APIs that point a gun at your foot by default, with an easily-missed comment in the documentation recommending that you do more work to aim the gun away before pulling the trigger. Aim the gun away from the foot by default. Default to having the safety on. Require explicit intent to disable the safety and to aim the gun at the foot.

It is not reasonable to expect anyone who is not a cryptographer to use createCipher safely. Making it the simplest, most straightforward entry point to your library is malpractice.

[bnoordhuis]

@stouset Volunteering?