GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — crypto: fix faulty logic in iv size check by bnoordhuis · Pull Request #9032 · nodejs/node

Conversation

bnoordhuis commented Oct 11, 2016

Copy link

Copy Markdown

Member

Fix a regression introduced in commit 2996b5c ("crypto: Allow GCM
ciphers to have a longer IV length") from April 2016 where a misplaced
parenthesis in a 'is ECB cipher?' check made it possible to use empty
IVs with non-ECB ciphers.

Refs: #6376
Refs: #9024

R=@nodejs/crypto @addaleax

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnoordhuis added crypto

Issues and PRs related to the crypto subsystem.

c++ Issues and PRs that require attention from people who are familiar with C++. labels

Oct 11, 2016

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label

Oct 11, 2016

bnoordhuis added the lts-watch-v4.x label

Oct 11, 2016

indutny approved these changes

Oct 11, 2016

View reviewed changes

indutny left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

addaleax approved these changes

Oct 11, 2016

View reviewed changes

addaleax left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnoordhuis commented Oct 11, 2016

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

shigeki reviewed

Oct 12, 2016

View reviewed changes

Comment thread

src/node_crypto.cc Outdated

shigeki Oct 12, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Seeing the above comment in regard of the openssl bug, the issue of ECB mode has already resolved in openssl/openssl@97fe2b4 so that we no longer need to check the iv length of ECB mode.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

shigeki reviewed

Oct 12, 2016

View reviewed changes

Comment thread

src/node_crypto.cc Outdated

shigeki Oct 12, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I could not figure out the reason why the GCM test of zero length of iv_len in test-crypto-authenticated.js has passed. There seems two reasons. One is the test has a bug to skip some tests due to checking test.password and i>0 but iv_len in GCM can be checked in the following EVP_CIPHER_CTX_ctrl(&ctx_, EVP_CTRL_GCM_SET_IVLEN, iv_len, nullptr).
I think the fix of test-crypto-authenticated.js as in https://gist.github.com/shigeki/afc23ecef12006777b668a2b8a24b044 is also needed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnoordhuis Oct 12, 2016

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Huh, good point. I'll investigate.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnoordhuis mentioned this pull request

Oct 12, 2016

test: cleanup IIFE tests #7694

Merged

3 tasks done

bnoordhuis force-pushed the fix9024 branch from 6e5ae94 to d2780b2 Compare

October 12, 2016 13:34

bnoordhuis commented Oct 12, 2016

Copy link

Copy Markdown

Member Author

Good catch, @shigeki. I think I got it now, PTAL.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

shigeki approved these changes

Oct 12, 2016

View reviewed changes

shigeki left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is very fine. I'd like to have another LGTM from @indutny too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

shigeki commented Oct 12, 2016

Copy link

Copy Markdown

Contributor

A new CI is running on https://ci.nodejs.org/job/node-test-pull-request/4492/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

indutny approved these changes

Oct 12, 2016

View reviewed changes

indutny left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/node_crypto.cc Outdated

indutny Oct 12, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

!is_gcm_mode?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnoordhuis Oct 12, 2016

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Believe it or not, I chose not to use that because !i is not very distinct in some fonts.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

indutny Oct 12, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

oh gosh!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MylesBorins reacted with thumbs up emoji

All reactions

👍 1 reaction

bnoordhuis commented Oct 12, 2016

Copy link

Copy Markdown

Member Author

One genuine failure on the FIPS buildbot, probably DES is not allowed in FIPS mode, but what is up with the freebsd and smartos buildbots? Lots of seemingly random flakes and not just in this run.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnoordhuis commented Oct 12, 2016

Copy link

Copy Markdown

Member Author

Now with FIPS fix-ups: https://ci.nodejs.org/job/node-test-pull-request/4493/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

indutny approved these changes

Oct 12, 2016

View reviewed changes

indutny left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM if CI is green.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/node_crypto.cc Outdated

indutny Oct 12, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

oh gosh!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MylesBorins reacted with thumbs up emoji

All reactions

👍 1 reaction

MylesBorins commented Oct 12, 2016

Copy link

Copy Markdown

Contributor

@bnoordhuis I've been noticing all sorts of flakes on BSD + smartos last 48 hours

gibfahn reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnoordhuis force-pushed the fix9024 branch from 6967e7a to ebf2065 Compare

October 13, 2016 11:28

bnoordhuis commented Oct 13, 2016

Copy link

Copy Markdown

Member Author

One more FIPS fix-up: https://ci.nodejs.org/job/node-test-pull-request/4498/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          crypto: fix faulty logic in iv size check

6ef6d42

Fix a regression introduced in commit 2996b5c ("crypto: Allow GCM
ciphers to have a longer IV length") from April 2016 where a misplaced
parenthesis in a 'is ECB cipher?' check made it possible to use empty
IVs with non-ECB ciphers.

Also fix some exit bugs in test/parallel/test-crypto-authenticated.js
that were introduced in commit 4a40832 ("test: cleanup IIFE tests")
where removing the IFFEs made the test exit prematurely instead of just
skipping subtests.

PR-URL: nodejs#9032
Refs: nodejs#6376
Refs: nodejs#9024
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>

bnoordhuis force-pushed the fix9024 branch from ebf2065 to 6ef6d42 Compare

October 17, 2016 15:08

bnoordhuis closed this

Oct 17, 2016

bnoordhuis deleted the fix9024 branch

October 17, 2016 15:08

bnoordhuis merged commit 6ef6d42 into nodejs:master

Oct 17, 2016

jasnell pushed a commit that referenced this pull request

Oct 17, 2016


          crypto: fix faulty logic in iv size check

c60d43b

Fix a regression introduced in commit 2996b5c ("crypto: Allow GCM
ciphers to have a longer IV length") from April 2016 where a misplaced
parenthesis in a 'is ECB cipher?' check made it possible to use empty
IVs with non-ECB ciphers.

Also fix some exit bugs in test/parallel/test-crypto-authenticated.js
that were introduced in commit 4a40832 ("test: cleanup IIFE tests")
where removing the IFFEs made the test exit prematurely instead of just
skipping subtests.

PR-URL: #9032
Refs: #6376
Refs: #9024
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>

MylesBorins added the lts-watch-v6.x label

Nov 11, 2016

MylesBorins pushed a commit that referenced this pull request

Nov 11, 2016


          crypto: fix faulty logic in iv size check

4d4d02a

Fix a regression introduced in commit 2996b5c ("crypto: Allow GCM
ciphers to have a longer IV length") from April 2016 where a misplaced
parenthesis in a 'is ECB cipher?' check made it possible to use empty
IVs with non-ECB ciphers.

Also fix some exit bugs in test/parallel/test-crypto-authenticated.js
that were introduced in commit 4a40832 ("test: cleanup IIFE tests")
where removing the IFFEs made the test exit prematurely instead of just
skipping subtests.

PR-URL: #9032
Refs: #6376
Refs: #9024
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>

MylesBorins added the land-on-v6.x label

Nov 11, 2016

MylesBorins removed the lts-watch-v6.x label

Nov 11, 2016

MylesBorins commented Nov 11, 2016

Copy link

Copy Markdown

Contributor

@bnoordhuis this lands cleanly on v6.x but not on v4.x. Would you be willing to manually backport?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sam-github pushed a commit to sam-github/node that referenced this pull request

Nov 18, 2016


          crypto: fix faulty logic in iv size check

0855c18

Fix a regression introduced in commit 2996b5c ("crypto: Allow GCM
ciphers to have a longer IV length") from April 2016 where a misplaced
parenthesis in a 'is ECB cipher?' check made it possible to use empty
IVs with non-ECB ciphers.

Also fix some exit bugs in test/parallel/test-crypto-authenticated.js
that were introduced in commit 4a40832 ("test: cleanup IIFE tests")
where removing the IFFEs made the test exit prematurely instead of just
skipping subtests.

PR-URL: nodejs#9032
Refs: nodejs#6376
Refs: nodejs#9024
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>

sam-github commented Nov 18, 2016

Copy link

Copy Markdown

Contributor

@thealphanerd I'm not sure where you are accumulating back-ports, I was thinking there might be a v4.xxx-proposal, so maybe you already have this. Anyhow, I back-ported as an exercise https://github.com/sam-github/node/commits/v4-pr/9032

@bnoordhuis if you didn't back-port already, PTAL at above.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MylesBorins commented Nov 18, 2016

Copy link

Copy Markdown

Contributor

@sam-github would you be willing to send that backport to v4.x as a PR so it can be appropriately reviewed?

Thanks for taking the time to do it!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sam-github mentioned this pull request

Nov 18, 2016

crypto: fix faulty logic in iv size check #9686

Closed

4 tasks

sam-github commented Nov 18, 2016

Copy link

Copy Markdown

Contributor

@thealphanerd #9686, is there any way to list all the PRs that target 4.x?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sam-github mentioned this pull request

Nov 18, 2016

crypto: fix faulty logic in iv size check (backport #9032) #9687

Closed

4 tasks

addaleax commented Nov 18, 2016

Copy link

Copy Markdown

Member

is there any way to list all the PRs that target 4.x?

https://github.com/nodejs/node/pulls?q=is%3Aopen+label%3Av4.x+is%3Apr might come close?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MylesBorins commented Nov 19, 2016

Copy link

Copy Markdown

Contributor

@sam-github currently all pr's that target v4.x should have the v4.x label

There are also commits to be audited such as well as commits that have the lts-watch-v4.x label but are closed. There are also commits in master that have not been labelled the need to be audited

Feel free to ping me outside github if you wanna chat more in depth about the process

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sam-github commented Nov 21, 2016

Copy link

Copy Markdown

Contributor

@thealphanerd Thanks, I think I got the gist, target -staging, label with the target version.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MylesBorins pushed a commit that referenced this pull request

Nov 22, 2016


          crypto: fix faulty logic in iv size check

1b1bf4e

Fix a regression introduced in commit 2996b5c ("crypto: Allow GCM
ciphers to have a longer IV length") from April 2016 where a misplaced
parenthesis in a 'is ECB cipher?' check made it possible to use empty
IVs with non-ECB ciphers.

Also fix some exit bugs in test/parallel/test-crypto-authenticated.js
that were introduced in commit 4a40832 ("test: cleanup IIFE tests")
where removing the IFFEs made the test exit prematurely instead of just
skipping subtests.

PR-URL: #9032
Refs: #6376
Refs: #9024
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels

Nov 22, 2016

MylesBorins pushed a commit that referenced this pull request

Nov 22, 2016


          crypto: fix faulty logic in iv size check

Fix a regression introduced in commit 2996b5c ("crypto: Allow GCM
ciphers to have a longer IV length") from April 2016 where a misplaced
parenthesis in a 'is ECB cipher?' check made it possible to use empty
IVs with non-ECB ciphers.

Also fix some exit bugs in test/parallel/test-crypto-authenticated.js
that were introduced in commit 4a40832 ("test: cleanup IIFE tests")
where removing the IFFEs made the test exit prematurely instead of just
skipping subtests.

PR-URL: #9032
Refs: #6376
Refs: #9024
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>

This was referenced Nov 22, 2016

v6.9.2 proposal #9735

Merged

v4.7.0 proposal #9736

Merged

Reviewers

addaleax

addaleax approved these changes

+2 more reviewers

indutny

indutny approved these changes

shigeki

shigeki approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.