crypto: Deprecate createCipher for createCipheriv #13941

iangcarroll · 2017-06-27T03:24:14Z

This is the start of an implementation of @cbarcenas's proposal and an evolution of #13821, except createCipher ends up being deprecated. I believe the poor KDF in createCipher is also harmful, as it allows arbitrary length keys but only puts them through a round of MD5, hence the removal.

crypto.createCipher is deprecated entirely. Initially, this pull request made createCipher error when passed an IV-dependent cipher, but this has been removed.
crypto.createCipheriv now supports a null IV for algorithms without an IV.

This is my first Node PR, so please let me know if I've done anything wrong.

Raise a deprecation warning when createCipher is called
Implement null IV support into createCipheriv for use with ECB
Expose the old and insecure KDF in a new API method

> let cipher = crypto.createCipher('aes-128-gcm', new Buffer(crypto.randomBytes(16)));
Error: crypto.createCipher() is no longer supported with ciphers that require initialization vectors. Generate a random IV and pass it to crypto.createCipheriv().

> let cipher = crypto.createCipheriv('aes-128-ecb', new Buffer(crypto.randomBytes(16)), null); cipher.update('abc', 'utf-8'); cipher.final('hex');
'e5791e621970d682bb0e353b7bbc6fce'

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, doc

jasnell · 2017-06-27T04:03:12Z

src/node_crypto.cc

+
+  if (encrypt && iv_len != 0) {
+    return env()->ThrowError("crypto.createCipher() is no longer supported with ciphers that require initialization vectors. Generate a random IV and pass it to crypto.createCipheriv().");
+  }


No comments on whether this is something we should do (ping @nodejs/crypto) ... but this is not the correct way of doing it. There are two kinds of deprecations we use: documentation and runtime. A runtime deprecation uses either the util.deprecate() or process.emitWarning('...', 'DeprecationWarning') API to emit a warning at runtime, but the code still continues to operate as it did in every other respect.

My PR intends to immediately prohibit the use of this API for ciphers that require an IV. The deprecation of this API entirely hasn’t been done yet. I’m guessing that would go in lib/crypto.js?

Yep, and that's the point: Our deprecation policy requires that APIs go through a proper deprecation cycle so this PR would not be able to land as is. The first step is to do a docs or runtime deprecation as a semver-major, which would go into Node.js 9.0.0.

We do have provisions for making exceptions for security-related issues but as this is more about protecting users against themselves, it's up for debate whether those provisions apply.

I'm personally leaning towards 'acceptable in node 9' (raising an exception, that is) but I won't hold it against anyone who feels differently.

This prohibits users from using not only counter modes but also cbc modes. I'm not sure if the cbc mode has a security issue in this API.

jasnell · 2017-06-27T04:03:50Z

src/node_crypto.cc

+
+  if (!Buffer::HasInstance(args[2]) && !args[2]->IsNull()) {
+    return env->ThrowTypeError("IV must be a buffer or null");
+  }


why this change?

Because this API will now accept null as a valid IV.

(This means you don’t need to pass an empty Buffer for ciphers where there is no IV)

The better approach here then, would be to do the type checking at the js layer and use the internal/errors API to generate a proper ERR_INVALID_ARG_TYPE error.

Is this about createCipheriv? Why would any sane person come up with the idea to use a function createCipheriv for ciphers where there is no IV?

tniessen · 2017-06-27T09:05:14Z

This might be a start, but I don't think we can land this without the other suggested changes. Summary of those changes:

Deprecate createCipher support for all ciphers which require an IV.
Add generateLegacyKey and generateLegacyIV.

Without the second point implemented, I would not want to land the first one.

crypto.createCipher will no longer function for ciphers that require an initialization vector, and is deprecated entirely.

Why would we want to deprecate it as a whole? It is perfectly fine to use it with ciphers that do not require an IV.

Additionally, we should consider @stouset's #13821 (comment), but for reasons discussed in #13821, we might want to give that function a new name.

