From 1ad660b72d6cea260188dc7d29b8199c8c84bffd 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] 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: https://github.com/nodejs/node/pull/23427 Reviewed-By: Anna Henningsen Reviewed-By: Refael Ackermann --- src/node_crypto.cc | 71 +++++++++++++++++++++++++--------------------- src/node_crypto.h | 13 ++++----- src/util.h | 7 ++++- 3 files changed, 50 insertions(+), 41 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index dd961b86b49eb7..490ec0df28936a 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 @@ -3523,46 +3524,51 @@ 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) { + sig.Truncate(sig_len); + return sig; } - 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, - 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, @@ -3573,7 +3579,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 */ @@ -3597,10 +3603,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)); } @@ -3624,22 +3629,22 @@ 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); - Error err = sign->SignFinal( + std::pair> 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(ret) != kSignOk) + return sign->CheckThrow(std::get(ret)); + + MallocedBuffer sig = + std::move(std::get>(ret)); Local rc = - Buffer::Copy(env, reinterpret_cast(md_value), md_len) + Buffer::New(env, reinterpret_cast(sig.release()), sig.size) .ToLocalChecked(); args.GetReturnValue().Set(rc); } diff --git a/src/node_crypto.h b/src/node_crypto.h index f4afd2fdaf5758..9603fcf3b2edb4 100644 --- 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 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> 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..880408df4d57fd 100644 --- 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(size)), size(size) {} MallocedBuffer(T* data, size_t size) : data(data), size(size) {} MallocedBuffer(MallocedBuffer&& other) : data(other.data), size(other.size) {