crypto: Allow GCM ciphers to have a longer IV length #6376
Conversation
|
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. |
|
/cc @nodejs/crypto |
7da4fd4 to
c7066fb
Compare
src/node_crypto.cc
Outdated
There was a problem hiding this comment.
Perhaps you can use EVP_CIPHER_iv_length(cipher_) instead of a magic(ish) number.
|
LGTM with nits. |
ed78728 to
dfb3d8b
Compare
|
@bnoordhuis Fixed the nits 👍 |
|
LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/2406/ Can one other member of @nodejs/crypto take a look? |
|
That build failed. Seems to be an issue on OSX, dont think it is related to the code change. |
|
LGTM |
|
argh! the tick-processor test again :-( really need to figure out why that's flaky again |
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. |
|
@shigeki Agreed, probably worded that comment wrong. I was tempted to remove the the >96bit check. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also, since you're working in this file, would it be possible to update the require statements to use const (in a separate commit)
|
Awesome, thank you!
|
There was a problem hiding this comment.
for the sake of readability and linting, can these be split into multiple concatenated lines? e.g.
'd9313225f88406e5a55909c5aff526' +
'9a86a7a9531534f7da2e4c303d8a31' +
'8a721c3c0c95956809532fcf0e2449' +
'a6b525b16aedf5aa0de657ba637b39' +
'1aafd255'
|
One additional comment from me. Otherwise LGTM once @shigeki is happy with it. Thank you for adding the source comments |
Yes. If we permit to use larger size of IV than 12 bytes, we should allow smaller size as well. |
919fdc3 to
0fab5be
Compare
|
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 Noticed the CI failed in FIPS mode. openssl-fips has an lower limit of 12 byte iv length. Could you rerun ? |
|
Woops I just submitted https://ci.nodejs.org/job/node-test-pull-request/2493/ |
|
@nodejs/crypto @nodejs/lts should we backport to v4.x? |
|
I think this would be an acceptable change for a semver-minor LTS release. |
|
@bnoordhuis this change is not landing cleanly, would you be willing to back port? |
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>
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>
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>
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>
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>
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>
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>
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>
Checklist
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_ctrlwith typeEVP_CTRL_GCM_SET_IVLEN