iangcarroll · 2017-06-27T14:27:23Z

I didn't mean for this to be immediately landed, just for discussion to happen over what's done so far.

It is perfectly fine to use it with ciphers that do not require an IV.

Not sure I agree. First, the fact it takes a password and not (always) a key buffer is non-standard. But more importantly it derives a key very insecurely. So the two options are essentially to fix it, to the extent we can (i.e. we can't easily add a salt), or pass this duty to the user and force a proper key.

We could change createCipher's signature to require a proper-length Buffer instead of removing it. That does sound more reasonable than pushing it to createCipheriv.

I guess this all depends on whether or not these APIs are supposed to be low or high level, as @cbarcenas has mentioned.

@stouset's proposal probably needs its own PR.

shigeki · 2017-06-27T15:59:26Z

I was conservative to deprecate this API because it is very old API and I could not imagine how much it is affected like Buffer API.

I believe the poor KDF in createCipher is also harmful, as it allows arbitrary length keys but only puts them through a round of MD5, hence the removal.

I agree this. It is a harmful to have md5 KDF at this time. If we can really proceed to make deprecation, I think we had better to deprecate entire createCipher().

tniessen · 2017-06-27T17:14:14Z

I see your point, the current key generation is not too secure... I actually expected a way to change the signature such that users could either specify a password or a key, but I don't see such a solution right now. We might actually have to deprecate the whole function, which is terrible, as createCipher was such a nice name for the function. (Plus, I don't like having to call a function createCipheriv for ciphers without an IV). So I would still be happy if we found another solution.

stouset · 2017-06-28T18:31:13Z

@tniessen Creating ciphers without an IV is inherently dangerous, and in my opinion should not be the easiest way to invoke the library. It's fine for this to be considered a low-level API, but without a very widely-promoted high-level one, a module named crypto is going to be reached for by pretty much anyone searching for how to do it in Node.

ECB is the only mode I can think of that doesn't require instantiation with an IV. Given that, an API like createUnsafeRawBlockCipher is in my eyes significantly better for a function that instantiates ECB, in that it steers people away who have no business touching raw block ciphers.

Once createCipher is deprecated and removed, it can always be brought back with a new API that wraps a much safer approach.

iangcarroll · 2017-07-04T05:58:31Z

I've pushed the deprecation warning for createCipher and Cipher, along with the documentation for the deprecation (not yet for the function). I also made the createCipheriv call easier for ECB mode, so explicitly providing null is no longer required.

One thing I saw is the options argument to createCipher and createCipheriv, which is undocumented but used in a call to LazyTransform. Can this be removed or is it used somewhere?

Also, we could think about using crypto.Cipher() as an alias/replacement for createCipheriv, seeing how its usage has not been documented (though it currently points to createCipher).

I still need to add the legacy methods, more documentation, and tests.

iangcarroll · 2017-07-04T20:40:56Z

generateLegacyKey and generateLegacyIV have been implemented.

Old:

> let cipher = crypto.createCipher('aes-128-ecb', 'password');
undefined
> cipher.update('hello');
<Buffer >
> cipher.final('base64');
'LJ6pZaGs8G7A/W5gW+txkw=='

New:

> var cipher = crypto.createCipheriv("aes-128-ecb", crypto.generateLegacyKey("aes-128-ecb", "password"));
undefined
> cipher.update("hello");
<Buffer >
> cipher.final("base64");
'LJ6pZaGs8G7A/W5gW+txkw=='

iangcarroll · 2017-07-09T22:30:22Z

All tests/linters now pass on my machine. Can someone trigger a CI build? Not sure how to go about that.

tniessen · 2017-07-10T07:41:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9055/

BridgeAR · 2017-08-30T21:44:50Z

This needs a rebase and it seems like not all tests passed.

iangcarroll · 2017-08-31T05:37:30Z

I can finish this up soon. I believe there is a bug (with the tests?) when FIPS mode is used.

BridgeAR · 2017-09-13T21:44:10Z

Ping @iangcarroll

