From 9389572cbc781d36b31213f5756da2ecde21d4a0 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 11 Oct 2016 20:01:32 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/9032 Refs: https://github.com/nodejs/node/pull/6376 Refs: https://github.com/nodejs/node/issues/9024 Reviewed-By: Anna Henningsen Reviewed-By: Fedor Indutny Reviewed-By: Shigeki Ohtsu --- src/node_crypto.cc | 20 +++++------ test/parallel/test-crypto-authenticated.js | 18 ++++++++-- .../test-crypto-cipheriv-decipheriv.js | 35 +++++++++++++++++++ 3 files changed, 58 insertions(+), 15 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index c3d61026d38a8a..c6414a4ba82f8d 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3068,11 +3068,10 @@ void CipherBase::InitIv(const char* cipher_type, return env()->ThrowError("Unknown cipher"); } - /* OpenSSL versions up to 0.9.8l failed to return the correct - 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) { + 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) { return env()->ThrowError("Invalid IV length"); } @@ -3080,13 +3079,10 @@ void CipherBase::InitIv(const char* cipher_type, 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 not default iv. */ - if (EVP_CIPHER_mode(cipher_) == EVP_CIPH_GCM_MODE && - iv_len != EVP_CIPHER_iv_length(cipher_)) { - if (!EVP_CIPHER_CTX_ctrl(&ctx_, EVP_CTRL_GCM_SET_IVLEN, iv_len, nullptr)) { - EVP_CIPHER_CTX_cleanup(&ctx_); - return env()->ThrowError("Invalid IV length"); - } + if (is_gcm_mode && + !EVP_CIPHER_CTX_ctrl(&ctx_, EVP_CTRL_GCM_SET_IVLEN, iv_len, nullptr)) { + EVP_CIPHER_CTX_cleanup(&ctx_); + return env()->ThrowError("Invalid IV length"); } if (!EVP_CIPHER_CTX_set_key_length(&ctx_, key_len)) { diff --git a/test/parallel/test-crypto-authenticated.js b/test/parallel/test-crypto-authenticated.js index b816d8ada3db9c..19643a9d80f2f3 100644 --- a/test/parallel/test-crypto-authenticated.js +++ b/test/parallel/test-crypto-authenticated.js @@ -307,10 +307,10 @@ const TEST_CASES = [ tag: 'a44a8266ee1c8eb0c8b5d4cf5ae9f19a', tampered: false }, ]; -var ciphers = crypto.getCiphers(); +const ciphers = crypto.getCiphers(); -for (var i in TEST_CASES) { - var test = TEST_CASES[i]; +for (const i in TEST_CASES) { + const test = TEST_CASES[i]; if (ciphers.indexOf(test.algo) === -1) { common.skip('unsupported ' + test.algo + ' test'); @@ -452,3 +452,15 @@ for (var i in TEST_CASES) { }, /Invalid IV length/); })(); } + +// Non-authenticating mode: +(function() { + const encrypt = + crypto.createCipheriv('aes-128-cbc', + 'ipxp9a6i1Mb4USb4', + '6fKjEjR3Vl30EUYC'); + encrypt.update('blah', 'ascii'); + encrypt.final(); + assert.throws(() => encrypt.getAuthTag(), / state/); + assert.throws(() => encrypt.setAAD(Buffer.from('123', 'ascii')), / state/); +})(); diff --git a/test/parallel/test-crypto-cipheriv-decipheriv.js b/test/parallel/test-crypto-cipheriv-decipheriv.js index 377f0f3ee43615..4462ee0a5fc34a 100644 --- a/test/parallel/test-crypto-cipheriv-decipheriv.js +++ b/test/parallel/test-crypto-cipheriv-decipheriv.js @@ -63,3 +63,38 @@ testCipher1(new Buffer('0123456789abcd0123456789'), '12345678'); testCipher1(new Buffer('0123456789abcd0123456789'), new Buffer('12345678')); testCipher2(new Buffer('0123456789abcd0123456789'), new Buffer('12345678')); + +// Zero-sized IV should be accepted in ECB mode. +crypto.createCipheriv('aes-128-ecb', Buffer.alloc(16), Buffer.alloc(0)); + +// But non-empty IVs should be rejected. +for (let n = 1; n < 256; n += 1) { + assert.throws( + () => crypto.createCipheriv('aes-128-ecb', Buffer.alloc(16), + Buffer.alloc(n)), + /Invalid IV length/); +} + +// Correctly sized IV should be accepted in CBC mode. +crypto.createCipheriv('aes-128-cbc', Buffer.alloc(16), Buffer.alloc(16)); + +// But all other IV lengths should be rejected. +for (let n = 0; n < 256; n += 1) { + if (n === 16) continue; + assert.throws( + () => crypto.createCipheriv('aes-128-cbc', Buffer.alloc(16), + Buffer.alloc(n)), + /Invalid IV length/); +} + +// Zero-sized IV should be rejected in GCM mode. +assert.throws( + () => crypto.createCipheriv('aes-128-gcm', Buffer.alloc(16), + Buffer.alloc(0)), + /Invalid IV length/); + +// But all other IV lengths should be accepted. +for (let n = 1; n < 256; n += 1) { + if (common.hasFipsCrypto && n < 12) continue; + crypto.createCipheriv('aes-128-gcm', Buffer.alloc(16), Buffer.alloc(n)); +}