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: #22421 PR-URL: #22538 Reviewed-By: Anna Henningsen <anna@addaleax.net>
nodejs · Sep 3, 2018 · c47c79e · c47c79e
1 parent d3740d8
commit c47c79e
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 16 deletions.
diff --git 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
@@ -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
@@ -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));
+  }
+}