crypto: allow passing null as IV unless required #18644

tniessen · 2018-02-08T15:05:42Z

There has been a lot of discussion around deprecating create(Dec|C)ipher in favor of create(Dec|C)ipheriv. This PR is a preparation which allows to pass null as the IV for ciphers that do not use an IV (e.g. ECB) instead of having to pass an empty buffer. Note that the parameter cannot currently be ommitted, which I think is fair considering that the IV is part of the name of the function.

This was separated from the actual deprecation of create(Dec|C)ipher to allow backporting and to make reviewing a little easier.

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

tniessen · 2018-02-08T15:08:46Z

src/node_crypto.cc

-  const char* iv_buf = Buffer::Data(args[2]);
+  ssize_t iv_len;
+  const char* iv_buf;
+  if (args[2]->IsNullOrUndefined()) {


This could as well be ->IsNull() for consistency, no strong opinion here.

I'd go with IsNull() for consistency with the checks in JS land.

bnoordhuis · 2018-02-08T18:13:51Z

src/node_crypto.cc

-  const char* iv_buf = Buffer::Data(args[2]);
+  ssize_t iv_len;
+  const char* iv_buf;
+  if (args[2]->IsNullOrUndefined()) {


I'd go with IsNull() for consistency with the checks in JS land.

bnoordhuis · 2018-02-08T18:25:46Z

src/node_crypto.cc

+  }
+
+  // Throw if an IV was passed which does not match the cipher's fixed IV length
+  if (is_gcm_mode == false && has_iv && iv_len != expected_iv_len) {


At least in theory we could end up calling EVP_CIPHER_CTX_ctrl(EVP_CTRL_GCM_SET_IVLEN) on line 3116 with a value of -1.

That's currently harmless, openssl returns an error, but I'm not sure if we have tests that exercise that particular code path. Perhaps it's best to add a CHECK(has_iv) there.

That case should be handled by !has_iv && expected_iv_len != 0 (the expected_iv_len is 12 for GCM). Where should the CHECK be, after EVP_CIPHER_CTX_ctrl(EVP_CTRL_GCM_SET_IVLEN) fails?

I'd do it right before the call to EVP_CIPHER_CTX_ctrl().

tniessen · 2018-02-09T15:32:47Z

Light CI #243 passes, let's see: https://ci.nodejs.org/job/node-test-pull-request/13061/

PR-URL: nodejs#18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR · 2018-02-10T14:43:38Z

Landed in 38bac42 🎉

PR-URL: nodejs#18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

tniessen · 2018-02-10T15:06:18Z

@BridgeAR That was fast, it's barely been 48 hours and it's a weekend.

BridgeAR · 2018-02-10T15:10:08Z

@tniessen true. I am going to add a check in my script to break in case the time is not done / no fast track is set.

MylesBorins · 2018-02-21T00:43:30Z

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

PR-URL: nodejs#18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

tniessen · 2018-03-14T14:25:21Z

@MylesBorins Sorry, must have missed the notification. #19347

Backport-PR-URL: #19347 PR-URL: #18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * assert: - From now on all error messages produced by `assert` in strict mode will produce a error diff. (Ruben Bridgewater) #17615 - From now on it is possible to use a validation object in throws instead of the other possibilities. (Ruben Bridgewater) #17584 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * fs: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * tty: - Add getColorDepth function to determine if terminal supports colors (Ruben Bridgewater) #17615 * util: - add util.inspect compact option (Ruben Bridgewater) #17576 * **Added new collaborators** - [watson](https://github.com/watson) Thomas Watson PR-URL: #19428

PR-URL: nodejs#18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-08-07T18:56:54Z

I've backported to v8.x

Please lmk if it shouldn't have landed and I'll back it out

Backport-PR-URL: #19347 PR-URL: #18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) nodejs#18633 - remove runtime deprecation (Ali Ijaz Sheikh) nodejs#19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 * cluster: - add cwd to cluster.settings (cjihrig) nodejs#18399 - support windowsHide option for workers (Todd Wong) nodejs#17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) nodejs#18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) nodejs#21592 - upgrade libuv to 1.19.2 (cjihrig) nodejs#18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) nodejs#21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) nodejs#18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) nodejs#19408 * http, http2: - add options to http.createServer() (Peter Marton) nodejs#15752 - add 103 Early Hints status code (Yosuke Furukawa) nodejs#16644 - add http fallback options to .createServer (Peter Marton) nodejs#15752 * n-api: - take n-api out of experimental (Michael Dawson) nodejs#19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) nodejs#18087 * src: - add public API for managing NodePlatform (Cheng Zhao) nodejs#16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) nodejs#17600 - node internals' postmortem metadata (Matheus Marchini) nodejs#14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) nodejs#19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) nodejs#18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) nodejs#18186 PR-URL: nodejs#21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.4.1 (Kat Marchán) #22591 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

tniessen added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 8, 2018

tniessen requested a review from bnoordhuis February 8, 2018 15:05

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 Feb 8, 2018

tniessen commented Feb 8, 2018

View reviewed changes

bnoordhuis approved these changes Feb 8, 2018

View reviewed changes

crypto: allow passing null as IV unless required

781d7e7

tniessen force-pushed the crypto-allow-null-iv branch from 30efdad to 781d7e7 Compare February 9, 2018 02:10

add CHECK for GCM

5d4d3ff

bnoordhuis approved these changes Feb 9, 2018

View reviewed changes

jasnell approved these changes Feb 9, 2018

View reviewed changes

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

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 10, 2018

crypto: allow passing null as IV unless required

1d3fc52

PR-URL: nodejs#18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR closed this Feb 10, 2018

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 10, 2018

crypto: allow passing null as IV unless required

38bac42

PR-URL: nodejs#18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

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

tniessen added a commit to tniessen/node that referenced this pull request Mar 14, 2018

crypto: allow passing null as IV unless required

0a776f3

PR-URL: nodejs#18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

tniessen mentioned this pull request Mar 14, 2018

(v9.x backport) crypto: allow passing null as IV unless required #19347

Closed

MylesBorins pushed a commit that referenced this pull request Mar 20, 2018

crypto: allow passing null as IV unless required

03c321a

Backport-PR-URL: #19347 PR-URL: #18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

targos mentioned this pull request Mar 20, 2018

v9.9.0 proposal #19428

Merged

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

crypto: allow passing null as IV unless required

1641caa

PR-URL: nodejs#18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

tniessen mentioned this pull request Aug 2, 2018

crypto: move createCipher to runtime deprecation #22089

Closed

4 tasks

MylesBorins pushed a commit that referenced this pull request Aug 7, 2018

crypto: allow passing null as IV unless required

c1c6253

Backport-PR-URL: #19347 PR-URL: #18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Aug 7, 2018

crypto: allow passing null as IV unless required

9790c09

Backport-PR-URL: #19347 PR-URL: #18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Aug 7, 2018

crypto: allow passing null as IV unless required

7be8f16

Backport-PR-URL: #19347 PR-URL: #18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Aug 9, 2018

crypto: allow passing null as IV unless required

802e26b

Backport-PR-URL: #19347 PR-URL: #18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

MylesBorins pushed a commit that referenced this pull request Aug 16, 2018

crypto: allow passing null as IV unless required

fc34f5c

Backport-PR-URL: #19347 PR-URL: #18644 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: allow passing null as IV unless required #18644

crypto: allow passing null as IV unless required #18644

tniessen commented Feb 8, 2018 •

edited

Loading

tniessen Feb 8, 2018

bnoordhuis Feb 8, 2018

bnoordhuis Feb 8, 2018

bnoordhuis Feb 8, 2018

tniessen Feb 9, 2018

bnoordhuis Feb 9, 2018

tniessen commented Feb 9, 2018

BridgeAR commented Feb 10, 2018 •

edited

Loading

tniessen commented Feb 10, 2018

BridgeAR commented Feb 10, 2018

MylesBorins commented Feb 21, 2018

tniessen commented Mar 14, 2018

MylesBorins commented Aug 7, 2018

crypto: allow passing null as IV unless required #18644

crypto: allow passing null as IV unless required #18644

Conversation

tniessen commented Feb 8, 2018 • edited Loading

Checklist

Affected core subsystem(s)

tniessen Feb 8, 2018

Choose a reason for hiding this comment

bnoordhuis Feb 8, 2018

Choose a reason for hiding this comment

bnoordhuis Feb 8, 2018

Choose a reason for hiding this comment

bnoordhuis Feb 8, 2018

Choose a reason for hiding this comment

tniessen Feb 9, 2018

Choose a reason for hiding this comment

bnoordhuis Feb 9, 2018

Choose a reason for hiding this comment

tniessen commented Feb 9, 2018

BridgeAR commented Feb 10, 2018 • edited Loading

tniessen commented Feb 10, 2018

BridgeAR commented Feb 10, 2018

MylesBorins commented Feb 21, 2018

tniessen commented Mar 14, 2018

MylesBorins commented Aug 7, 2018

tniessen commented Feb 8, 2018 •

edited

Loading

BridgeAR commented Feb 10, 2018 •

edited

Loading