From 4b4b71b7d4df268b3e74c3f434682399bc50ce69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Thu, 11 Oct 2018 19:56:49 +0200 Subject: [PATCH 1/4] 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. --- src/node_crypto.cc | 42 ++++++++++++++++++++---------------------- src/node_crypto.h | 3 +-- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 96f4cf2ba7076d..7b108fde29eccf 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3517,36 +3517,38 @@ void Sign::SignUpdate(const FunctionCallbackInfo& 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 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(); + + int signed_sig_len = EVP_PKEY_size(pkey.get()); + CHECK_GE(signed_sig_len, 0); + size_t sig_len = static_cast(signed_sig_len); + MallocedBuffer sig(sig_len); - size_t sltmp = static_cast(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) { + return MallocedBuffer(sig.release(), sig_len); } - return 0; + + return MallocedBuffer(); } SignBase::Error Sign::SignFinal(const char* key_pem, int key_pem_len, const char* passphrase, - unsigned char* sig, - unsigned int* sig_len, + MallocedBuffer* buffer, int padding, int salt_len) { if (!mdctx_) @@ -3591,10 +3593,8 @@ 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); + return buffer->is_empty() ? kSignPrivateKey : kSignOk; } @@ -3618,22 +3618,20 @@ void Sign::SignFinal(const FunctionCallbackInfo& args) { int salt_len = args[3].As()->Value(); ClearErrorOnReturn clear_error_on_return; - unsigned char md_value[8192]; - unsigned int md_len = sizeof(md_value); + MallocedBuffer sig; Error err = sign->SignFinal( buf, buf_len, len >= 2 && !args[1]->IsNull() ? *passphrase : nullptr, - md_value, - &md_len, + &sig, padding, salt_len); if (err != kSignOk) return sign->CheckThrow(err); Local rc = - Buffer::Copy(env, reinterpret_cast(md_value), md_len) + Buffer::New(env, reinterpret_cast(sig.data), sig.size) .ToLocalChecked(); args.GetReturnValue().Set(rc); } diff --git a/src/node_crypto.h b/src/node_crypto.h index f4afd2fdaf5758..eac9049228b213 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -521,8 +521,7 @@ class Sign : public SignBase { Error SignFinal(const char* key_pem, int key_pem_len, const char* passphrase, - unsigned char* sig, - unsigned int* sig_len, + MallocedBuffer* sig, int padding, int saltlen); From e5bd9d0f90fd95b631fb05555b3cd03261e7936c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Fri, 12 Oct 2018 23:52:03 +0200 Subject: [PATCH 2/4] fixup! crypto: reduce memory usage of SignFinal --- src/node_crypto.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 7b108fde29eccf..554e817a88ba60 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3631,7 +3631,7 @@ void Sign::SignFinal(const FunctionCallbackInfo& args) { return sign->CheckThrow(err); Local rc = - Buffer::New(env, reinterpret_cast(sig.data), sig.size) + Buffer::New(env, reinterpret_cast(sig.release()), sig.size) .ToLocalChecked(); args.GetReturnValue().Set(rc); } From 202d506106664051ac76c5ebf54f952b27c017ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Mon, 15 Oct 2018 21:25:39 +0200 Subject: [PATCH 3/4] fixup! crypto: reduce memory usage of SignFinal --- src/node_crypto.cc | 41 ++++++++++++++++++++++++----------------- src/node_crypto.h | 12 ++++++------ src/util.h | 5 +++++ 3 files changed, 35 insertions(+), 23 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 554e817a88ba60..0d806d18ebe416 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -43,6 +43,7 @@ #include #include +#include #include static const int X509_NAME_FLAGS = ASN1_STRFLGS_ESC_CTRL @@ -3539,26 +3540,29 @@ static MallocedBuffer Node_SignFinal(EVPMDPointer&& mdctx, EVP_PKEY_CTX_set_signature_md(pkctx.get(), EVP_MD_CTX_md(mdctx.get())) > 0 && EVP_PKEY_sign(pkctx.get(), sig.data, &sig_len, m, m_len) > 0) { - return MallocedBuffer(sig.release(), sig_len); + sig.Truncate(sig_len); + return sig; } return MallocedBuffer(); } -SignBase::Error Sign::SignFinal(const char* key_pem, - int key_pem_len, - const char* passphrase, - MallocedBuffer* buffer, - int padding, - int salt_len) { +std::pair> Sign::SignFinal( + const char* key_pem, + int key_pem_len, + const char* passphrase, + int padding, + int salt_len) { + MallocedBuffer 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(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, @@ -3569,7 +3573,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 */ @@ -3593,8 +3597,9 @@ SignBase::Error Sign::SignFinal(const char* key_pem, } #endif // NODE_FIPS_MODE - *buffer = Node_SignFinal(std::move(mdctx), pkey, padding, salt_len); - return buffer->is_empty() ? kSignPrivateKey : kSignOk; + 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)); } @@ -3618,17 +3623,19 @@ void Sign::SignFinal(const FunctionCallbackInfo& args) { int salt_len = args[3].As()->Value(); ClearErrorOnReturn clear_error_on_return; - MallocedBuffer sig; - Error err = sign->SignFinal( + std::pair> ret = sign->SignFinal( buf, buf_len, len >= 2 && !args[1]->IsNull() ? *passphrase : nullptr, - &sig, padding, salt_len); - if (err != kSignOk) - return sign->CheckThrow(err); + + if (std::get(ret) != kSignOk) + return sign->CheckThrow(std::get(ret)); + + MallocedBuffer sig = + std::get>(std::move(ret)); Local rc = Buffer::New(env, reinterpret_cast(sig.release()), sig.size) diff --git a/src/node_crypto.h b/src/node_crypto.h index eac9049228b213..9603fcf3b2edb4 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -518,12 +518,12 @@ class Sign : public SignBase { public: static void Initialize(Environment* env, v8::Local target); - Error SignFinal(const char* key_pem, - int key_pem_len, - const char* passphrase, - MallocedBuffer* sig, - int padding, - int saltlen); + std::pair> SignFinal( + const char* key_pem, + int key_pem_len, + const char* passphrase, + int padding, + int saltlen); protected: static void New(const v8::FunctionCallbackInfo& args); diff --git a/src/util.h b/src/util.h index d41255bd32cc81..5b56f2aee973e6 100644 --- a/src/util.h +++ b/src/util.h @@ -439,6 +439,11 @@ 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) {} From 27f59473d55e03f8546182d4569f1ddfb160df38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Wed, 17 Oct 2018 21:33:10 +0200 Subject: [PATCH 4/4] fixup! crypto: reduce memory usage of SignFinal --- src/node_crypto.cc | 2 +- src/util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 0d806d18ebe416..fa9d278083563a 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3635,7 +3635,7 @@ void Sign::SignFinal(const FunctionCallbackInfo& args) { return sign->CheckThrow(std::get(ret)); MallocedBuffer sig = - std::get>(std::move(ret)); + std::move(std::get>(ret)); Local rc = Buffer::New(env, reinterpret_cast(sig.release()), sig.size) diff --git a/src/util.h b/src/util.h index 5b56f2aee973e6..880408df4d57fd 100644 --- a/src/util.h +++ b/src/util.h @@ -446,7 +446,7 @@ struct MallocedBuffer { inline bool is_empty() const { return data == nullptr; } - MallocedBuffer() : data(nullptr) {} + MallocedBuffer() : data(nullptr), size(0) {} explicit MallocedBuffer(size_t size) : data(Malloc(size)), size(size) {} MallocedBuffer(T* data, size_t size) : data(data), size(size) {} MallocedBuffer(MallocedBuffer&& other) : data(other.data), size(other.size) {