crypto: allow to restrict valid GCM tag length by tniessen · Pull Request #20039 · nodejs/node

tniessen · 2018-04-15T00:52:07Z

This change allows users to restrict accepted GCM authentication tag lengths to a single value as proposed in #17523 (comment). This relies on the API changes introduced in #18138.

Fixes: #17523

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

This change allows users to restrict accepted GCM authentication tag lengths to a single value. Fixes: nodejs#17523

tniessen · 2018-04-15T00:52:55Z

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

Trott · 2018-04-15T00:58:55Z

@nodejs/crypto

jasnell · 2018-04-16T16:07:46Z

src/node_crypto.cc

+      char msg[50];
+      snprintf(msg, sizeof(msg),
+          "Invalid GCM authentication tag length: %u", auth_tag_len);
+      env()->ThrowError(msg);


Would be ideal if the error can be thrown in JS but that's not necessary for landing.

I hope to have some time between the release of node 10 and node 11 to refactor the whole cipher API to use the new error system, this PR aims for consistency with the rest :)

yhwang · 2018-04-18T18:02:50Z

src/node_crypto.cc

@@ -2911,7 +2930,8 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
  unsigned int tag_len = Buffer::Length(args[0]);
  const int mode = EVP_CIPHER_CTX_mode(cipher->ctx_);
  if (mode == EVP_CIPH_GCM_MODE) {


@tniessen I know your change is for GCM. I just think what's the minimal check we should do for CCM? Something like:

(cipher->auth_tag_len_ >= 0 && cipher->auth_tag_len_ != tag_len) || tag_len > EVP_GCM_TLS_TAG_LEN

@yhwang This is basically the same problem as we discussed in #20040. I will fix it in that PR so this can remain semver-minor. This patch should work either way, right?

yhwang

LGTM

tniessen · 2018-04-21T09:05:33Z

It would be great if @bnoordhuis could take a look at this before landing.

bnoordhuis · 2018-04-21T09:27:36Z

src/node_crypto.cc



+static bool IsValidGCMTagLength(int tag_len) {
+  return tag_len == 4 || tag_len == 8 || (tag_len >= 12 && tag_len <= 16);


Micro-nit: superfluous parens.

bnoordhuis · 2018-04-21T09:29:32Z

src/node_crypto.cc

  const int mode = EVP_CIPHER_CTX_mode(cipher->ctx_);
  if (mode == EVP_CIPH_GCM_MODE) {
-    if (tag_len > 16 || (tag_len < 12 && tag_len != 8 && tag_len != 4)) {
+    if ((cipher->auth_tag_len_ >= 0 && cipher->auth_tag_len_ != tag_len) ||


bnoordhuis · 2018-04-21T09:32:02Z

src/node_crypto.h

  const CipherKind kind_;
  bool auth_tag_set_;
-  unsigned int auth_tag_len_;
+  int auth_tag_len_;


I'd prefer using some unsigned sentinel value, e.g.:

static const unsigned kNoAuthTag = static_cast<unsigned>(-1);

tniessen · 2018-04-21T13:28:22Z

Thanks @bnoordhuis, hope it is okay now! :)

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

bnoordhuis

LGTM. It would be slightly nicer still to convert to unsigned / the sentinel immediately when converting from v8::Int32 to int.

This change allows users to restrict accepted GCM authentication tag lengths to a single value. PR-URL: #20039 Fixes: #17523 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

tniessen · 2018-04-22T23:59:33Z

Landed in 358d8ff. I will address your suggestion in another PR, @bnoordhuis.

jasnell · 2018-04-23T14:22:32Z

This does not land cleanly in v10.x-staging because it appears to depend on one of the semver-major's that landed last week that will not be making it in to 10.0.0. A backport PR may allow it to land in 10.x but it won't make it in time for 10.0.0

Refs: nodejs#20039

Add parentheses, like my compiler suggests. In the `IsValidGCMTagLength()` case I’m pretty sure that this grouping is the correct one, because it matches the earlier code; in the other case it would be good to have explicit confirmation. Refs: nodejs#20039

Backport-PR-URL: #20706 PR-URL: #20225 Refs: #20039 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

The authTagLength option can now be used to produce GCM authentication tags with a specific length. Backport-PR-URL: #20706 PR-URL: #20235 Refs: #20039 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Notable Changes: * **addons**: - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668) * **assert**: - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485) * **crypto**: - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039) * **http**: - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611) * Embedder support: - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (helloshuangzi) [#20639](#20639) - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377) - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541) * **timers**: - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298) PR-URL: #20724

Notable Changes: * **addons**: - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668) * **assert**: - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485) * **crypto**: - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039) * **http**: - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611) * Embedder support: - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (helloshuangzi) [#20639](#20639) - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377) - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541) * **esm**: - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) [#20403](#20403) * **timers**: - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298) PR-URL: #20724

Notable Changes: * **addons**: - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668) * **assert**: - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485) * **crypto**: - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039) * **http**: - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611) * Embedder support: - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (Allen Yonghuang Wang) [#20639](#20639) - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377) - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541) * **esm**: - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) [#20403](#20403) * **timers**: - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298) PR-URL: #20724

* addons: - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) #20668 * assert: - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) #20485 * crypto: - The `authTagLength` option has been made more flexible (Tobias Nießen) #20235) #20039 * esm: - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) #20403 * http: - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) #20075 #20611 * module: - add --preserve-symlinks-main (David Goldstein) #19911 * timers: - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) #20298 * Embedder support: - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (Allen Yonghuang Wang) #20639 - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) #19377 - Support for multi-threaded embedding has been improved. (Anna Henningsen) #20542 #20539 #20541 PR-URL: #20724

