From 058c5b81cdbabe8989a194ba5d388f4c230f4af6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= <tniessen@tnie.de>
Date: Tue, 18 Sep 2018 14:14:50 +0200
Subject: [PATCH] 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: https://github.com/nodejs/node/pull/22931
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
---
 doc/api/crypto.md                          |  2 +-
 src/node_crypto.cc                         |  7 ++----
 test/parallel/test-crypto-authenticated.js | 26 ++++++++++++++++++++++
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/doc/api/crypto.md b/doc/api/crypto.md
index b26e9cae04a..ecb0c8186c7 100644
--- 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
index 47e0ba5349e..4cf3ac5652b 100644
--- 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
index 77587fadf79..ec5c05cb120 100644
--- 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));
+  }
+}