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.

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: Allow GCM ciphers to have a longer IV length #6376

Closed
wants to merge 2 commits into from

Conversation

mwain
Copy link
Contributor

@mwain mwain commented Apr 25, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

crypto

Description of change

GCM cipher IV length can have an value >=12 bytes.
When not the default 12 bytes (96 bits) sets the IV length using
EVP_CIPHER_CTX_ctrl with type EVP_CTRL_GCM_SET_IVLEN

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 25, 2016
@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Apr 25, 2016
@mwain
Copy link
Contributor Author

mwain commented Apr 25, 2016

In addition, the GCM spec does state the IV can have arbitrary length from 1 to 2^64 bits. This PR still enforces the 96 bit recommend default.

@mscdex
Copy link
Contributor

mscdex commented Apr 25, 2016

/cc @nodejs/crypto

EVP_CIPHER_CTX_init(&ctx_);
const bool encrypt = (kind_ == kCipher);
EVP_CipherInit_ex(&ctx_, cipher_, nullptr, nullptr, nullptr, encrypt);

/* Set IV length. Only required if GCM cipher and IV is >12 bytes. */
if (EVP_CIPHER_mode(cipher_) == EVP_CIPH_GCM_MODE && iv_len > 12) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you can use EVP_CIPHER_iv_length(cipher_) instead of a magic(ish) number.

@bnoordhuis
Copy link
Member

LGTM with nits.

@mwain
Copy link
Contributor Author

mwain commented Apr 27, 2016

@bnoordhuis Fixed the nits 👍

@bnoordhuis
Copy link
Member

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

Can one other member of @nodejs/crypto take a look?

@mwain
Copy link
Contributor Author

mwain commented Apr 27, 2016

That build failed. Seems to be an issue on OSX, dont think it is related to the code change.

@indutny
Copy link
Member

indutny commented Apr 27, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Apr 27, 2016

argh! the tick-processor test again :-( really need to figure out why that's flaky again

@shigeki
Copy link
Contributor

shigeki commented Apr 27, 2016

@mwain

In addition, the GCM spec does state the IV can have arbitrary length from 1 to 2^64 bits. This PR still enforces the 96 bit recommend default.

96 bits IV in GCM has only a performance benefit rather than a security one. I think that there is no reason to limits its size to be lager than 96 bits as long as it is nonce.

@mwain
Copy link
Contributor Author

mwain commented Apr 27, 2016

@shigeki Agreed, probably worded that comment wrong. I was tempted to remove the the >96bit check.

key: '6970787039613669314d623455536234',
iv: 'a582a11e7579bfdd0e28f008e1790990d100', plain: 'Hello World!',
ct: '50A054B6DA42B7A853DDE0CC',
tag: 'B05EEC074CE5D0984A1ECAD16300283E', tampered: false }
];
Copy link
Contributor

Choose a reason for hiding this comment

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

The official GCM spec has test vectors in Appendix B.
http://csrc.nist.gov/groups/ST/toolkit/BCM/documents/proposedmodes/gcm/gcm-revised-spec.pdf
For 8bytes and 60bytes IV, there are Test Case 5/6 of aes128-gcm, Test Case 11/12 of aes192-gcm and Test Case 17/18 of aes256-gcm.
I think it is better to use them. Also I'd be glad if you replace existing 12bytes IV tests to the official test vectors.

Copy link
Member

Choose a reason for hiding this comment

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

Quick nit: for future reference for anyone looking at this bit of code... some inline comments indicating where the iv's came from and why they were selected would be helpful. As is, the test is rather difficult to follow.

Copy link
Member

Choose a reason for hiding this comment

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

Also, since you're working in this file, would it be possible to update the require statements to use const (in a separate commit)

@mwain
Copy link
Contributor Author

mwain commented Apr 29, 2016

@shigeki Ive added new test cases from the spec.
Do you think we should remove the 12 byte lower limit?

@jasnell Ive updated the the require to use const.

@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

Awesome, thank you!
On Apr 29, 2016 5:19 AM, "Michael" notifications@github.com wrote:

@shigeki https://github.com/shigeki Ive added new test cases from the
spec.
Do you think we should remove the 12 byte lower limit?

@jasnell https://github.com/jasnell Ive updated the the require to use
const.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6376 (comment)

