crypto: move createCipher to runtime deprecation by tniessen · Pull Request #22089 · nodejs/node

tniessen · 2018-08-02T15:34:53Z

createCipher is still being used, but it seems appropriate to have a runtime deprecation in one of the next major releases. Reasons include:

It uses static IVs: Encrypting the same message with the same password will always yield the same ciphertext. While this is generally considered bad practice and can lead to severe security issues on its own, it also means that any counter-mode cipher is more or less useless when used with createCipher, see crypto: warn if counter mode used in createCipher #13821.
Its KDF is non-configurable and doesn't use a salt, meaning that the same password always results in the same key. It also allows brute-force attacks since computing MD5 with a single iteration is incredibly fast on modern processors / GPUs.
There are instances where using createCipher is relatively secure, but its simple API makes it especially appealing to people who are new to cryptography, possibly leading to immense security risks.
It has been documentation-only deprected since node 10, see crypto: doc-only deprecate createCipher/Decipher #19343.
If people want to have this functionality for whatever reason, it is still possible by using createCipheriv (thanks to crypto: allow passing null as IV unless required #18644) and manually deriving the static key / IV. If people are using this for legitimate reasons, we can still un-deprecate it.

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

nodejs-github-bot · 2018-08-02T15:34:57Z

@tniessen build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/403/pipeline

tniessen · 2018-08-02T15:39:07Z

cc @nodejs/tsc @nodejs/crypto

TimothyGu

LGTM with a nit.

/cc @nodejs/security

TimothyGu · 2018-08-02T17:01:21Z

test/parallel/test-process-emit-warning-from-native.js


  // Emits regular warning expected by expectWarning()
-  crypto.createCipher('aes-256-gcm', key);
+  crypto.createCipher('aes-256-cbc', key);


This test checks EmitWarning in C++ code, evident from the file name. Instead of changing the message we should look for another warning.

Oh right, I can make this work either way, thanks for the hint!

TimothyGu

LGTM.

Trott · 2018-08-02T21:45:45Z

@nodejs/security-wg

tniessen · 2018-08-04T23:02:41Z

Full CI: https://ci.nodejs.org/job/node-test-pull-request/16199/
Resumed CI: https://ci.nodejs.org/job/node-test-pull-request/16205/

PR-URL: #22089 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

tniessen · 2018-08-05T21:19:12Z

Landed in 933d8eb with TSC approvals from @TimothyGu, @jasnell and @cjihrig. Thank you for reviewing, everyone.

The current documentation clearly states that createCipher() and createDecipher() should not be used with ciphers in counter mode, but (1) this is an understatement, and (2) these functions are (semantically) insecure for ciphers in any other supported block cipher mode as well. Semantic security requires IND-CPA, but a deterministic cipher with fixed key and IV, such as those generated by these functions, does not fulfill IND-CPA. Are there justified use cases for createCipher() and createDecipher()? Yes and no. The only case in which these functions can be used in a semantically secure manner arises only when the password argument is not actually a password but rather a random or pseudo-random sequence that is unpredictable and that is never reused (e.g., securely derived from a password with a proper salt). Insofar, it is possible to use these APIs without immediately creating a vulnerability. However, - any application that manages to fulfill this requirement should also be able to fulfill the similar requirements of crypto.createCipheriv() and those of crypto.createDecipheriv(), which give much more control over key and initialization vector, and - the MD5-based key derivation step generally does not help and might even reduce the overall security due to its many weaknesses. Refs: nodejs#13821 Refs: nodejs#19343 Refs: nodejs#22089

The current documentation clearly states that createCipher() and createDecipher() should not be used with ciphers in counter mode, but (1) this is an understatement, and (2) these functions are (semantically) insecure for ciphers in any other supported block cipher mode as well. Semantic security requires IND-CPA, but a deterministic cipher with fixed key and IV, such as those generated by these functions, does not fulfill IND-CPA. Are there justified use cases for createCipher() and createDecipher()? Yes and no. The only case in which these functions can be used in a semantically secure manner arises only when the password argument is not actually a password but rather a random or pseudo-random sequence that is unpredictable and that is never reused (e.g., securely derived from a password with a proper salt). Insofar, it is possible to use these APIs without immediately creating a vulnerability. However, - any application that manages to fulfill this requirement should also be able to fulfill the similar requirements of crypto.createCipheriv() and those of crypto.createDecipheriv(), which give much more control over key and initialization vector, and - the MD5-based key derivation step generally does not help and might even reduce the overall security due to its many weaknesses. Refs: #13821 Refs: #19343 Refs: #22089 PR-URL: #44538 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

The current documentation clearly states that createCipher() and createDecipher() should not be used with ciphers in counter mode, but (1) this is an understatement, and (2) these functions are (semantically) insecure for ciphers in any other supported block cipher mode as well. Semantic security requires IND-CPA, but a deterministic cipher with fixed key and IV, such as those generated by these functions, does not fulfill IND-CPA. Are there justified use cases for createCipher() and createDecipher()? Yes and no. The only case in which these functions can be used in a semantically secure manner arises only when the password argument is not actually a password but rather a random or pseudo-random sequence that is unpredictable and that is never reused (e.g., securely derived from a password with a proper salt). Insofar, it is possible to use these APIs without immediately creating a vulnerability. However, - any application that manages to fulfill this requirement should also be able to fulfill the similar requirements of crypto.createCipheriv() and those of crypto.createDecipheriv(), which give much more control over key and initialization vector, and - the MD5-based key derivation step generally does not help and might even reduce the overall security due to its many weaknesses. Refs: nodejs#13821 Refs: nodejs#19343 Refs: nodejs#22089 PR-URL: nodejs#44538 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

The current documentation clearly states that createCipher() and createDecipher() should not be used with ciphers in counter mode, but (1) this is an understatement, and (2) these functions are (semantically) insecure for ciphers in any other supported block cipher mode as well. Semantic security requires IND-CPA, but a deterministic cipher with fixed key and IV, such as those generated by these functions, does not fulfill IND-CPA. Are there justified use cases for createCipher() and createDecipher()? Yes and no. The only case in which these functions can be used in a semantically secure manner arises only when the password argument is not actually a password but rather a random or pseudo-random sequence that is unpredictable and that is never reused (e.g., securely derived from a password with a proper salt). Insofar, it is possible to use these APIs without immediately creating a vulnerability. However, - any application that manages to fulfill this requirement should also be able to fulfill the similar requirements of crypto.createCipheriv() and those of crypto.createDecipheriv(), which give much more control over key and initialization vector, and - the MD5-based key derivation step generally does not help and might even reduce the overall security due to its many weaknesses. Refs: #13821 Refs: #19343 Refs: #22089 PR-URL: #44538 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

The current documentation clearly states that createCipher() and createDecipher() should not be used with ciphers in counter mode, but (1) this is an understatement, and (2) these functions are (semantically) insecure for ciphers in any other supported block cipher mode as well. Semantic security requires IND-CPA, but a deterministic cipher with fixed key and IV, such as those generated by these functions, does not fulfill IND-CPA. Are there justified use cases for createCipher() and createDecipher()? Yes and no. The only case in which these functions can be used in a semantically secure manner arises only when the password argument is not actually a password but rather a random or pseudo-random sequence that is unpredictable and that is never reused (e.g., securely derived from a password with a proper salt). Insofar, it is possible to use these APIs without immediately creating a vulnerability. However, - any application that manages to fulfill this requirement should also be able to fulfill the similar requirements of crypto.createCipheriv() and those of crypto.createDecipheriv(), which give much more control over key and initialization vector, and - the MD5-based key derivation step generally does not help and might even reduce the overall security due to its many weaknesses. Refs: nodejs/node#13821 Refs: nodejs/node#19343 Refs: nodejs/node#22089 PR-URL: nodejs/node#44538 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

crypto: move createCipher to runtime deprecation

d291a1b

nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Aug 2, 2018

tniessen added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 2, 2018

TimothyGu approved these changes Aug 2, 2018

View reviewed changes

yhwang approved these changes Aug 2, 2018

View reviewed changes

fixup! crypto: move createCipher to runtime deprecation

a17f907

TimothyGu approved these changes Aug 2, 2018

View reviewed changes

lpinca approved these changes Aug 3, 2018

View reviewed changes

ryzokuken approved these changes Aug 3, 2018

View reviewed changes

jasnell approved these changes Aug 3, 2018

View reviewed changes

tniessen added this to the 11.0.0 milestone Aug 4, 2018

tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 4, 2018

cjihrig approved these changes Aug 5, 2018

View reviewed changes

tniessen closed this Aug 5, 2018

tniessen added deprecations Issues and PRs related to deprecations. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 7, 2018

tniessen mentioned this pull request Sep 6, 2022

doc: emphasize that createCipher is never secure #44538

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

crypto: move createCipher to runtime deprecation#22089

crypto: move createCipher to runtime deprecation#22089
tniessen wants to merge 2 commits intonodejs:masterfrom
tniessen:crypto-runtime-deprecate-createcipher

tniessen commented Aug 2, 2018

Uh oh!

nodejs-github-bot commented Aug 2, 2018

Uh oh!

tniessen commented Aug 2, 2018

Uh oh!

TimothyGu left a comment

Uh oh!

TimothyGu Aug 2, 2018

Uh oh!

tniessen Aug 2, 2018

Uh oh!

TimothyGu left a comment

Uh oh!

Trott commented Aug 2, 2018

Uh oh!

tniessen commented Aug 4, 2018 •

edited

Loading

Uh oh!

tniessen commented Aug 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

Conversation

tniessen commented Aug 2, 2018

Checklist

Uh oh!

nodejs-github-bot commented Aug 2, 2018

Uh oh!

tniessen commented Aug 2, 2018

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu Aug 2, 2018

Choose a reason for hiding this comment

Uh oh!

tniessen Aug 2, 2018

Choose a reason for hiding this comment

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 2, 2018

Uh oh!

tniessen commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Aug 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

tniessen commented Aug 4, 2018 •

edited

Loading