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?

[shigeki]

I think that emitting warning is a good idea. This is a side effect of the current crypto.createCipher that it kindly takes care of missing IV with the fixed value derived from password. It is better to avoid security risks of nonce reuse due to its API design.
As of the current, CTR, GCM and CCM should be checked. OCB and ChaCha20-Poly1350 will be included in the future. Timely, AES-SIV is coming in openssl/openssl#3540 for the safe use of counter mode.

[tqbf]

If there's no safe way to use a crypto API, why would you retain it? There's no compat issue here with any mainstream system, and any system that depends on this behavior has a grave security flaw.

[stouset]

@bnoordhuis Volunteering my advice on the design of cryptographic APIs, sure.

@tqbf My point exactly. It's not wrong to expose the raw AES block cipher, or to allow an all-zero IV. Sometimes your users have to integrate with existing broken systems, or are genuinely qualified to build systems out of these these sharp-edged tools. But these are specialized use-cases, and should not be the easiest or default way of using the API.

[bnoordhuis]

That's infosec for you...

[stouset]

My sincerest apologies for contributing to the extent that I'm qualified. I can assure you it will never happen again.

Perhaps if you'd had the foresight to make this mistake in a project using Rust, Ruby, C, go, or one of the other languages I'm actually comfortable committing crypto-related code in we could have actually made a difference here.

[bnoordhuis]

I hope you can see it from my point of view. Someone posts a strongly opinionated comment and then when you ask them to follow up, no dice.

It's not just you. Security-related issues have a habit of attracting random people with Big Opinions, generating lots of heated discussion, and then... nothing. Off to another bug tracker to fight the good fight, presumably. It's frustrating.

[tqbf]

It is important that Node.js not make it easy to instantiate CTR mode with a deterministic nonce/IV. That's not a "Big Opinion"; it's a cryptographic fact. It is critically important that stream cipher keystreams never repeat, and deterministic nonces/IVs lead to repeated keystreams. This is probably one of the oldest practical vulnerabilities in cryptography.

[cbarcenas]

I have an alternate proposal for a crypto module API change, see here.

[stouset]

I can assure you it's equally as frustrating for cryptographers to discover yet another open source project with a footgun crypto API. And further frustrating when the response to this being pointed out is that there's a warning in the documentation, there are surely users somewhere who want a footgun, and if we just log a warning everything will be fine.

I am happy to contribute in the form of API design. If you don't think that's valuable, consider that the current API has resulted in (naïvely) 350 instances of misuse and 100 instances of correct use in public GitHub repos. Fewer than 25% of developers have used the current API safely, and that's generously assuming all 100 createCipheriv are generating IVs correctly.

Edit: Modified the search terms to hopefully get a more representative result.

[bnoordhuis]

Not to undermine your point but that looks like the same two or three files cloned 2,500x.

[stouset]

Updated the searches. There's still some copy-pasted files, but that doesn't necessarily speak against my point. Vulnerable code cloned to multiple repos is just as bad as vulnerable code hand-rolled in those same repos.

[gibfahn]

the response to this being pointed out is that there's a warning in the documentation,

That's not really accurate. The response to there should probably be a warning about this in the function's documentation. was there is, not that a warning solves all issues.

I can assure you it's equally as frustrating for cryptographers to discover yet another open source project with a footgun crypto API. And further frustrating when the response to this being pointed out is that there's a warning in the documentation,

The real response to these things is "could you submit a PR?" Node could really do with some more people with the expertise to look at and fix crypto issues.

[ss23]

As pointed out by other people, there is no way to use the current implementation of crypto.createCipher safely.
Would the developers accept a PR removing it completely?

[iangcarroll]

@ss23: See #13941, which does that. Just need to finish it up as it seems to break with FIPS mode enabled.