From 85932667875898df453228b8a9f14b5f8af1d8a4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= <tniessen@tnie.de>
Date: Sun, 26 Aug 2018 19:17:09 +0200
Subject: [PATCH] crypto: improve setAuthTag

This is an attempt to make the behavior of setAuthTag match the
documentation: In GCM mode, it can be called at any time before
invoking final, even after the last call to update.

Fixes: https://github.com/nodejs/node/issues/22421

PR-URL: https://github.com/nodejs/node/pull/22538
Reviewed-By: Anna Henningsen <anna@addaleax.net>
---
 src/node_crypto.cc                         | 40 +++++++++++++---------
 src/node_crypto.h                          |  1 +
 test/parallel/test-crypto-authenticated.js | 26 ++++++++++++++
 3 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index b24b541d725f4c..c28ae3098bf239 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -2931,6 +2931,20 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
 }
 
 
+bool CipherBase::MaybePassAuthTagToOpenSSL() {
+  if (!auth_tag_set_ && auth_tag_len_ != kNoAuthTagLength) {
+    if (!EVP_CIPHER_CTX_ctrl(ctx_.get(),
+                             EVP_CTRL_AEAD_SET_TAG,
+                             auth_tag_len_,
+                             reinterpret_cast<unsigned char*>(auth_tag_))) {
+      return false;
+    }
+    auth_tag_set_ = true;
+  }
+  return true;
+}
+
+
 bool CipherBase::SetAAD(const char* data, unsigned int len, int plaintext_len) {
   if (!ctx_ || !IsAuthenticatedMode())
     return false;
@@ -2950,15 +2964,9 @@ bool CipherBase::SetAAD(const char* data, unsigned int len, int plaintext_len) {
     if (!CheckCCMMessageLength(plaintext_len))
       return false;
 
-    if (kind_ == kDecipher && !auth_tag_set_ && auth_tag_len_ > 0 &&
-        auth_tag_len_ != kNoAuthTagLength) {
-      if (!EVP_CIPHER_CTX_ctrl(ctx_.get(),
-                               EVP_CTRL_CCM_SET_TAG,
-                               auth_tag_len_,
-                               reinterpret_cast<unsigned char*>(auth_tag_))) {
+    if (kind_ == kDecipher) {
+      if (!MaybePassAuthTagToOpenSSL())
         return false;
-      }
-      auth_tag_set_ = true;
     }
 
     // Specify the plaintext length.
@@ -3003,14 +3011,10 @@ CipherBase::UpdateResult CipherBase::Update(const char* data,
       return kErrorMessageSize;
   }
 
-  // on first update:
-  if (kind_ == kDecipher && IsAuthenticatedMode() && auth_tag_len_ > 0 &&
-      auth_tag_len_ != kNoAuthTagLength && !auth_tag_set_) {
-    CHECK(EVP_CIPHER_CTX_ctrl(ctx_.get(),
-                              EVP_CTRL_AEAD_SET_TAG,
-                              auth_tag_len_,
-                              reinterpret_cast<unsigned char*>(auth_tag_)));
-    auth_tag_set_ = true;
+  // Pass the authentication tag to OpenSSL if possible. This will only happen
+  // once, usually on the first update.
+  if (kind_ == kDecipher && IsAuthenticatedMode()) {
+    CHECK(MaybePassAuthTagToOpenSSL());
   }
 
   *out_len = 0;
@@ -3110,6 +3114,10 @@ bool CipherBase::Final(unsigned char** out, int* out_len) {
   *out = Malloc<unsigned char>(
       static_cast<size_t>(EVP_CIPHER_CTX_block_size(ctx_.get())));
 
+  if (kind_ == kDecipher && IsSupportedAuthenticatedMode(mode)) {
+    MaybePassAuthTagToOpenSSL();
+  }
+
   // In CCM mode, final() only checks whether authentication failed in update().
   // EVP_CipherFinal_ex must not be called and will fail.
   bool ok;
diff --git a/src/node_crypto.h b/src/node_crypto.h
index 692978412fc023..86aa3ba4ba8395 100644
--- a/src/node_crypto.h
+++ b/src/node_crypto.h
@@ -385,6 +385,7 @@ class CipherBase : public BaseObject {
 
   bool IsAuthenticatedMode() const;
   bool SetAAD(const char* data, unsigned int len, int plaintext_len);
+  bool MaybePassAuthTagToOpenSSL();
 
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void Init(const v8::FunctionCallbackInfo<v8::Value>& args);
diff --git a/test/parallel/test-crypto-authenticated.js b/test/parallel/test-crypto-authenticated.js
index 29ebad3108570c..4b2d8526ddb46e 100644
--- a/test/parallel/test-crypto-authenticated.js
+++ b/test/parallel/test-crypto-authenticated.js
@@ -577,3 +577,29 @@ for (const test of TEST_CASES) {
   encrypt.update('boom');  // Should not throw 'Message exceeds maximum size'.
   encrypt.final();
 }
+
+// Test that the authentication tag can be set at any point before calling
+// final() in GCM mode.
+{
+  const plain = Buffer.from('Hello world', 'utf8');
+  const key = Buffer.from('0123456789abcdef', 'utf8');
+  const iv = Buffer.from('0123456789ab', 'utf8');
+
+  const cipher = crypto.createCipheriv('aes-128-gcm', key, iv);
+  const ciphertext = Buffer.concat([cipher.update(plain), cipher.final()]);
+  const authTag = cipher.getAuthTag();
+
+  for (const authTagBeforeUpdate of [true, false]) {
+    const decipher = crypto.createDecipheriv('aes-128-gcm', key, iv);
+    if (authTagBeforeUpdate) {
+      decipher.setAuthTag(authTag);
+    }
+    const resultUpdate = decipher.update(ciphertext);
+    if (!authTagBeforeUpdate) {
+      decipher.setAuthTag(authTag);
+    }
+    const resultFinal = decipher.final();
+    const result = Buffer.concat([resultUpdate, resultFinal]);
+    assert(result.equals(plain));
+  }
+}