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

crypto: fix faulty logic in iv size check #9032

Merged
merged 1 commit into from Oct 17, 2016

Conversation

Projects
None yet
7 participants
@bnoordhuis
Member

bnoordhuis commented Oct 11, 2016

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

This comment has been minimized.

Show comment
Hide comment
Show outdated Hide outdated src/node_crypto.cc Outdated
Show outdated Hide outdated src/node_crypto.cc Outdated

@bnoordhuis bnoordhuis referenced this pull request Oct 12, 2016

Merged

test: cleanup IIFE tests #7694

3 of 3 tasks complete
@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Oct 12, 2016

Member

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

Member

bnoordhuis commented Oct 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@shigeki
Contributor

shigeki commented Oct 12, 2016

@indutny

LGTM

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

This comment has been minimized.

@indutny

indutny Oct 12, 2016

Member

!is_gcm_mode?

@indutny

indutny Oct 12, 2016

Member

!is_gcm_mode?

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 12, 2016

Member

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

@bnoordhuis

bnoordhuis Oct 12, 2016

Member

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

This comment has been minimized.

@indutny

indutny Oct 12, 2016

Member

oh gosh!

@indutny

indutny Oct 12, 2016

Member

oh gosh!

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Oct 12, 2016

Member

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.

Member

bnoordhuis commented Oct 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis
Member

bnoordhuis commented Oct 12, 2016

@indutny

LGTM if CI is green.

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

This comment has been minimized.

@indutny

indutny Oct 12, 2016

Member

oh gosh!

@indutny

indutny Oct 12, 2016

Member

oh gosh!

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Oct 12, 2016

Member

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

Member

MylesBorins commented Oct 12, 2016

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

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis
Member

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

@bnoordhuis bnoordhuis closed this Oct 17, 2016

@bnoordhuis bnoordhuis deleted the bnoordhuis:fix9024 branch Oct 17, 2016

@bnoordhuis bnoordhuis merged commit 6ef6d42 into nodejs:master Oct 17, 2016

jasnell added a commit that referenced this pull request Oct 17, 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 <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

MylesBorins added a commit that referenced this pull request Nov 11, 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 <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 11, 2016

Member

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

Member

MylesBorins commented Nov 11, 2016

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

sam-github added a commit to sam-github/node that referenced this pull request Nov 18, 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: 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

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Nov 18, 2016

Member

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

Member

sam-github commented Nov 18, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 18, 2016

Member

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

Member

MylesBorins commented Nov 18, 2016

@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 sam-github referenced this pull request Nov 18, 2016

Closed

crypto: fix faulty logic in iv size check #9686

0 of 4 tasks complete
@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Nov 18, 2016

Member

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

Member

sam-github commented Nov 18, 2016

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

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 18, 2016

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?

Member

addaleax commented Nov 18, 2016

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

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 19, 2016

Member

@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

Member

MylesBorins commented Nov 19, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Nov 21, 2016

Member

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

Member

sam-github commented Nov 21, 2016

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

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

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

This was referenced Nov 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment