crypto: throw in setAuthTag on invalid length

The current implementation performs limited checks only and silently
ignores superfluous bytes of the authentication tag. This change makes
setAuthTag throw when
- the user-specified authTagLength does not match the actual tag length,
  especially when the authentication tag is longer than 16 bytes, and
  when
- the mode is GCM, no authTagLength option has been specified and the
  tag length is not a valid GCM tag length.

This change makes the conditional assignment in SetAuthTag unnecessary,
which is replaced with a CHECK.

Refs: https://github.com/nodejs/node/pull/17825

PR-URL: https://github.com/nodejs/node/pull/20040
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Yihong Wang <yh.wang@ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

2 files changed, 25 insertions, 20 deletions

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index bedc84b56b..d81853331b 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -2806,9 +2806,7 @@ bool CipherBase::InitAuthenticated(const char* cipher_type, int iv_len,
       return false;
     }
 
-    // When decrypting in CCM mode, this field will be set in setAuthTag().
-    if (kind_ == kCipher)
-      auth_tag_len_ = auth_tag_len;
+    auth_tag_len_ = auth_tag_len;
 
     // The message length is restricted to 2 ^ (8 * (15 - iv_len)) - 1 bytes.
     CHECK(iv_len >= 7 && iv_len <= 13);
@@ -2824,7 +2822,7 @@ bool CipherBase::InitAuthenticated(const char* cipher_type, int iv_len,
       if (!IsValidGCMTagLength(auth_tag_len)) {
         char msg[50];
         snprintf(msg, sizeof(msg),
-            "Invalid GCM authentication tag length: %u", auth_tag_len);
+            "Invalid authentication tag length: %u", auth_tag_len);
         env()->ThrowError(msg);
         return false;
       }
@@ -2891,21 +2889,26 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
   // Restrict GCM tag lengths according to NIST 800-38d, page 9.
   unsigned int tag_len = Buffer::Length(args[0]);
   const int mode = EVP_CIPHER_CTX_mode(cipher->ctx_.get());
+  bool is_valid;
   if (mode == EVP_CIPH_GCM_MODE) {
-    if ((cipher->auth_tag_len_ != kNoAuthTagLength &&
-        cipher->auth_tag_len_ != tag_len) ||
-        !IsValidGCMTagLength(tag_len)) {
-      char msg[50];
-      snprintf(msg, sizeof(msg),
-          "Invalid GCM authentication tag length: %u", tag_len);
-      return cipher->env()->ThrowError(msg);
-    }
+    is_valid = (cipher->auth_tag_len_ == kNoAuthTagLength ||
+                cipher->auth_tag_len_ == tag_len) &&
+               IsValidGCMTagLength(tag_len);
+  } else {
+    CHECK_EQ(mode, EVP_CIPH_CCM_MODE);
+    CHECK_NE(cipher->auth_tag_len_, kNoAuthTagLength);
+    is_valid = cipher->auth_tag_len_ == tag_len;
+  }
+
+  if (!is_valid) {
+    char msg[50];
+    snprintf(msg, sizeof(msg),
+        "Invalid authentication tag length: %u", tag_len);
+    return cipher->env()->ThrowError(msg);
   }
 
-  // Note: we don't use std::min() here to work around a header conflict.
   cipher->auth_tag_len_ = tag_len;
-  if (cipher->auth_tag_len_ > sizeof(cipher->auth_tag_))
-    cipher->auth_tag_len_ = sizeof(cipher->auth_tag_);
+  CHECK_LE(cipher->auth_tag_len_, sizeof(cipher->auth_tag_));
 
   memset(cipher->auth_tag_, 0, sizeof(cipher->auth_tag_));
   memcpy(cipher->auth_tag_, Buffer::Data(args[0]), cipher->auth_tag_len_);
diff --git a/test/parallel/test-crypto-authenticated.js b/test/parallel/test-crypto-authenticated.js
index e915db4e9d..ee91e31e9c 100644
--- a/test/parallel/test-crypto-authenticated.js
+++ b/test/parallel/test-crypto-authenticated.js
@@ -724,7 +724,7 @@ for (const test of TEST_CASES) {
       decrypt.setAuthTag(Buffer.from('1'.repeat(length)));
     }, {
       type: Error,
-      message: `Invalid GCM authentication tag length: ${length}`
+      message: `Invalid authentication tag length: ${length}`
     });
 
     common.expectsError(() => {
@@ -736,7 +736,7 @@ for (const test of TEST_CASES) {
                             });
     }, {
       type: Error,
-      message: `Invalid GCM authentication tag length: ${length}`
+      message: `Invalid authentication tag length: ${length}`
     });
 
     common.expectsError(() => {
@@ -748,7 +748,7 @@ for (const test of TEST_CASES) {
                               });
     }, {
       type: Error,
-      message: `Invalid GCM authentication tag length: ${length}`
+      message: `Invalid authentication tag length: ${length}`
     });
   }
 }
@@ -783,7 +783,7 @@ for (const test of TEST_CASES) {
     decipher.setAuthTag(Buffer.from('1'.repeat(12)));
   }, {
     type: Error,
-    message: 'Invalid GCM authentication tag length: 12'
+    message: 'Invalid authentication tag length: 12'
   });
 
   // The Decipher object should be left intact.
@@ -985,7 +985,7 @@ for (const test of TEST_CASES) {
   }
 }
 
-// Test that setAAD throws in CCM mode when no authentication tag is provided.
+// Test that final() throws in CCM mode when no authentication tag is provided.
 {
   if (!common.hasFipsCrypto) {
     const key = Buffer.from('1ed2233fa2223ef5d7df08546049406c', 'hex');
@@ -1000,6 +1000,8 @@ for (const test of TEST_CASES) {
       decrypt.setAAD(Buffer.from('63616c76696e', 'hex'), {
         plaintextLength: ct.length
       });
+      decrypt.update(ct);
+      decrypt.final();
     }, errMessages.state);
   }
 }