crypto: fix edge case in authenticated encryption

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>

Author: tniessen

src/node_crypto.cc

@@ -2891,6 +2891,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);
+
 unsignedint tag_len = Buffer::Length(args[0]);
 constint mode = EVP_CIPHER_CTX_mode(cipher->ctx_.get());
 if (mode == EVP_CIPH_GCM_MODE) {
@@ -2923,6 +2927,7 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
 
 // Note: we don't use std::min() here to work around a header conflict.
  cipher->auth_tag_len_ = tag_len;
+ cipher->auth_tag_state_ = kAuthTagKnown;
 if (cipher->auth_tag_len_ > sizeof(cipher->auth_tag_))
  cipher->auth_tag_len_ = sizeof(cipher->auth_tag_);
 
@@ -2934,14 +2939,14 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
 
 
 boolCipherBase::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<unsignedchar*>(auth_tag_))) {
 returnfalse;
  }
-auth_tag_set_ = true;
+auth_tag_state_ = kAuthTagPassedToOpenSSL;
  }
 returntrue;
 }

src/node_crypto.h

@@ -363,6 +363,11 @@ class CipherBase : public BaseObject {
 kErrorMessageSize,
 kErrorState
  };
+enum AuthTagState {
+kAuthTagUnknown,
+kAuthTagKnown,
+kAuthTagPassedToOpenSSL
+ };
 staticconstunsignedkNoAuthTagLength = static_cast<unsigned>(-1);
 
 voidInit(constchar* 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_;
 unsignedint auth_tag_len_;
 char auth_tag_[EVP_GCM_TLS_TAG_LEN];
 bool pending_auth_failed_;

test/parallel/test-crypto-authenticated.js

@@ -579,27 +579,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.
 {
 constplain=Buffer.from('Hello world','utf8');
 constkey=Buffer.from('0123456789abcdef','utf8');
 constiv=Buffer.from('0123456789ab','utf8');
 
-constcipher=crypto.createCipheriv('aes-128-gcm',key,iv);
-constciphertext=Buffer.concat([cipher.update(plain),cipher.final()]);
-constauthTag=cipher.getAuthTag();
-
-for(constauthTagBeforeUpdateof[true,false]){
-constdecipher=crypto.createDecipheriv('aes-128-gcm',key,iv);
-if(authTagBeforeUpdate){
-decipher.setAuthTag(authTag);
-}
-constresultUpdate=decipher.update(ciphertext);
-if(!authTagBeforeUpdate){
-decipher.setAuthTag(authTag);
+for(constmodeof['gcm','ocb']){
+for(constauthTagLengthofmode==='gcm' ? [undefined,8] : [8]){
+constcipher=crypto.createCipheriv(`aes-128-${mode}`,key,iv,{
+ authTagLength
+});
+constciphertext=Buffer.concat([cipher.update(plain),cipher.final()]);
+constauthTag=cipher.getAuthTag();
+
+for(constauthTagBeforeUpdateof[true,false]){
+constdecipher=crypto.createDecipheriv(`aes-128-${mode}`,key,iv,{
+ authTagLength
+});
+if(authTagBeforeUpdate){
+decipher.setAuthTag(authTag);
+}
+constresultUpdate=decipher.update(ciphertext);
+if(!authTagBeforeUpdate){
+decipher.setAuthTag(authTag);
+}
+constresultFinal=decipher.final();
+constresult=Buffer.concat([resultUpdate,resultFinal]);
+assert(result.equals(plain));
+}
 }
-constresultFinal=decipher.final();
-constresult=Buffer.concat([resultUpdate,resultFinal]);
-assert(result.equals(plain));
 }
 }