FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: fix faulty logic in iv size check · nodejs/node@4d4d02a · GitHub

/ node Public

Commit 4d4d02a

Browse files
bnoordhuis
authored and
Myles Borins
committed
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>
1 parent def6874 commit 4d4d02a

3 files changed

Lines changed: 60 additions & 38 deletions

File tree

‎src/node_crypto.cc‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3259,25 +3259,21 @@ void CipherBase::InitIv(const char* cipher_type,
32593259
return env()->ThrowError("Unknown cipher");
32603260
}
32613261

3262-
/* OpenSSL versions up to 0.9.8l failed to return the correct
3263-
iv_length (0) for ECB ciphers */
3264-
if (EVP_CIPHER_iv_length(cipher_) != iv_len &&
3265-
!(EVP_CIPHER_mode(cipher_) == EVP_CIPH_ECB_MODE && iv_len == 0) &&
3266-
!(EVP_CIPHER_mode(cipher_) == EVP_CIPH_GCM_MODE) && iv_len > 0) {
3262+
const int expected_iv_len = EVP_CIPHER_iv_length(cipher_);
3263+
const bool is_gcm_mode = (EVP_CIPH_GCM_MODE == EVP_CIPHER_mode(cipher_));
3264+
3265+
if (is_gcm_mode == false && iv_len != expected_iv_len) {
32673266
return env()->ThrowError("Invalid IV length");
32683267
}
32693268

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

3274-
/* Set IV length. Only required if GCM cipher and IV is not default iv. */
3275-
if (EVP_CIPHER_mode(cipher_) == EVP_CIPH_GCM_MODE &&
3276-
iv_len != EVP_CIPHER_iv_length(cipher_)) {
3277-
if (!EVP_CIPHER_CTX_ctrl(&ctx_, EVP_CTRL_GCM_SET_IVLEN, iv_len, nullptr)) {
3278-
EVP_CIPHER_CTX_cleanup(&ctx_);
3279-
return env()->ThrowError("Invalid IV length");
3280-
}
3273+
if (is_gcm_mode &&
3274+
!EVP_CIPHER_CTX_ctrl(&ctx_, EVP_CTRL_GCM_SET_IVLEN, iv_len, nullptr)) {
3275+
EVP_CIPHER_CTX_cleanup(&ctx_);
3276+
return env()->ThrowError("Invalid IV length");
32813277
}
32823278

32833279
if (!EVP_CIPHER_CTX_set_key_length(&ctx_, key_len)) {

‎test/parallel/test-crypto-authenticated.js‎

Lines changed: 17 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -307,10 +307,10 @@ const TEST_CASES = [
307307
tag: 'a44a8266ee1c8eb0c8b5d4cf5ae9f19a', tampered: false },
308308
];
309309

310-
var ciphers = crypto.getCiphers();
310+
const ciphers = crypto.getCiphers();
311311

312-
for (var i in TEST_CASES) {
313-
var test = TEST_CASES[i];
312+
for (const i in TEST_CASES) {
313+
const test = TEST_CASES[i];
314314

315315
if (ciphers.indexOf(test.algo) === -1) {
316316
common.skip('unsupported ' + test.algo + ' test');
@@ -359,8 +359,7 @@ for (var i in TEST_CASES) {
359359
}
360360
}
361361

362-
{
363-
if (!test.password) return;
362+
if (test.password) {
364363
if (common.hasFipsCrypto) {
365364
assert.throws(() => { crypto.createCipher(test.algo, test.password); },
366365
/not supported in FIPS mode/);
@@ -379,8 +378,7 @@ for (var i in TEST_CASES) {
379378
}
380379
}
381380

382-
{
383-
if (!test.password) return;
381+
if (test.password) {
384382
if (common.hasFipsCrypto) {
385383
assert.throws(() => { crypto.createDecipher(test.algo, test.password); },
386384
/not supported in FIPS mode/);
@@ -400,24 +398,6 @@ for (var i in TEST_CASES) {
400398
}
401399
}
402400

403-
// after normal operation, test some incorrect ways of calling the API:
404-
// it's most certainly enough to run these tests with one algorithm only.
405-
406-
if (i > 0) {
407-
continue;
408-
}
409-
410-
{
411-
// non-authenticating mode:
412-
const encrypt = crypto.createCipheriv('aes-128-cbc',
413-
'ipxp9a6i1Mb4USb4', '6fKjEjR3Vl30EUYC');
414-
encrypt.update('blah', 'ascii');
415-
encrypt.final();
416-
assert.throws(() => { encrypt.getAuthTag(); }, / state/);
417-
assert.throws(() => { encrypt.setAAD(Buffer.from('123', 'ascii')); },
418-
/ state/);
419-
}
420-
421401
{
422402
// trying to get tag before inputting all data:
423403
const encrypt = crypto.createCipheriv(test.algo,
@@ -452,3 +432,15 @@ for (var i in TEST_CASES) {
452432
}, /Invalid IV length/);
453433
}
454434
}
435+
436+
// Non-authenticating mode:
437+
{
438+
const encrypt =
439+
crypto.createCipheriv('aes-128-cbc',
440+
'ipxp9a6i1Mb4USb4',
441+
'6fKjEjR3Vl30EUYC');
442+
encrypt.update('blah', 'ascii');
443+
encrypt.final();
444+
assert.throws(() => encrypt.getAuthTag(), / state/);
445+
assert.throws(() => encrypt.setAAD(Buffer.from('123', 'ascii')), / state/);
446+
}

‎test/parallel/test-crypto-cipheriv-decipheriv.js‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,5 +61,39 @@ testCipher1('0123456789abcd0123456789', '12345678');
6161
testCipher1('0123456789abcd0123456789', Buffer.from('12345678'));
6262
testCipher1(Buffer.from('0123456789abcd0123456789'), '12345678');
6363
testCipher1(Buffer.from('0123456789abcd0123456789'), Buffer.from('12345678'));
64-
6564
testCipher2(Buffer.from('0123456789abcd0123456789'), Buffer.from('12345678'));
65+
66+
// Zero-sized IV should be accepted in ECB mode.
67+
crypto.createCipheriv('aes-128-ecb', Buffer.alloc(16), Buffer.alloc(0));
68+
69+
// But non-empty IVs should be rejected.
70+
for (let n = 1; n < 256; n += 1) {
71+
assert.throws(
72+
() => crypto.createCipheriv('aes-128-ecb', Buffer.alloc(16),
73+
Buffer.alloc(n)),
74+
/Invalid IV length/);
75+
}
76+
77+
// Correctly sized IV should be accepted in CBC mode.
78+
crypto.createCipheriv('aes-128-cbc', Buffer.alloc(16), Buffer.alloc(16));
79+
80+
// But all other IV lengths should be rejected.
81+
for (let n = 0; n < 256; n += 1) {
82+
if (n === 16) continue;
83+
assert.throws(
84+
() => crypto.createCipheriv('aes-128-cbc', Buffer.alloc(16),
85+
Buffer.alloc(n)),
86+
/Invalid IV length/);
87+
}
88+
89+
// Zero-sized IV should be rejected in GCM mode.
90+
assert.throws(
91+
() => crypto.createCipheriv('aes-128-gcm', Buffer.alloc(16),
92+
Buffer.alloc(0)),
93+
/Invalid IV length/);
94+
95+
// But all other IV lengths should be accepted.
96+
for (let n = 1; n < 256; n += 1) {
97+
if (common.hasFipsCrypto && n < 12) continue;
98+
crypto.createCipheriv('aes-128-gcm', Buffer.alloc(16), Buffer.alloc(n));
99+
}

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL