crypto: fix faulty logic in iv size check #9032

bnoordhuis · 2016-10-11T18:35:31Z

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

indutny

LGTM

addaleax

LGTM

bnoordhuis · 2016-10-11T18:45:57Z

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

shigeki · 2016-10-12T03:50:54Z

src/node_crypto.cc

@@ -3261,7 +3261,7 @@ void CipherBase::InitIv(const char* cipher_type,
     iv_length (0) for ECB ciphers */
  if (EVP_CIPHER_iv_length(cipher_) != iv_len &&
      !(EVP_CIPHER_mode(cipher_) == EVP_CIPH_ECB_MODE && iv_len == 0) &&


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.

shigeki · 2016-10-12T04:36:04Z

src/node_crypto.cc

@@ -3261,7 +3261,7 @@ void CipherBase::InitIv(const char* cipher_type,
     iv_length (0) for ECB ciphers */
  if (EVP_CIPHER_iv_length(cipher_) != iv_len &&
      !(EVP_CIPHER_mode(cipher_) == EVP_CIPH_ECB_MODE && iv_len == 0) &&
-      !(EVP_CIPHER_mode(cipher_) == EVP_CIPH_GCM_MODE) && iv_len > 0) {
+      !(EVP_CIPHER_mode(cipher_) == EVP_CIPH_GCM_MODE && iv_len > 0)) {
    return env()->ThrowError("Invalid IV length");


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.

bnoordhuis · 2016-10-12T13:35:20Z

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

shigeki

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

shigeki · 2016-10-12T15:35:33Z

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

indutny

LGTM

indutny · 2016-10-12T16:43:07Z

src/node_crypto.cc

+  const int expected_iv_len = EVP_CIPHER_iv_length(cipher_);
+  const bool is_gcm_mode = (EVP_CIPH_GCM_MODE == EVP_CIPHER_mode(cipher_));
+
+  if (is_gcm_mode == false && iv_len != expected_iv_len) {


!is_gcm_mode?

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

bnoordhuis · 2016-10-12T17:10:48Z

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.

bnoordhuis · 2016-10-12T17:26:16Z

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

indutny

LGTM if CI is green.

indutny · 2016-10-12T19:10:00Z

src/node_crypto.cc

+  const int expected_iv_len = EVP_CIPHER_iv_length(cipher_);
+  const bool is_gcm_mode = (EVP_CIPH_GCM_MODE == EVP_CIPHER_mode(cipher_));
+
+  if (is_gcm_mode == false && iv_len != expected_iv_len) {


MylesBorins · 2016-10-12T19:27:05Z

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

bnoordhuis · 2016-10-13T11:41:29Z

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>

MylesBorins · 2016-11-11T20:32:41Z

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

sam-github · 2016-11-18T19:34:27Z

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

MylesBorins · 2016-11-18T21:14:04Z

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

sam-github · 2016-11-18T21:46:26Z

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

addaleax · 2016-11-18T22:03:45Z

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?

MylesBorins · 2016-11-19T00:10:45Z

@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

sam-github · 2016-11-21T17:47:04Z

@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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: fix faulty logic in iv size check #9032

crypto: fix faulty logic in iv size check #9032

bnoordhuis commented Oct 11, 2016

indutny left a comment

addaleax left a comment

bnoordhuis commented Oct 11, 2016

shigeki Oct 12, 2016

shigeki Oct 12, 2016

bnoordhuis Oct 12, 2016

bnoordhuis commented Oct 12, 2016

shigeki left a comment

shigeki commented Oct 12, 2016

indutny left a comment

indutny Oct 12, 2016

bnoordhuis Oct 12, 2016

indutny Oct 12, 2016

bnoordhuis commented Oct 12, 2016

bnoordhuis commented Oct 12, 2016

indutny left a comment

indutny Oct 12, 2016

MylesBorins commented Oct 12, 2016

bnoordhuis commented Oct 13, 2016

MylesBorins commented Nov 11, 2016

sam-github commented Nov 18, 2016

MylesBorins commented Nov 18, 2016

sam-github commented Nov 18, 2016

addaleax commented Nov 18, 2016

MylesBorins commented Nov 19, 2016

sam-github commented Nov 21, 2016

crypto: fix faulty logic in iv size check #9032

crypto: fix faulty logic in iv size check #9032

Conversation

bnoordhuis commented Oct 11, 2016

indutny left a comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

bnoordhuis commented Oct 11, 2016

shigeki Oct 12, 2016

Choose a reason for hiding this comment

shigeki Oct 12, 2016

Choose a reason for hiding this comment

bnoordhuis Oct 12, 2016

Choose a reason for hiding this comment

bnoordhuis commented Oct 12, 2016

shigeki left a comment

Choose a reason for hiding this comment

shigeki commented Oct 12, 2016

indutny left a comment

Choose a reason for hiding this comment

indutny Oct 12, 2016

Choose a reason for hiding this comment

bnoordhuis Oct 12, 2016

Choose a reason for hiding this comment

indutny Oct 12, 2016

Choose a reason for hiding this comment

bnoordhuis commented Oct 12, 2016

bnoordhuis commented Oct 12, 2016

indutny left a comment

Choose a reason for hiding this comment

indutny Oct 12, 2016

Choose a reason for hiding this comment

MylesBorins commented Oct 12, 2016

bnoordhuis commented Oct 13, 2016

MylesBorins commented Nov 11, 2016

sam-github commented Nov 18, 2016

MylesBorins commented Nov 18, 2016

sam-github commented Nov 18, 2016

addaleax commented Nov 18, 2016

MylesBorins commented Nov 19, 2016

sam-github commented Nov 21, 2016