Given that #17825 and #20039 have landed on master, this statement is no longer true. PR-URL: #21445 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

crypto: allow to restrict valid GCM tag length

c2f1870

This change allows users to restrict accepted GCM authentication tag lengths to a single value. Fixes: nodejs#17523

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

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 Apr 15, 2018

tniessen added dont-land-on-v4.x labels Apr 15, 2018

tniessen requested a review from bnoordhuis April 15, 2018 11:58

jasnell approved these changes Apr 16, 2018

View reviewed changes

jasnell reviewed Apr 16, 2018

View reviewed changes

yhwang reviewed Apr 18, 2018

View reviewed changes

yhwang approved these changes Apr 18, 2018

View reviewed changes

bnoordhuis approved these changes Apr 21, 2018

View reviewed changes

fixup! crypto: allow to restrict valid GCM tag length

d4520f8

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

bnoordhuis approved these changes Apr 22, 2018

View reviewed changes

tniessen closed this Apr 22, 2018

tniessen mentioned this pull request Apr 23, 2018

src: fix node_crypto.cc compiler warnings #20216

Closed

2 tasks

tniessen added a commit to tniessen/node that referenced this pull request Apr 23, 2018

crypto: use kNoAuthTagLength in InitAuthenticated

c9126f1

Refs: nodejs#20039

tniessen mentioned this pull request Apr 23, 2018

crypto: use kNoAuthTagLength in InitAuthenticated #20225

Closed

2 tasks

addaleax mentioned this pull request May 14, 2018

v10.2.0 proposal #20724

Merged

targos added backported-to-v10.x and removed backport-requested-v10.x labels May 14, 2018

tniessen mentioned this pull request Jun 21, 2018

doc: update notes about GCM decryption #21445

Closed

3 tasks

tniessen removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 12, 2018

tniessen mentioned this pull request Sep 19, 2018

how to do feature detection #22585

Closed

TheNorthMemory added a commit to TheNorthMemory/wechatpay-axios-plugin that referenced this pull request Mar 2, 2021

patch for aes-gcm@node10.x, ref nodejs/node#20039

aa36a56



		static bool IsValidGCMTagLength(int tag_len) {
		return tag_len == 4 \|\| tag_len == 8 \|\| (tag_len >= 12 && tag_len <= 16);

Uh oh!

Conversation

tniessen commented Apr 15, 2018

Checklist

Uh oh!

tniessen commented Apr 15, 2018

Uh oh!

Trott commented Apr 15, 2018

Uh oh!

jasnell Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

tniessen Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

yhwang Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhwang Apr 19, 2018

Choose a reason for hiding this comment

Uh oh!

yhwang left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen commented Apr 21, 2018

Uh oh!

bnoordhuis Apr 21, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Apr 21, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Apr 21, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen commented Apr 22, 2018

Uh oh!

jasnell commented Apr 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

tniessen Apr 19, 2018 •

edited

Loading

tniessen commented Apr 21, 2018 •

edited

Loading