crypto: reduce memory usage of SignFinal

The fixed-size buffer on the stack is unnecessary and way too large for most applications. This change removes it and allocates the required memory directly instead of copying into heap later. PR-URL: #23427 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
nodejs · Dec 26, 2018 · 4749640 · 4749640
1 parent ffb4087
commit 4749640
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 41 deletions.
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -43,6 +43,7 @@
 
 #include <algorithm>
 #include <memory>
+#include <utility>
 #include <vector>
 
 static const int X509_NAME_FLAGS = ASN1_STRFLGS_ESC_CTRL
@@ -3530,46 +3531,51 @@ void Sign::SignUpdate(const FunctionCallbackInfo<Value>& args) {
   sign->CheckThrow(err);
 }
 
-static int Node_SignFinal(EVPMDPointer&& mdctx, unsigned char* md,
-                          unsigned int* sig_len,
-                          const EVPKeyPointer& pkey, int padding,
-                          int pss_salt_len) {
+static MallocedBuffer<unsigned char> Node_SignFinal(EVPMDPointer&& mdctx,
+                                                    const EVPKeyPointer& pkey,
+                                                    int padding,
+                                                    int pss_salt_len) {
   unsigned char m[EVP_MAX_MD_SIZE];
   unsigned int m_len;
 
-  *sig_len = 0;
   if (!EVP_DigestFinal_ex(mdctx.get(), m, &m_len))
-    return 0;
+    return MallocedBuffer<unsigned char>();
+
+  int signed_sig_len = EVP_PKEY_size(pkey.get());
+  CHECK_GE(signed_sig_len, 0);
+  size_t sig_len = static_cast<size_t>(signed_sig_len);
+  MallocedBuffer<unsigned char> sig(sig_len);
 
-  size_t sltmp = static_cast<size_t>(EVP_PKEY_size(pkey.get()));
   EVPKeyCtxPointer pkctx(EVP_PKEY_CTX_new(pkey.get(), nullptr));
   if (pkctx &&
       EVP_PKEY_sign_init(pkctx.get()) > 0 &&
       ApplyRSAOptions(pkey, pkctx.get(), padding, pss_salt_len) &&
       EVP_PKEY_CTX_set_signature_md(pkctx.get(),
                                     EVP_MD_CTX_md(mdctx.get())) > 0 &&
-      EVP_PKEY_sign(pkctx.get(), md, &sltmp, m, m_len) > 0) {
-    *sig_len = sltmp;
-    return 1;
+      EVP_PKEY_sign(pkctx.get(), sig.data, &sig_len, m, m_len) > 0) {
+    sig.Truncate(sig_len);
+    return sig;
   }
-  return 0;
+
+  return MallocedBuffer<unsigned char>();
 }
 
-SignBase::Error Sign::SignFinal(const char* key_pem,
-                                int key_pem_len,
-                                const char* passphrase,
-                                unsigned char* sig,
-                                unsigned int* sig_len,
-                                int padding,
-                                int salt_len) {
+std::pair<SignBase::Error, MallocedBuffer<unsigned char>> Sign::SignFinal(
+    const char* key_pem,
+    int key_pem_len,
+    const char* passphrase,
+    int padding,
+    int salt_len) {
+  MallocedBuffer<unsigned char> buffer;
+
   if (!mdctx_)
-    return kSignNotInitialised;
+    return std::make_pair(kSignNotInitialised, std::move(buffer));
 
   EVPMDPointer mdctx = std::move(mdctx_);
 
   BIOPointer bp(BIO_new_mem_buf(const_cast<char*>(key_pem), key_pem_len));
   if (!bp)
-    return kSignPrivateKey;
+    return std::make_pair(kSignPrivateKey, std::move(buffer));
 
   EVPKeyPointer pkey(PEM_read_bio_PrivateKey(bp.get(),
                                              nullptr,
@@ -3580,7 +3586,7 @@ SignBase::Error Sign::SignFinal(const char* key_pem,
   // without `pkey` being set to nullptr;
   // cf. the test of `test_bad_rsa_privkey.pem` for an example.
   if (!pkey || 0 != ERR_peek_error())
-    return kSignPrivateKey;
+    return std::make_pair(kSignPrivateKey, std::move(buffer));
 
 #ifdef NODE_FIPS_MODE
   /* Validate DSA2 parameters from FIPS 186-4 */
@@ -3604,10 +3610,9 @@ SignBase::Error Sign::SignFinal(const char* key_pem,
   }
 #endif  // NODE_FIPS_MODE
 
-  if (Node_SignFinal(std::move(mdctx), sig, sig_len, pkey, padding, salt_len))
-    return kSignOk;
-  else
-    return kSignPrivateKey;
+  buffer = Node_SignFinal(std::move(mdctx), pkey, padding, salt_len);
+  Error error = buffer.is_empty() ? kSignPrivateKey : kSignOk;
+  return std::make_pair(error, std::move(buffer));
 }
 
 
@@ -3631,22 +3636,22 @@ void Sign::SignFinal(const FunctionCallbackInfo<Value>& args) {
   int salt_len = args[3].As<Int32>()->Value();
 
   ClearErrorOnReturn clear_error_on_return;
-  unsigned char md_value[8192];
-  unsigned int md_len = sizeof(md_value);
 
-  Error err = sign->SignFinal(
+  std::pair<Error, MallocedBuffer<unsigned char>> ret = sign->SignFinal(
       buf,
       buf_len,
       len >= 2 && !args[1]->IsNull() ? *passphrase : nullptr,
-      md_value,
-      &md_len,
       padding,
       salt_len);
-  if (err != kSignOk)
-    return sign->CheckThrow(err);
+
+  if (std::get<Error>(ret) != kSignOk)
+    return sign->CheckThrow(std::get<Error>(ret));
+
+  MallocedBuffer<unsigned char> sig =
+      std::move(std::get<MallocedBuffer<unsigned char>>(ret));
 
   Local<Object> rc =
-      Buffer::Copy(env, reinterpret_cast<char*>(md_value), md_len)
+      Buffer::New(env, reinterpret_cast<char*>(sig.release()), sig.size)
       .ToLocalChecked();
   args.GetReturnValue().Set(rc);
 }

diff --git a/src/node_crypto.h b/src/node_crypto.h
@@ -518,13 +518,12 @@ class Sign : public SignBase {
  public:
   static void Initialize(Environment* env, v8::Local<v8::Object> target);
 
-  Error SignFinal(const char* key_pem,
-                  int key_pem_len,
-                  const char* passphrase,
-                  unsigned char* sig,
-                  unsigned int* sig_len,
-                  int padding,
-                  int saltlen);
+  std::pair<Error, MallocedBuffer<unsigned char>> SignFinal(
+      const char* key_pem,
+      int key_pem_len,
+      const char* passphrase,
+      int padding,
+      int saltlen);
 
  protected:
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);

diff --git a/src/util.h b/src/util.h
@@ -439,9 +439,14 @@ struct MallocedBuffer {
     return ret;
   }
 
+  void Truncate(size_t new_size) {
+    CHECK(new_size <= size);
+    size = new_size;
+  }
+
   inline bool is_empty() const { return data == nullptr; }
 
-  MallocedBuffer() : data(nullptr) {}
+  MallocedBuffer() : data(nullptr), size(0) {}
   explicit MallocedBuffer(size_t size) : data(Malloc<T>(size)), size(size) {}
   MallocedBuffer(T* data, size_t size) : data(data), size(size) {}
   MallocedBuffer(MallocedBuffer&& other) : data(other.data), size(other.size) {