{ algo: 'aes-128-gcm',
key: 'feffe9928665731c6d6a8f9467308308',
iv: 'cafebabefacedbaddecaf888',
plain: 'd9313225f88406e5a55909c5aff5269a86a7a9531534f7da2e4c303d8a318a721c3c0c95956809532fcf0e2449a6b525b16aedf5aa0de657ba637b391aafd255',
Copy link
Member

Choose a reason for hiding this comment

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

for the sake of readability and linting, can these be split into multiple concatenated lines? e.g.

'd9313225f88406e5a55909c5aff526' +
'9a86a7a9531534f7da2e4c303d8a31' +
'8a721c3c0c95956809532fcf0e2449' +
'a6b525b16aedf5aa0de657ba637b39' +
'1aafd255'

@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

One additional comment from me. Otherwise LGTM once @shigeki is happy with it. Thank you for adding the source comments

@shigeki
Copy link
Contributor

shigeki commented Apr 30, 2016

@mwain

Do you think we should remove the 12 byte lower limit?

Yes. If we permit to use larger size of IV than 12 bytes, we should allow smaller size as well.

@mwain mwain force-pushed the crypt-gcm-ivLen-increase branch from 919fdc3 to 0fab5be Compare May 3, 2016 10:10
@mwain
Copy link
Contributor Author

mwain commented May 3, 2016

@shigeki Updated to allow smaller IV lengths. Added more tests to.
@jasnell Ive split the long lines down 👍

Once your all happy i will squish and tidy the commits up.

@jasnell
Copy link
Member

jasnell commented May 3, 2016

Appreciate it! Let's give @shigeki some time to review. I know he's looking at the openssl update today so he's likely to be tied up for a bit. I'll throw this into CI for testing here soon tho while we wait.

@jasnell
Copy link
Member

jasnell commented May 3, 2016

@mwain
Copy link
Contributor Author

mwain commented May 4, 2016

@jasnell Noticed the CI failed in FIPS mode. openssl-fips has an lower limit of 12 byte iv length.
Skipping tests which have IV lengths <12bytes when it FIPs mode.

Could you rerun ?

@bnoordhuis
Copy link
Member

bnoordhuis commented May 4, 2016

@shigeki
Copy link
Contributor

shigeki commented May 4, 2016

Woops I just submitted https://ci.nodejs.org/job/node-test-pull-request/2493/

@MylesBorins
Copy link
Contributor

@nodejs/crypto @nodejs/lts should we backport to v4.x?

@bnoordhuis
Copy link
Member

I think this would be an acceptable change for a semver-minor LTS release.

@MylesBorins
Copy link
Contributor

@bnoordhuis this change is not landing cleanly, would you be willing to back port?

bnoordhuis pushed a commit to bnoordhuis/io.js that referenced this pull request Jul 13, 2016
GCM cipher IV length can be >=1 bytes.
When not the default 12 bytes (96 bits) sets the IV length using
`EVP_CIPHER_CTX_ctrl` with type `EVP_CTRL_GCM_SET_IVLEN`

PR-URL: nodejs#6376
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@bnoordhuis
Copy link
Member

#7705

MylesBorins pushed a commit that referenced this pull request Jul 13, 2016
GCM cipher IV length can be >=1 bytes.
When not the default 12 bytes (96 bits) sets the IV length using
`EVP_CIPHER_CTX_ctrl` with type `EVP_CTRL_GCM_SET_IVLEN`

PR-URL: #6376
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
GCM cipher IV length can be >=1 bytes.
When not the default 12 bytes (96 bits) sets the IV length using
`EVP_CIPHER_CTX_ctrl` with type `EVP_CTRL_GCM_SET_IVLEN`

PR-URL: #6376
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
GCM cipher IV length can be >=1 bytes.
When not the default 12 bytes (96 bits) sets the IV length using
`EVP_CIPHER_CTX_ctrl` with type `EVP_CTRL_GCM_SET_IVLEN`

PR-URL: #6376
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@MylesBorins MylesBorins mentioned this pull request Jul 14, 2016
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Oct 17, 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.

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>
jasnell pushed a commit that referenced this pull request Oct 17, 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.

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 pushed a commit that referenced this pull request Nov 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.

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

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>
MylesBorins pushed a commit that referenced this pull request Nov 22, 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.

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 pushed a commit that referenced this pull request Nov 22, 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.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

9 participants