crypto: throw on invalid authentication tag length by tniessen · Pull Request #17825 · nodejs/node

tniessen · 2017-12-22T18:02:53Z

This is a follow-up to #17566.

Refs: #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

Affected core subsystem(s)

crypto

tniessen · 2017-12-22T18:03:24Z

cc @nodejs/crypto @willclarktech

tniessen · 2018-01-05T10:43:26Z

ping @nodejs/crypto

tniessen · 2018-01-06T11:29:48Z

ping @nodejs/tsc as this is a semver-major change

joyeecheung · 2018-01-06T11:35:02Z

I would prefer keeping the invalid tag_len in the message for debuggability, but it's not blocking.

Thank you, done! :)

joyeecheung · 2018-01-06T11:38:04Z

By the way, the original deprecation does not seem to appear in deprecations.md, should we assign a code for it and move it to end-of-life now?

tniessen · 2018-01-06T11:46:47Z

By the way, the original deprecation does not seem to appear in deprecations.md, should we assign a code for it and move it to end-of-life now?

That's a good idea, I will open a PR.

targos · 2018-02-03T16:52:10Z

What is blocking this?

tniessen · 2018-02-04T08:29:58Z

@targos This cannot be landed in the same semver-major release as #18017 (at least that's what I assume is our rule for deprecation cycles).

Refs: nodejs#17523

tniessen · 2018-04-13T14:26:53Z

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

tniessen · 2018-04-13T14:27:26Z

@joyeecheung PTAL :)

jasnell · 2018-04-14T17:49:04Z

Failure in CI is unrelated.

jasnell · 2018-04-14T17:49:57Z

I'm going to land this but I'm not going to pull it in to 10.x

Refs: #17523 PR-URL: #17825 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #17825 Refs: #17523 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2018-04-14T17:54:03Z

Landed in d81a7b4, cf35085, and 985d180

tniessen · 2018-04-14T22:15:43Z

I'm going to land this but I'm not going to pull it in to 10.x

@jasnell Thank you, this was part of the 11.0.0 milestone and was not supposed to land on node 10 :)

The current implementation performs limited checks only and silently ignores superfluous bytes of the authentication tag. This change makes setAuthTag throw when - the user-specified authTagLength does not match the actual tag length, especially when the authentication tag is longer than 16 bytes, and when - the mode is GCM, no authTagLength option has been specified and the tag length is not a valid GCM tag length. This change makes the conditional assignment in SetAuthTag unnecessary, which is replaced with a CHECK. Refs: nodejs#17825

The current implementation performs limited checks only and silently ignores superfluous bytes of the authentication tag. This change makes setAuthTag throw when - the user-specified authTagLength does not match the actual tag length, especially when the authentication tag is longer than 16 bytes, and when - the mode is GCM, no authTagLength option has been specified and the tag length is not a valid GCM tag length. This change makes the conditional assignment in SetAuthTag unnecessary, which is replaced with a CHECK. Refs: #17825 PR-URL: #20040 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Yihong Wang <yh.wang@ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

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>

tniessen requested a review from bnoordhuis December 22, 2017 18:02

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 Dec 22, 2017

tniessen added notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version. labels Dec 22, 2017

tniessen force-pushed the throw-auth-tag-length branch 2 times, most recently from 42d00bb to dc0f9f0Compare December 28, 2017 18:16

bnoordhuis approved these changes Jan 5, 2018
View reviewed changes

joyeecheung approved these changes Jan 6, 2018
View reviewed changes

tniessen force-pushed the throw-auth-tag-length branch from 5a09d9f to 5debc52Compare January 6, 2018 12:29

tniessen mentioned this pull request Jan 6, 2018
crypto: assign deprecation code for setAuthTag/GCM #18017
Closed
4 tasks

jasnell approved these changes Jan 6, 2018
View reviewed changes

tniessen added the blocked PRs that are blocked by other issues or PRs. label Jan 11, 2018

tniessen added this to the 11.0.0 milestone Jan 11, 2018

This was referenced Jan 21, 2018
Additional validation of tag length in AES GCM decryption #17523
Closed
doc: warn about GCM authenticity #18376
Closed

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

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

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

tniessen force-pushed the throw-auth-tag-length branch from 5debc52 to 6ee8771Compare April 11, 2018 11:54

tniessen added 3 commits April 11, 2018 13:59

crypto: throw on invalid authentication tag length
4f9fb30
Refs: nodejs#17523

doc: note that setAuthTag throws on invalid length
fe805b7

doc: move support for invalid GCM tags to EOL
2e68afc

tniessen force-pushed the throw-auth-tag-length branch from 6ee8771 to 2e68afcCompare April 11, 2018 12:00

tniessen removed the blocked PRs that are blocked by other issues or PRs. label Apr 11, 2018

joyeecheung approved these changes Apr 14, 2018
View reviewed changes

jasnell added the dont-land-on-v10.x label Apr 14, 2018

jasnell pushed a commit that referenced this pull request Apr 14, 2018
doc: move support for invalid GCM tags to EOL
985d180
PR-URL: #17825 Refs: #17523 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell closed this Apr 14, 2018

tniessen mentioned this pull request Apr 15, 2018
crypto: throw in setAuthTag on invalid length #20040
Closed
2 tasks

tniessen added the deprecations Issues and PRs related to deprecations. label Apr 28, 2018

targos removed the dont-land-on-v10.x label Jun 8, 2018

tniessen mentioned this pull request Jun 21, 2018
doc: update notes about GCM decryption #21445
Closed
3 tasks

Uh oh!

Uh oh!

Conversation

tniessen commented Dec 22, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

tniessen commented Dec 22, 2017

Uh oh!

tniessen commented Jan 5, 2018

Uh oh!

tniessen commented Jan 6, 2018

Uh oh!

joyeecheungJan 6, 2018

Choose a reason for hiding this comment

Uh oh!

tniessenJan 6, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jan 6, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Jan 6, 2018

Uh oh!

targos commented Feb 3, 2018

Uh oh!

tniessen commented Feb 4, 2018

Uh oh!

tniessen commented Apr 13, 2018

Uh oh!

tniessen commented Apr 13, 2018

Uh oh!

jasnell commented Apr 14, 2018

Uh oh!

jasnell commented Apr 14, 2018

Uh oh!

jasnell commented Apr 14, 2018

Uh oh!

tniessen commented Apr 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tniessen commented Dec 22, 2017•
edited
Loading

joyeecheung commented Jan 6, 2018•
edited
Loading