★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/17825Nouvelle recherche | Portail wanayoo
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

crypto: throw on invalid authentication tag length #17825

Closed
wants to merge 3 commits into from
Closed

crypto: throw on invalid authentication tag length #17825

wants to merge 3 commits into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Dec 22, 2017 •
edited

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 tniessen requested a review from bnoordhuis Dec 22, 2017
@tniessen
Copy link
Member Author

@tniessen tniessen commented Dec 22, 2017

cc @nodejs/crypto @willclarktech

@tniessen tniessen force-pushed the throw-auth-tag-length branch 2 times, most recently from 42d00bb to dc0f9f0 Compare Dec 28, 2017
@tniessen
Copy link
Member Author

@tniessen tniessen commented Jan 5, 2018

ping @nodejs/crypto

@tniessen
Copy link
Member Author

@tniessen tniessen commented Jan 6, 2018

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

joyeecheung
joyeecheung approved these changes Jan 6, 2018
View changes
src/node_crypto.cc Outdated
"Valid GCM tag lengths are 4, 8, 12, 13, 14, 15, 16.",
tag_len);
Environment* env = Environment::GetCurrent(args);
return env->ThrowError("Invalid GCM authentication tag length");
Copy link
Member

@joyeecheung joyeecheung Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@tniessen tniessen Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, done! :)

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 6, 2018 •
edited

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
Copy link
Member Author

@tniessen tniessen commented Jan 6, 2018

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.

@tniessen tniessen force-pushed the throw-auth-tag-length branch from 5a09d9f to 5debc52 Compare Jan 6, 2018
@tniessen tniessen mentioned this pull request Jan 6, 2018
Closed
4 tasks
jasnell
jasnell approved these changes Jan 6, 2018
View changes
@tniessen tniessen added this to the 11.0.0 milestone Jan 11, 2018
@tniessen tniessen mentioned this pull request Jan 21, 2018
Closed
@tniessen tniessen mentioned this pull request Jan 25, 2018
Closed
2 tasks
@targos
Copy link
Member

@targos targos commented Feb 3, 2018

What is blocking this?

@tniessen
Copy link
Member Author

@tniessen tniessen commented Feb 4, 2018

@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).

@tniessen tniessen removed the blocked label Apr 11, 2018
@tniessen
Copy link
Member Author

@tniessen tniessen commented Apr 13, 2018

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

@tniessen
Copy link
Member Author

@tniessen tniessen commented Apr 13, 2018

@joyeecheung PTAL :)

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 14, 2018

Failure in CI is unrelated.

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 14, 2018

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

jasnell added a commit that referenced this issue Apr 14, 2018
@tniessen @jasnell
crypto: throw on invalid authentication tag length
d81a7b4 
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>
jasnell added a commit that referenced this issue Apr 14, 2018
@tniessen @jasnell
doc: note that setAuthTag throws on invalid length
cf35085 
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 added a commit that referenced this issue Apr 14, 2018
@tniessen @jasnell
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
Copy link
Member

@jasnell jasnell commented Apr 14, 2018

Landed in d81a7b4, cf35085, and 985d180

@jasnell jasnell closed this Apr 14, 2018
@tniessen
Copy link
Member Author

@tniessen tniessen commented Apr 14, 2018

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 :)

@tniessen tniessen mentioned this pull request Apr 15, 2018
Closed
2 tasks
tniessen added a commit to tniessen/node that referenced this issue May 23, 2018
@tniessen
crypto: throw in setAuthTag on invalid length
00cf4aa 
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
tniessen added a commit that referenced this issue Jun 1, 2018
@tniessen
crypto: throw in setAuthTag on invalid length
faf449c 
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>
@tniessen tniessen mentioned this pull request Jun 21, 2018
Closed
3 tasks
targos added a commit that referenced this issue Jun 24, 2018
@tniessen @targos
doc: update notes about GCM decryption
193d6d1 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis

@jasnell jasnell

@joyeecheung joyeecheung

Assignees
No one assigned
Labels
c++ crypto deprecations notable-change security semver-major
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet
7 participants
@tniessen @joyeecheung @targos @jasnell @bnoordhuis @ChALkeR @nodejs-github-bot