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.

Sign up for GitHub

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 in setAuthTag on invalid length #20040

Closed
wants to merge 1 commit into from
Closed

crypto: throw in setAuthTag on invalid length #20040

wants to merge 1 commit into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Apr 15, 2018 •
edited
Loading

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot 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
Copy link
Member Author

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

@mscdex
Copy link
Contributor

mscdex commented Apr 15, 2018

/cc @nodejs/crypto

@yhwang

This comment has been minimized.

Sign in to view
yhwang
yhwang approved these changes Apr 17, 2018
View reviewed changes
Copy link
Member

@yhwang yhwang left a comment

Choose a reason for hiding this comment

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

LGTM

@tniessen

This comment has been minimized.

Sign in to view
@yhwang

This comment has been minimized.

Sign in to view
@tniessen

This comment has been minimized.

Sign in to view
@yhwang

This comment has been minimized.

Sign in to view
@tniessen

This comment has been minimized.

Sign in to view
@yhwang

This comment has been minimized.

Sign in to view
@tniessen

This comment has been minimized.

Sign in to view
@yhwang
Copy link
Member

yhwang commented Apr 18, 2018 •
edited
Loading

but I guess we should throw here, which would make this a breaking change.

+1 for throwing an exception.

and yes, old implementation truncate the authtag......

@tniessen tniessen mentioned this pull request Apr 19, 2018
Closed
4 tasks
@BridgeAR
Copy link
Member

Is this ready to land?

@tniessen
Copy link
Member Author

@BridgeAR No, I missed an edge case which @yhwang discovered, so this could cause a regression. The only solution seems to be to throw an error if the input value is too long, I will update the PR as soon as I have the time to do so.

richardlau
richardlau previously requested changes Apr 22, 2018
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

As per #20040 (comment).

@mscdex mscdex added the wip Issues and PRs that are still a work in progress. label May 8, 2018
@tniessen tniessen force-pushed the crypto-dont-limit-tag-by-sizeof branch from 4dad3dc to 18611c6 Compare May 21, 2018 11:26
@tniessen tniessen changed the title crypto: remove conditional assignment in setAuthTag crypto: throw in setAuthTag on invalid length May 21, 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 tniessen force-pushed the crypto-dont-limit-tag-by-sizeof branch from 18611c6 to 00cf4aa Compare May 23, 2018 17:22
@tniessen tniessen added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 23, 2018
@tniessen tniessen removed the wip Issues and PRs that are still a work in progress. label May 23, 2018
@tniessen
Copy link
Member Author

tniessen commented May 23, 2018 •
edited
Loading

This patch turned out completely different than I had originally planned, but it still goes into the correct direction in my opinion. Please take another look.

As this change is now semver-major anyway, I also unified related error messages.

I also took the liberty of marking the discussion of the old change as "Outdated", I can't find the issue about when to use that feature. Feel free to unhide the discussion if this was incorrect.

@tniessen tniessen requested a review from a team May 26, 2018 13:27
bnoordhuis
bnoordhuis approved these changes May 30, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM at a quick glance.

@tniessen
Copy link
Member Author

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

I know that most of the TSC is probably travelling right now, but still pinging @nodejs/tsc as this is a semver-major change now. Also @richardlau due to his blocking review.

@tniessen
Copy link
Member Author

Only failure seems to be infra-related:

/var/folders/dr/m8hrvy3d71172187rtx7vm380000gp/T/jenkins12712608435428624123.sh: line 6: /Users/iojs/Library/Python/2.7/bin/tap2junit: No such file or directory

Here's CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1446/

@Trott
Copy link
Member

Trott commented May 31, 2018

Re-running node-test-commit-osx: https://ci.nodejs.org/job/node-test-commit-osx/18957/

tniessen added a commit that referenced this pull request 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
Copy link
Member Author

tniessen commented Jun 1, 2018

CitGM looks good as far as I can tell. Landed in faf449c.

@tniessen tniessen closed this Jun 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yhwang yhwang yhwang approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@richardlau richardlau richardlau left review comments

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.