jasnell · 2017-09-13T21:47:18Z

doc/api/deprecations.md

+
+Type: End-of-Life
+
+[`crypto.createCipher()`][] generates keys from strings in an insecure manner, and, when used with a cipher that utilizes an initialization vector, will dangerously re-use initialization vectors. As such, it is immediately marked as End-of-Life when used with ciphers that require initialization vectors, and will be fully removed in a later version.


Please line wrap at <= 80 chars

jasnell · 2017-09-13T21:48:01Z

doc/api/deprecations.md

+<a id="DEP00XX"></a>
+### DEP00XX: crypto.createCipher()
+
+Type: End-of-Life


This should not be automatically moved to End-of-Life. This needs to go through a proper deprecation cycle with either a Documentation-only or Runtime stage.

jasnell · 2017-09-13T21:48:28Z

doc/api/deprecations.md

+
+[`crypto.createCipher()`][] generates keys from strings in an insecure manner, and, when used with a cipher that utilizes an initialization vector, will dangerously re-use initialization vectors. As such, it is immediately marked as End-of-Life when used with ciphers that require initialization vectors, and will be fully removed in a later version.
+
+[`crypto.createCipheriv()`][] should be used in place of [`crypto.createCipher()`][]. Since [`crypto.createCipheriv()`][] will no longer attempt to derive a proper encryption key from a string, you must use a key-derivation function such as [`crypto.pbkdf2()`][] to obtain a valid key if you normally supply a string to [`crypto.createCipher()`][].


Please avoid using the informal pronouns you and your in the documentation.

jasnell

Almost there. I'm -1 on going directly to End-of-life stage. This should sit as a Runtime deprecation first.

iangcarroll · 2017-09-15T16:30:53Z

Can someone please re-run the CI build? I can't reproduce the failing build locally.

Is there any sort of process for deciding the plan for deprecating this? Or is there no hope of directly EOLing it with IV-dependent ciphers?

BridgeAR · 2017-09-24T16:19:33Z

@iangcarroll do you still want to pursue this PR?

iangcarroll · 2017-09-27T16:31:48Z

Yeah, I have local fixes for the CI failures, there's just one more bug I need to fix with current (mis)uses of createCipheriv, particularly that these functions return different things, which I did not expect and don't actually understand at the moment.

> let encrypt = crypto.createCipheriv('BF-ECB', 'SomeRandomBlahz0c5GZVnR', '');
undefined
> encrypt.update('Hello World!', 'ascii', 'hex');
'6d385f424aab0cfb'

> let encrypt = crypto.createCipher('BF-ECB', 'SomeRandomBlahz0c5GZVnR');
undefined
> encrypt.update('Hello World!', 'ascii', 'hex');
'b2941213258d679b'

bnoordhuis · 2017-09-27T18:09:56Z

That's because crypto.createCipher() generates a key with EVP_BytesToKey(EVP_md5()). That is, these two produce identical output:

> crypto.createCipher('bf-ecb', 'pass').update('11223344', 'utf8', 'hex')
'93b5c44c33c68cd1'

> var pass = crypto.createHash('md5').update('pass').digest();
> crypto.createCipheriv('bf-ecb', pass, '').update('11223344', 'utf8', 'hex')
'93b5c44c33c68cd1'

iangcarroll · 2017-09-29T14:15:28Z

I have changed the deprecation so it is now a runtime warning for all calls to createCipher, and added documentation for the changes. Let me know how it looks -- still a bit new to this so might be missing some stuff.

tniessen

Still not entirely happy with the need to change this, even though I understand the reasoning.

tniessen · 2017-09-29T14:29:31Z

benchmark/crypto/cipher-stream.js

+  const iv = crypto.generateLegacyIV(conf.cipher, alice_secret);
+
+  var alice_cipher = crypto.createCipheriv(conf.cipher, key, iv);
+  var bob_cipher = crypto.createDecipheriv(conf.cipher, key, iv);


Use const instead of var.

