crypto: do not allow multiple calls to setAuthTag

Calling setAuthTag multiple times can result in hard to detect bugs since to the user, it is unclear which invocation actually affected OpenSSL. PR-URL: #22931 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
nodejs · Sep 21, 2018 · 058c5b8 · 058c5b8
1 parent 56493bf
commit 058c5b8
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 6 deletions.
diff --git a/doc/api/crypto.md b/doc/api/crypto.md
@@ -445,7 +445,7 @@ is invalid according to [NIST SP 800-38D][] or does not match the value of the
 `authTagLength` option, `decipher.setAuthTag()` will throw an error.
 
 The `decipher.setAuthTag()` method must be called before
-[`decipher.final()`][].
+[`decipher.final()`][] and can only be called once.
 
 ### decipher.setAutoPadding([autoPadding])
 <!-- YAML

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -2894,14 +2894,11 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
 
   if (!cipher->ctx_ ||
       !cipher->IsAuthenticatedMode() ||
-      cipher->kind_ != kDecipher) {
+      cipher->kind_ != kDecipher ||
+      cipher->auth_tag_state_ != kAuthTagUnknown) {
     return args.GetReturnValue().Set(false);
   }
 
-  // TODO(tniessen): Throw if the authentication tag has already been set.
-  if (cipher->auth_tag_state_ == kAuthTagPassedToOpenSSL)
-    return args.GetReturnValue().Set(true);
-
   unsigned int tag_len = Buffer::Length(args[0]);
   const int mode = EVP_CIPHER_CTX_mode(cipher->ctx_.get());
   bool is_valid;

diff --git a/test/parallel/test-crypto-authenticated.js b/test/parallel/test-crypto-authenticated.js
@@ -589,3 +589,29 @@ for (const test of TEST_CASES) {
     }
   }
 }
+
+// Test that setAuthTag can only be called once.
+{
+  const plain = Buffer.from('Hello world', 'utf8');
+  const key = Buffer.from('0123456789abcdef', 'utf8');
+  const iv = Buffer.from('0123456789ab', 'utf8');
+  const opts = { authTagLength: 8 };
+
+  for (const mode of ['gcm', 'ccm', 'ocb']) {
+    const cipher = crypto.createCipheriv(`aes-128-${mode}`, key, iv, opts);
+    const ciphertext = Buffer.concat([cipher.update(plain), cipher.final()]);
+    const tag = cipher.getAuthTag();
+
+    const decipher = crypto.createDecipheriv(`aes-128-${mode}`, key, iv, opts);
+    decipher.setAuthTag(tag);
+    assert.throws(() => {
+      decipher.setAuthTag(tag);
+    }, errMessages.state);
+    // Decryption should still work.
+    const plaintext = Buffer.concat([
+      decipher.update(ciphertext),
+      decipher.final()
+    ]);
+    assert(plain.equals(plaintext));
+  }
+}