crypto: fix faulty logic in iv size check by bnoordhuis · Pull Request #9032 · nodejs/node · GitHub

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

LGTM

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

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.

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.

Huh, good point. I'll investigate.

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

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

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

LGTM

!is_gcm_mode?

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

oh gosh!

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.

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

LGTM if CI is green.

oh gosh!

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

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

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 <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

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 <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

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

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 <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

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

@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!

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

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?

@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

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

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 <anna@addaleax.net> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

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

addaleax approved these changes Oct 11, 2016

View reviewed changes

shigeki reviewed Oct 12, 2016

View reviewed changes

bnoordhuis mentioned this pull request Oct 12, 2016

test: cleanup IIFE tests #7694

Merged

3 tasks

bnoordhuis force-pushed the fix9024 branch from 6e5ae94 to d2780b2 Compare October 12, 2016 13:34

shigeki approved these changes Oct 12, 2016

View reviewed changes

indutny approved these changes Oct 12, 2016

View reviewed changes

bnoordhuis force-pushed the fix9024 branch from 6967e7a to ebf2065 Compare October 13, 2016 11:28

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

MylesBorins added the lts-watch-v6.x label Nov 11, 2016

MylesBorins added the land-on-v6.x label Nov 11, 2016

MylesBorins removed the lts-watch-v6.x label Nov 11, 2016

sam-github mentioned this pull request Nov 18, 2016

crypto: fix faulty logic in iv size check #9686

Closed

4 tasks

sam-github mentioned this pull request Nov 18, 2016

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

Closed

4 tasks

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Nov 22, 2016

This was referenced Nov 22, 2016

v6.9.2 proposal #9735

Merged

v4.7.0 proposal #9736

Merged

crypto: fix faulty logic in iv size check by bnoordhuis · Pull Request #9032 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: fix faulty logic in iv size check - #9032

crypto: fix faulty logic in iv size check#9032bnoordhuis merged 1 commit intonodejs:masternodejs/node:masterfrom bnoordhuis:fix9024bnoordhuis/io.js:fix9024Copy head branch name to clipboard

Conversation

bnoordhuis commented Oct 11, 2016

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Oct 11, 2016

Uh oh!

shigeki Oct 12, 2016

Choose a reason for hiding this comment

Uh oh!

shigeki Oct 12, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Oct 12, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Oct 12, 2016

Uh oh!

shigeki left a comment

Choose a reason for hiding this comment

Uh oh!

shigeki commented Oct 12, 2016

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

indutny Oct 12, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Oct 12, 2016

Choose a reason for hiding this comment

Uh oh!

indutny Oct 12, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Oct 12, 2016

Uh oh!

bnoordhuis commented Oct 12, 2016

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

indutny Oct 12, 2016

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Oct 12, 2016

Uh oh!

bnoordhuis commented Oct 13, 2016

Uh oh!

MylesBorins commented Nov 11, 2016

Uh oh!

sam-github commented Nov 18, 2016

Uh oh!

MylesBorins commented Nov 18, 2016

Uh oh!

sam-github commented Nov 18, 2016

Uh oh!

addaleax commented Nov 18, 2016

Uh oh!

MylesBorins commented Nov 19, 2016

Uh oh!

sam-github commented Nov 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

crypto: fix faulty logic in iv size check#9032
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:fix9024