crypto: fix edge case in authenticated encryption

tniessen · tniessen · commit a9e7369b117f · 2018-09-18T12:55:07.000+02:00
Restricting the authentication tag length and calling update or setAAD before setAuthTag caused an incorrect authentication tag to be passed to OpenSSL: The auth_tag_len_ field was already set, so the implementation assumed that the tag itself was known as well. PR-URL: #22828 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -2897,6 +2897,10 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
     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;
@@ -2921,6 +2925,7 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
   }
 
   cipher->auth_tag_len_ = tag_len;
+  cipher->auth_tag_state_ = kAuthTagKnown;
   CHECK_LE(cipher->auth_tag_len_, sizeof(cipher->auth_tag_));
 
   memset(cipher->auth_tag_, 0, sizeof(cipher->auth_tag_));
@@ -2931,14 +2936,14 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
 
 
 bool CipherBase::MaybePassAuthTagToOpenSSL() {
-  if (!auth_tag_set_ && auth_tag_len_ != kNoAuthTagLength) {
+  if (auth_tag_state_ == kAuthTagKnown) {
     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;
+    auth_tag_state_ = kAuthTagPassedToOpenSSL;
   }
   return true;
 }
diff --git a/src/node_crypto.h b/src/node_crypto.h
@@ -363,6 +363,11 @@ class CipherBase : public BaseObject {
     kErrorMessageSize,
     kErrorState
   };
+  enum AuthTagState {
+    kAuthTagUnknown,
+    kAuthTagKnown,
+    kAuthTagPassedToOpenSSL
+  };
   static const unsigned kNoAuthTagLength = static_cast<unsigned>(-1);
 
   void Init(const char* cipher_type,
@@ -404,7 +409,7 @@ class CipherBase : public BaseObject {
       : BaseObject(env, wrap),
         ctx_(nullptr),
         kind_(kind),
-        auth_tag_set_(false),
+        auth_tag_state_(kAuthTagUnknown),
         auth_tag_len_(kNoAuthTagLength),
         pending_auth_failed_(false) {
     MakeWeak();
@@ -413,7 +418,7 @@ class CipherBase : public BaseObject {
  private:
   DeleteFnPtr<EVP_CIPHER_CTX, EVP_CIPHER_CTX_free> ctx_;
   const CipherKind kind_;
-  bool auth_tag_set_;
+  AuthTagState auth_tag_state_;
   unsigned int auth_tag_len_;
   char auth_tag_[EVP_GCM_TLS_TAG_LEN];
   bool pending_auth_failed_;
diff --git a/test/parallel/test-crypto-authenticated.js b/test/parallel/test-crypto-authenticated.js
@@ -557,27 +557,35 @@ for (const test of TEST_CASES) {
 }
 
 // Test that the authentication tag can be set at any point before calling
-// final() in GCM mode.
+// final() in GCM or OCB 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);
+  for (const mode of ['gcm', 'ocb']) {
+    for (const authTagLength of mode === 'gcm' ? [undefined, 8] : [8]) {
+      const cipher = crypto.createCipheriv(`aes-128-${mode}`, key, iv, {
+        authTagLength
+      });
+      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-${mode}`, key, iv, {
+          authTagLength
+        });
+        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));
+      }
     }
-    const resultFinal = decipher.final();
-    const result = Buffer.concat([resultUpdate, resultFinal]);
-    assert(result.equals(plain));
   }
 }

Original file line number	Diff line number	Diff line change
`@@ -2897,6 +2897,10 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {`
`2897`	`2897`	`return args.GetReturnValue().Set(false);`
`2898`	`2898`	`}`
`2899`	`2899`
	`2900`	`+ // TODO(tniessen): Throw if the authentication tag has already been set.`
	`2901`	`+ if (cipher->auth_tag_state_ == kAuthTagPassedToOpenSSL)`
	`2902`	`+ return args.GetReturnValue().Set(true);`
	`2903`	`+`
`2900`	`2904`	`unsigned int tag_len = Buffer::Length(args[0]);`
`2901`	`2905`	`const int mode = EVP_CIPHER_CTX_mode(cipher->ctx_.get());`
`2902`	`2906`	`bool is_valid;`
`@@ -2921,6 +2925,7 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {`
`2921`	`2925`	`}`
`2922`	`2926`
`2923`	`2927`	`cipher->auth_tag_len_ = tag_len;`
	`2928`	`+ cipher->auth_tag_state_ = kAuthTagKnown;`
`2924`	`2929`	`CHECK_LE(cipher->auth_tag_len_, sizeof(cipher->auth_tag_));`
`2925`	`2930`
`2926`	`2931`	`memset(cipher->auth_tag_, 0, sizeof(cipher->auth_tag_));`
`@@ -2931,14 +2936,14 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {`
`2931`	`2936`
`2932`	`2937`
`2933`	`2938`	`bool CipherBase::MaybePassAuthTagToOpenSSL() {`
`2934`		`- if (!auth_tag_set_ && auth_tag_len_ != kNoAuthTagLength) {`
	`2939`	`+ if (auth_tag_state_ == kAuthTagKnown) {`
`2935`	`2940`	`if (!EVP_CIPHER_CTX_ctrl(ctx_.get(),`
`2936`	`2941`	`EVP_CTRL_AEAD_SET_TAG,`
`2937`	`2942`	`auth_tag_len_,`
`2938`	`2943`	`reinterpret_cast<unsigned char*>(auth_tag_))) {`
`2939`	`2944`	`return false;`
`2940`	`2945`	`}`
`2941`		`- auth_tag_set_ = true;`
	`2946`	`+ auth_tag_state_ = kAuthTagPassedToOpenSSL;`
`2942`	`2947`	`}`
`2943`	`2948`	`return true;`
`2944`	`2949`	`}`