tniessen · 2017-09-29T14:35:55Z

doc/api/crypto.md

+- `key` {string | Buffer | TypedArray | DataView}
+
+Creates and returns a [Buffer][`Buffer`] object, with the given `algorithm` and
+`key`.


Your docs seem to miss information about what the functions actually do.

tniessen · 2017-09-29T14:41:29Z

doc/api/deprecations.md

+
+If the previous key-derivation is required for backward compatiability, the new
+APIs [`crypto.generateLegacyKey()`][] and [`crypto.generateLegacyIV()`][] have
+been added.


Not really a causality.

tniessen · 2017-09-29T14:42:45Z

doc/api/deprecations.md

+
+Additionally, for ciphers that require an initialization vector, a proper-length
+initialization vector must be passed to [`crypto.createCipheriv()`][].
+Initialization vectors must never be re-used, especially in modes such as


must → should. The word must indicates that calling the function with such parameters leads to undefined behavior or an error, while this is actually just a strong recommendation.

tniessen · 2017-09-29T14:44:56Z

test/parallel/test-crypto.js

@@ -135,6 +135,10 @@ testImmutability(crypto.getCurves);

 // Regression tests for #5725: hex input that's not a power of two should
 // throw, not assert in C++ land.
+if (common.hasFipsCrypto) {
+
+}


I assume you are still working on this part.

iangcarroll · 2017-10-08T23:44:36Z

Updated. I'll take care of the merge conflicts shortly.

iangcarroll · 2017-10-25T02:39:35Z

It's going to be a couple weeks before I can wrap this up. But if there are things that need to be fixed please let me know in the interim.

tniessen · 2017-10-26T16:09:13Z

@iangcarroll Let us know if you need any help.

BridgeAR · 2017-11-22T11:39:31Z

@iangcarroll do you have the time to pursue this further? :-)

BridgeAR · 2017-12-05T23:51:48Z

Ping @iangcarroll

tniessen · 2017-12-08T17:11:22Z

I can take over this if necessary. By the way, I would like to reconsider whether we really want to include generateLegacy* in core (adding new APIs to support deprecated behavior hardly makes sense).

bnoordhuis · 2017-12-08T18:06:31Z

It's not necessary. I uploaded a script last week for computing legacy keys that can be made into a npm module if so desired.

tniessen · 2017-12-08T19:51:36Z

It's not necessary. I uploaded a script last week for computing legacy keys that can be made into a npm module if so desired.

Thanks @bnoordhuis, that's exactly what I had in mind. The crypto API is already quite bloated in my opinion, so I would prefer that very much.

BridgeAR · 2018-01-12T15:26:45Z

@tniessen do you still would like to work on this? It would be great if you could open a new PR accordingly. I also think your approach not to add a new API is good.

BridgeAR · 2018-02-07T02:37:44Z

Closing due to no further progress.

@iangcarroll thanks a lot for your work! I think it would really be great to get this in and I hope you or someone else might pick this up again.

@tniessen @bnoordhuis I guess one of you two might wants to pick it up?

tniessen · 2018-02-07T14:09:32Z

@BridgeAR I will start working on an alternative soon.

Ian Carroll added 2 commits June 26, 2017 23:06

crypto: Deprecate createCipher for createCipheriv

cc8976b

crypto: remove unused mode variable

3d0b5a5

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Jun 27, 2017

jasnell requested changes Jun 27, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 27, 2017

crypto: mark createCipher as deprecated

1e810d0

crypto: add legacy KDF methods

8f2b81f

crypto: update tests for createCipher

7f913ec

Ian Carroll added 2 commits July 9, 2017 18:32

crypto: fix IV buffer length

b7abe64

crypto: fix removal error

b16018a

iangcarroll mentioned this pull request Aug 9, 2017

crypto.createCipher should not work with AES-CTR #13801

Closed

tniessen mentioned this pull request Sep 11, 2017

Exposing OpenSSL RSA KeyGen #15116

Closed

jasnell reviewed Sep 13, 2017

