src: don't heap allocate GCM cipher auth tag

Fix a memory leak by removing the heap allocation altogether. Fixes: #13917 PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · Jul 19, 2017 · c84d0cd · c84d0cd
1 parent 3d4692b
commit c84d0cd
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 51 deletions.
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -3462,42 +3462,22 @@ bool CipherBase::IsAuthenticatedMode() const {
 }
 
 
-bool CipherBase::GetAuthTag(char** out, unsigned int* out_len) const {
-  // only callable after Final and if encrypting.
-  if (initialised_ || kind_ != kCipher || !auth_tag_)
-    return false;
-  *out_len = auth_tag_len_;
-  *out = node::Malloc(auth_tag_len_);
-  memcpy(*out, auth_tag_, auth_tag_len_);
-  return true;
-}
-
-
 void CipherBase::GetAuthTag(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   CipherBase* cipher;
   ASSIGN_OR_RETURN_UNWRAP(&cipher, args.Holder());
 
-  char* out = nullptr;
-  unsigned int out_len = 0;
-
-  if (cipher->GetAuthTag(&out, &out_len)) {
-    Local<Object> buf = Buffer::New(env, out, out_len).ToLocalChecked();
-    args.GetReturnValue().Set(buf);
-  } else {
-    env->ThrowError("Attempting to get auth tag in unsupported state");
+  // Only callable after Final and if encrypting.
+  if (cipher->initialised_ ||
+      cipher->kind_ != kCipher ||
+      cipher->auth_tag_len_ == 0) {
+    return env->ThrowError("Attempting to get auth tag in unsupported state");
   }
-}
-
 
-bool CipherBase::SetAuthTag(const char* data, unsigned int len) {
-  if (!initialised_ || !IsAuthenticatedMode() || kind_ != kDecipher)
-    return false;
-  delete[] auth_tag_;
-  auth_tag_len_ = len;
-  auth_tag_ = new char[len];
-  memcpy(auth_tag_, data, len);
-  return true;
+  Local<Object> buf =
+      Buffer::Copy(env, cipher->auth_tag_, cipher->auth_tag_len_)
+      .ToLocalChecked();
+  args.GetReturnValue().Set(buf);
 }
 
 
@@ -3509,8 +3489,20 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
   CipherBase* cipher;
   ASSIGN_OR_RETURN_UNWRAP(&cipher, args.Holder());
 
-  if (!cipher->SetAuthTag(Buffer::Data(args[0]), Buffer::Length(args[0])))
-    env->ThrowError("Attempting to set auth tag in unsupported state");
+  if (!cipher->initialised_ ||
+      !cipher->IsAuthenticatedMode() ||
+      cipher->kind_ != kDecipher) {
+    return env->ThrowError("Attempting to set auth tag in unsupported state");
+  }
+
+  // FIXME(bnoordhuis) Throw when buffer length is not a valid tag size.
+  // Note: we don't use std::max() here to work around a header conflict.
+  cipher->auth_tag_len_ = Buffer::Length(args[0]);
+  if (cipher->auth_tag_len_ > sizeof(cipher->auth_tag_))
+    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_);
 }
 
 
@@ -3550,13 +3542,12 @@ bool CipherBase::Update(const char* data,
     return 0;
 
   // on first update:
-  if (kind_ == kDecipher && IsAuthenticatedMode() && auth_tag_ != nullptr) {
+  if (kind_ == kDecipher && IsAuthenticatedMode() && auth_tag_len_ > 0) {
     EVP_CIPHER_CTX_ctrl(&ctx_,
                         EVP_CTRL_GCM_SET_TAG,
                         auth_tag_len_,
                         reinterpret_cast<unsigned char*>(auth_tag_));
-    delete[] auth_tag_;
-    auth_tag_ = nullptr;
+    auth_tag_len_ = 0;
   }
 
   *out_len = len + EVP_CIPHER_CTX_block_size(&ctx_);
@@ -3634,18 +3625,11 @@ bool CipherBase::Final(unsigned char** out, int *out_len) {
       static_cast<size_t>(EVP_CIPHER_CTX_block_size(&ctx_)));
   int r = EVP_CipherFinal_ex(&ctx_, *out, out_len);
 
-  if (r && kind_ == kCipher) {
-    delete[] auth_tag_;
-    auth_tag_ = nullptr;
-    if (IsAuthenticatedMode()) {
-      auth_tag_len_ = EVP_GCM_TLS_TAG_LEN;  // use default tag length
-      auth_tag_ = new char[auth_tag_len_];
-      memset(auth_tag_, 0, auth_tag_len_);
-      EVP_CIPHER_CTX_ctrl(&ctx_,
-                          EVP_CTRL_GCM_GET_TAG,
-                          auth_tag_len_,
-                          reinterpret_cast<unsigned char*>(auth_tag_));
-    }
+  if (r == 1 && kind_ == kCipher && IsAuthenticatedMode()) {
+    auth_tag_len_ = sizeof(auth_tag_);
+    r = EVP_CIPHER_CTX_ctrl(&ctx_, EVP_CTRL_GCM_GET_TAG, auth_tag_len_,
+                            reinterpret_cast<unsigned char*>(auth_tag_));
+    CHECK_EQ(r, 1);
   }
 
   EVP_CIPHER_CTX_cleanup(&ctx_);

diff --git a/src/node_crypto.h b/src/node_crypto.h
@@ -428,7 +428,6 @@ class CipherBase : public BaseObject {
   ~CipherBase() override {
     if (!initialised_)
       return;
-    delete[] auth_tag_;
     EVP_CIPHER_CTX_cleanup(&ctx_);
   }
 
@@ -451,8 +450,6 @@ class CipherBase : public BaseObject {
   bool SetAutoPadding(bool auto_padding);
 
   bool IsAuthenticatedMode() const;
-  bool GetAuthTag(char** out, unsigned int* out_len) const;
-  bool SetAuthTag(const char* data, unsigned int len);
   bool SetAAD(const char* data, unsigned int len);
 
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -473,7 +470,6 @@ class CipherBase : public BaseObject {
         cipher_(nullptr),
         initialised_(false),
         kind_(kind),
-        auth_tag_(nullptr),
         auth_tag_len_(0) {
     MakeWeak<CipherBase>(this);
   }
@@ -483,8 +479,8 @@ class CipherBase : public BaseObject {
   const EVP_CIPHER* cipher_; /* coverity[member_decl] */
   bool initialised_;
   CipherKind kind_;
-  char* auth_tag_;
   unsigned int auth_tag_len_;
+  char auth_tag_[EVP_GCM_TLS_TAG_LEN];
 };
 
 class Hmac : public BaseObject {