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 <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>

src/node_crypto.cc

@@ -3259,25 +3259,21 @@ 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");
   }
 
   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 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)) {

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');
@@ -359,8 +359,7 @@ for (var i in TEST_CASES) {
     }
   }
 
-  {
-    if (!test.password) return;
+  if (test.password) {
     if (common.hasFipsCrypto) {
       assert.throws(() => { crypto.createCipher(test.algo, test.password); },
                     /not supported in FIPS mode/);
@@ -379,8 +378,7 @@ for (var i in TEST_CASES) {
     }
   }
 
-  {
-    if (!test.password) return;
+  if (test.password) {
     if (common.hasFipsCrypto) {
       assert.throws(() => { crypto.createDecipher(test.algo, test.password); },
                     /not supported in FIPS mode/);
@@ -400,24 +398,6 @@ for (var i in TEST_CASES) {
     }
   }
 
-  // after normal operation, test some incorrect ways of calling the API:
-  // it's most certainly enough to run these tests with one algorithm only.
-
-  if (i > 0) {
-    continue;
-  }
-
-  {
-    // non-authenticating mode:
-    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/);
-  }
-
   {
     // trying to get tag before inputting all data:
     const encrypt = crypto.createCipheriv(test.algo,
@@ -452,3 +432,15 @@ for (var i in TEST_CASES) {
     }, /Invalid IV length/);
   }
 }
+
+// Non-authenticating mode:
+{
+  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/);
+}

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

@@ -61,5 +61,39 @@ testCipher1('0123456789abcd0123456789', '12345678');
 testCipher1('0123456789abcd0123456789', Buffer.from('12345678'));
 testCipher1(Buffer.from('0123456789abcd0123456789'), '12345678');
 testCipher1(Buffer.from('0123456789abcd0123456789'), Buffer.from('12345678'));
-
 testCipher2(Buffer.from('0123456789abcd0123456789'), Buffer.from('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));
+}