View reviewed changes

jasnell requested changes Sep 13, 2017

View reviewed changes

Ian Carroll added 4 commits September 29, 2017 09:15

crypto: Fix createCipheriv with null IVs and FIPS mode.

39a2112

crypto: fix style issues

d0f2778

doc: add crypto.createCipher deprecation information

14e0566

crypto: downgrade createCipher deprecation to runtime

3f0fdb2

tniessen reviewed Sep 29, 2017

View reviewed changes

Ian Carroll added 2 commits October 8, 2017 19:38

crypto: improve documentation of createCipher changes

b0fe314

crypto: improve createCipher testing

0025983

tniessen mentioned this pull request Dec 7, 2017

Additional validation of tag length in AES GCM decryption #17523

Closed

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

liuqipeng417 mentioned this pull request Jan 15, 2018

#7 createCipheriv 里的 aes-256-ecb 加密使用注意事项 liuqipeng417/blog#7

Open

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

ChALkeR added the security Issues and PRs related to security. label Jan 27, 2018

BridgeAR closed this Feb 7, 2018

tniessen mentioned this pull request Mar 14, 2018

crypto: doc-only deprecate createCipher/Decipher #19343

Closed

2 tasks


		Type: End-of-Life

		[`crypto.createCipher()`][] generates keys from strings in an insecure manner, and, when used with a cipher that utilizes an initialization vector, will dangerously re-use initialization vectors. As such, it is immediately marked as End-of-Life when used with ciphers that require initialization vectors, and will be fully removed in a later version.


		[`crypto.createCipher()`][] generates keys from strings in an insecure manner, and, when used with a cipher that utilizes an initialization vector, will dangerously re-use initialization vectors. As such, it is immediately marked as End-of-Life when used with ciphers that require initialization vectors, and will be fully removed in a later version.

		[`crypto.createCipheriv()`][] should be used in place of [`crypto.createCipher()`][]. Since [`crypto.createCipheriv()`][] will no longer attempt to derive a proper encryption key from a string, you must use a key-derivation function such as [`crypto.pbkdf2()`][] to obtain a valid key if you normally supply a string to [`crypto.createCipher()`][].

crypto: Deprecate createCipher for createCipheriv #13941

crypto: Deprecate createCipher for createCipheriv #13941

Conversation

iangcarroll commented Jun 27, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

tniessen commented Jun 27, 2017

iangcarroll commented Jun 27, 2017

shigeki commented Jun 27, 2017

tniessen commented Jun 27, 2017

stouset commented Jun 28, 2017 • edited Loading

iangcarroll commented Jul 4, 2017

iangcarroll commented Jul 4, 2017

iangcarroll commented Jul 9, 2017

tniessen commented Jul 10, 2017

BridgeAR commented Aug 30, 2017

iangcarroll commented Aug 31, 2017

BridgeAR commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

iangcarroll commented Sep 15, 2017 • edited Loading

BridgeAR commented Sep 24, 2017

iangcarroll commented Sep 27, 2017

bnoordhuis commented Sep 27, 2017 • edited Loading

iangcarroll commented Sep 29, 2017

tniessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iangcarroll commented Oct 8, 2017

iangcarroll commented Oct 25, 2017

tniessen commented Oct 26, 2017

BridgeAR commented Nov 22, 2017

BridgeAR commented Dec 5, 2017

tniessen commented Dec 8, 2017

bnoordhuis commented Dec 8, 2017

tniessen commented Dec 8, 2017

BridgeAR commented Jan 12, 2018

BridgeAR commented Feb 7, 2018

tniessen commented Feb 7, 2018

iangcarroll commented Jun 27, 2017 •

edited

Loading

tniessen Jun 27, 2017 •

edited

Loading

stouset commented Jun 28, 2017 •

edited

Loading

jasnell Sep 13, 2017 •

edited

Loading

iangcarroll commented Sep 15, 2017 •

edited

Loading

bnoordhuis commented Sep 27, 2017 •

edited

Loading