From a9d1431943660995656c1c04449f5bac3494faa9 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 01/18] src: remove extra heap allocations in CipherBase Don't allocate + copy + free; allocate and fill in place, then hand off the pointer to Buffer::New(). Avoids unnecessary heap allocations in the following methods: - crypto.Cipher#final() - crypto.Cipher#update() - crypto.Cipheriv#final() - crypto.Cipheriv#update() - crypto.Decipher#final() - crypto.Decipher#update() - crypto.Decipheriv#final() - crypto.Decipheriv#update() - crypto.privateDecrypt() - crypto.privateEncrypt() - crypto.publicDecrypt() - crypto.publicEncrypt() --- src/node_crypto.cc | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 36102e53f9c045..26445f1b833686 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3561,7 +3561,7 @@ bool CipherBase::Update(const char* data, } *out_len = len + EVP_CIPHER_CTX_block_size(&ctx_); - *out = new unsigned char[*out_len]; + *out = Malloc(static_cast(*out_len)); return EVP_CipherUpdate(&ctx_, *out, out_len, @@ -3595,7 +3595,7 @@ void CipherBase::Update(const FunctionCallbackInfo& args) { } if (!r) { - delete[] out; + free(out); return ThrowCryptoError(env, ERR_get_error(), "Trying to add data in unsupported state"); @@ -3603,9 +3603,7 @@ void CipherBase::Update(const FunctionCallbackInfo& args) { CHECK(out != nullptr || out_len == 0); Local buf = - Buffer::Copy(env, reinterpret_cast(out), out_len).ToLocalChecked(); - if (out) - delete[] out; + Buffer::New(env, reinterpret_cast(out), out_len).ToLocalChecked(); args.GetReturnValue().Set(buf); } @@ -3633,7 +3631,8 @@ bool CipherBase::Final(unsigned char** out, int *out_len) { if (!initialised_) return false; - *out = new unsigned char[EVP_CIPHER_CTX_block_size(&ctx_)]; + *out = Malloc( + static_cast(EVP_CIPHER_CTX_block_size(&ctx_))); int r = EVP_CipherFinal_ex(&ctx_, *out, out_len); if (r && kind_ == kCipher) { @@ -3670,7 +3669,7 @@ void CipherBase::Final(const FunctionCallbackInfo& args) { bool r = cipher->Final(&out_value, &out_len); if (out_len <= 0 || !r) { - delete[] out_value; + free(out_value); out_value = nullptr; out_len = 0; if (!r) { @@ -3684,12 +3683,11 @@ void CipherBase::Final(const FunctionCallbackInfo& args) { } } - Local buf = Buffer::Copy( + Local buf = Buffer::New( env, reinterpret_cast(out_value), out_len).ToLocalChecked(); args.GetReturnValue().Set(buf); - delete[] out_value; } @@ -4560,7 +4558,7 @@ bool PublicKeyCipher::Cipher(const char* key_pem, if (EVP_PKEY_cipher(ctx, nullptr, out_len, data, len) <= 0) goto exit; - *out = new unsigned char[*out_len]; + *out = Malloc(*out_len); if (EVP_PKEY_cipher(ctx, *out, out_len, data, len) <= 0) goto exit; @@ -4615,7 +4613,7 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo& args) { &out_len); if (out_len == 0 || !r) { - delete[] out_value; + free(out_value); out_value = nullptr; out_len = 0; if (!r) { @@ -4624,12 +4622,10 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo& args) { } } - Local vbuf = Buffer::Copy( - env, - reinterpret_cast(out_value), - out_len).ToLocalChecked(); + Local vbuf = + Buffer::New(env, reinterpret_cast(out_value), out_len) + .ToLocalChecked(); args.GetReturnValue().Set(vbuf); - delete[] out_value; } From df1e3e8df1e63f49012bd52c487f395bf038bc53 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 02/18] src: remove unneeded const_cast --- src/node_crypto.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 26445f1b833686..9ba146c82b4416 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2373,10 +2373,9 @@ int SSLWrap::TLSExtStatusCallback(SSL* s, void* arg) { if (resp == nullptr) { arg = Null(env->isolate()); } else { - arg = Buffer::Copy( - env, - reinterpret_cast(const_cast(resp)), - len).ToLocalChecked(); + arg = + Buffer::Copy(env, reinterpret_cast(resp), len) + .ToLocalChecked(); } w->MakeCallback(env->onocspresponse_string(), 1, &arg); From 87fc516e542e96683c11cfdb5491fd21dd13ca70 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 03/18] src: avoid heap allocation in sign.final() Put the 8 kB initial buffer on the stack first and don't copy it to the heap until its exact size is known (which is normally much smaller.) --- src/node_crypto.cc | 27 +++++++++------------------ src/node_crypto.h | 2 +- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 9ba146c82b4416..47df568c2d3d16 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4147,7 +4147,7 @@ static int Node_SignFinal(EVP_MD_CTX* mdctx, unsigned char* md, SignBase::Error Sign::SignFinal(const char* key_pem, int key_pem_len, const char* passphrase, - unsigned char** sig, + unsigned char* sig, unsigned int* sig_len, int padding, int salt_len) { @@ -4196,7 +4196,7 @@ SignBase::Error Sign::SignFinal(const char* key_pem, } #endif // NODE_FIPS_MODE - if (Node_SignFinal(&mdctx_, *sig, sig_len, pkey, padding, salt_len)) + if (Node_SignFinal(&mdctx_, sig, sig_len, pkey, padding, salt_len)) fatal = false; initialised_ = false; @@ -4222,9 +4222,6 @@ void Sign::SignFinal(const FunctionCallbackInfo& args) { Sign* sign; ASSIGN_OR_RETURN_UNWRAP(&sign, args.Holder()); - unsigned char* md_value; - unsigned int md_len; - unsigned int len = args.Length(); node::Utf8Value passphrase(env->isolate(), args[1]); @@ -4243,30 +4240,24 @@ void Sign::SignFinal(const FunctionCallbackInfo& args) { CHECK(maybe_salt_len.IsJust()); int salt_len = maybe_salt_len.ToChecked(); - md_len = 8192; // Maximum key size is 8192 bits - md_value = new unsigned char[md_len]; - ClearErrorOnReturn clear_error_on_return; + unsigned char md_value[8192]; + unsigned int md_len = sizeof(md_value); Error err = sign->SignFinal( buf, buf_len, len >= 2 && !args[1]->IsNull() ? *passphrase : nullptr, - &md_value, + md_value, &md_len, padding, salt_len); - if (err != kSignOk) { - delete[] md_value; - md_value = nullptr; - md_len = 0; + if (err != kSignOk) return sign->CheckThrow(err); - } - Local rc = Buffer::Copy(env->isolate(), - reinterpret_cast(md_value), - md_len).ToLocalChecked(); - delete[] md_value; + Local rc = + Buffer::Copy(env, reinterpret_cast(md_value), md_len) + .ToLocalChecked(); args.GetReturnValue().Set(rc); } diff --git a/src/node_crypto.h b/src/node_crypto.h index 33c9cf783ecedb..c1ba349d2d645d 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -587,7 +587,7 @@ class Sign : public SignBase { Error SignFinal(const char* key_pem, int key_pem_len, const char* passphrase, - unsigned char** sig, + unsigned char* sig, unsigned int *sig_len, int padding, int saltlen); From 647370a282d7f314e134542df99c20219821b2ed Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 04/18] src: don't heap allocate GCM cipher auth tag Fix a memory leak by removing the heap allocation altogether. Fixes: https://github.com/nodejs/node/issues/13917 --- src/node_crypto.cc | 76 ++++++++++++++++++---------------------------- src/node_crypto.h | 6 +--- 2 files changed, 31 insertions(+), 51 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 47df568c2d3d16..31096b6a82d4a3 100644 --- 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& 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 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 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& 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(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(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(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(auth_tag_)); + CHECK_EQ(r, 1); } EVP_CIPHER_CTX_cleanup(&ctx_); diff --git a/src/node_crypto.h b/src/node_crypto.h index c1ba349d2d645d..db7a97920be156 100644 --- 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& args); @@ -473,7 +470,6 @@ class CipherBase : public BaseObject { cipher_(nullptr), initialised_(false), kind_(kind), - auth_tag_(nullptr), auth_tag_len_(0) { MakeWeak(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 { From f46e980d02ce78b3a2fe27b96f45de86b70936a1 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 05/18] src: remove superfluous cipher_ data member The EVP_CIPHER can be reconstructed from the EVP_CIPHER_CTX instance, no need to store it separately. This brought to light the somewhat dubious practice of accessing the EVP_CIPHER after the EVP_CIPHER_CTX instance had been destroyed. It's mostly harmless due to the static nature of built-in EVP_CIPHER instances but it segfaults when the cipher is provided by an ENGINE and the ENGINE is unloaded because its reference count drops to zero. --- src/node_crypto.cc | 33 ++++++++++++++++++--------------- src/node_crypto.h | 2 -- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 31096b6a82d4a3..bd048b5d20149a 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3332,16 +3332,16 @@ void CipherBase::Init(const char* cipher_type, } #endif // NODE_FIPS_MODE - CHECK_EQ(cipher_, nullptr); - cipher_ = EVP_get_cipherbyname(cipher_type); - if (cipher_ == nullptr) { + CHECK_EQ(initialised_, false); + const EVP_CIPHER* const cipher = EVP_get_cipherbyname(cipher_type); + if (cipher == nullptr) { return env()->ThrowError("Unknown cipher"); } unsigned char key[EVP_MAX_KEY_LENGTH]; unsigned char iv[EVP_MAX_IV_LENGTH]; - int key_len = EVP_BytesToKey(cipher_, + int key_len = EVP_BytesToKey(cipher, EVP_md5(), nullptr, reinterpret_cast(key_buf), @@ -3352,7 +3352,7 @@ void CipherBase::Init(const char* cipher_type, EVP_CIPHER_CTX_init(&ctx_); const bool encrypt = (kind_ == kCipher); - EVP_CipherInit_ex(&ctx_, cipher_, nullptr, nullptr, nullptr, encrypt); + EVP_CipherInit_ex(&ctx_, cipher, nullptr, nullptr, nullptr, encrypt); if (!EVP_CIPHER_CTX_set_key_length(&ctx_, key_len)) { EVP_CIPHER_CTX_cleanup(&ctx_); return env()->ThrowError("Invalid key length"); @@ -3394,13 +3394,13 @@ void CipherBase::InitIv(const char* cipher_type, int iv_len) { HandleScope scope(env()->isolate()); - cipher_ = EVP_get_cipherbyname(cipher_type); - if (cipher_ == nullptr) { + const EVP_CIPHER* const cipher = EVP_get_cipherbyname(cipher_type); + if (cipher == nullptr) { return env()->ThrowError("Unknown cipher"); } - const int expected_iv_len = EVP_CIPHER_iv_length(cipher_); - const bool is_gcm_mode = (EVP_CIPH_GCM_MODE == EVP_CIPHER_mode(cipher_)); + const int expected_iv_len = EVP_CIPHER_iv_length(cipher); + const bool is_gcm_mode = (EVP_CIPH_GCM_MODE == EVP_CIPHER_mode(cipher)); if (is_gcm_mode == false && iv_len != expected_iv_len) { return env()->ThrowError("Invalid IV length"); @@ -3408,7 +3408,7 @@ void CipherBase::InitIv(const char* cipher_type, EVP_CIPHER_CTX_init(&ctx_); const bool encrypt = (kind_ == kCipher); - EVP_CipherInit_ex(&ctx_, cipher_, nullptr, nullptr, nullptr, encrypt); + EVP_CipherInit_ex(&ctx_, cipher, nullptr, nullptr, nullptr, encrypt); if (is_gcm_mode && !EVP_CIPHER_CTX_ctrl(&ctx_, EVP_CTRL_GCM_SET_IVLEN, iv_len, nullptr)) { @@ -3454,10 +3454,10 @@ void CipherBase::InitIv(const FunctionCallbackInfo& args) { bool CipherBase::IsAuthenticatedMode() const { - // check if this cipher operates in an AEAD mode that we support. - if (!cipher_) - return false; - int mode = EVP_CIPHER_mode(cipher_); + // Check if this cipher operates in an AEAD mode that we support. + CHECK_EQ(initialised_, true); + const EVP_CIPHER* const cipher = EVP_CIPHER_CTX_cipher(&ctx_); + int mode = EVP_CIPHER_mode(cipher); return mode == EVP_CIPH_GCM_MODE; } @@ -3644,11 +3644,14 @@ void CipherBase::Final(const FunctionCallbackInfo& args) { CipherBase* cipher; ASSIGN_OR_RETURN_UNWRAP(&cipher, args.Holder()); + if (!cipher->initialised_) return env->ThrowError("Unsupported state"); unsigned char* out_value = nullptr; int out_len = -1; Local outString; + // Check IsAuthenticatedMode() first, Final() destroys the EVP_CIPHER_CTX. + const bool is_auth_mode = cipher->IsAuthenticatedMode(); bool r = cipher->Final(&out_value, &out_len); if (out_len <= 0 || !r) { @@ -3656,7 +3659,7 @@ void CipherBase::Final(const FunctionCallbackInfo& args) { out_value = nullptr; out_len = 0; if (!r) { - const char* msg = cipher->IsAuthenticatedMode() ? + const char* msg = is_auth_mode ? "Unsupported state or unable to authenticate data" : "Unsupported state"; diff --git a/src/node_crypto.h b/src/node_crypto.h index db7a97920be156..f46e77d6bcb483 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -467,7 +467,6 @@ class CipherBase : public BaseObject { v8::Local wrap, CipherKind kind) : BaseObject(env, wrap), - cipher_(nullptr), initialised_(false), kind_(kind), auth_tag_len_(0) { @@ -476,7 +475,6 @@ class CipherBase : public BaseObject { private: EVP_CIPHER_CTX ctx_; /* coverity[member_decl] */ - const EVP_CIPHER* cipher_; /* coverity[member_decl] */ bool initialised_; CipherKind kind_; unsigned int auth_tag_len_; From 5ae2cb8b60372a8f9b91c74eccc608ef3978a3ce Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 06/18] src: remove unused Local --- src/node_crypto.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index bd048b5d20149a..fb5e8fd65b7a3b 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3648,7 +3648,6 @@ void CipherBase::Final(const FunctionCallbackInfo& args) { unsigned char* out_value = nullptr; int out_len = -1; - Local outString; // Check IsAuthenticatedMode() first, Final() destroys the EVP_CIPHER_CTX. const bool is_auth_mode = cipher->IsAuthenticatedMode(); From dd6f4a960f4e6fa057507d6fd497beff07b280d3 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 07/18] src: make CipherBase::kind_ const The cipher kind doesn't change over the lifetime of the cipher so make it const. --- src/node_crypto.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_crypto.h b/src/node_crypto.h index f46e77d6bcb483..a2b8cb7a8872eb 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -476,7 +476,7 @@ class CipherBase : public BaseObject { private: EVP_CIPHER_CTX ctx_; /* coverity[member_decl] */ bool initialised_; - CipherKind kind_; + const CipherKind kind_; unsigned int auth_tag_len_; char auth_tag_[EVP_GCM_TLS_TAG_LEN]; }; From d38304249ca39ffac6e69f320cb13c9eadef093f Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 08/18] src: remove extra heap allocation in GetSession() Don't allocate + copy + free; allocate and fill in place, then hand off the pointer to Buffer::New(). Avoids unnecessary heap allocations in the following methods: - crypto.CryptoStream#getSession() - tls.TLSSocket#getSession() --- src/node_crypto.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index fb5e8fd65b7a3b..a4f38595916281 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1804,11 +1804,10 @@ void SSLWrap::GetSession(const FunctionCallbackInfo& args) { int slen = i2d_SSL_SESSION(sess, nullptr); CHECK_GT(slen, 0); - char* sbuf = new char[slen]; + char* sbuf = Malloc(slen); unsigned char* p = reinterpret_cast(sbuf); i2d_SSL_SESSION(sess, &p); - args.GetReturnValue().Set(Encode(env->isolate(), sbuf, slen, BUFFER)); - delete[] sbuf; + args.GetReturnValue().Set(Buffer::New(env, sbuf, slen).ToLocalChecked()); } From 496ef4b0c2377232892f0610e4a65de849e3cdd2 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 09/18] src: avoid heap allocation in hmac.digest() Add a test that ensures the second call to .digest() returns an empty HMAC, like it did before. No comment on whether that is the right behavior or not. --- src/node_crypto.cc | 22 +++++-------------- src/node_crypto.h | 1 - test/parallel/test-crypto-hmac.js | 35 +++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 18 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index a4f38595916281..573688e2511248 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3767,17 +3767,6 @@ void Hmac::HmacUpdate(const FunctionCallbackInfo& args) { } -bool Hmac::HmacDigest(unsigned char** md_value, unsigned int* md_len) { - if (!initialised_) - return false; - *md_value = new unsigned char[EVP_MAX_MD_SIZE]; - HMAC_Final(&ctx_, *md_value, md_len); - HMAC_CTX_cleanup(&ctx_); - initialised_ = false; - return true; -} - - void Hmac::HmacDigest(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -3794,13 +3783,13 @@ void Hmac::HmacDigest(const FunctionCallbackInfo& args) { return env->ThrowError("hmac.digest() does not support UTF-16"); } - unsigned char* md_value = nullptr; + unsigned char md_value[EVP_MAX_MD_SIZE]; unsigned int md_len = 0; - bool r = hmac->HmacDigest(&md_value, &md_len); - if (!r) { - md_value = nullptr; - md_len = 0; + if (hmac->initialised_) { + HMAC_Final(&hmac->ctx_, md_value, &md_len); + HMAC_CTX_cleanup(&hmac->ctx_); + hmac->initialised_ = false; } Local error; @@ -3810,7 +3799,6 @@ void Hmac::HmacDigest(const FunctionCallbackInfo& args) { md_len, encoding, &error); - delete[] md_value; if (rc.IsEmpty()) { CHECK(!error.IsEmpty()); env->isolate()->ThrowException(error); diff --git a/src/node_crypto.h b/src/node_crypto.h index a2b8cb7a8872eb..eb5ca2b770b2c8 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -494,7 +494,6 @@ class Hmac : public BaseObject { protected: void HmacInit(const char* hash_type, const char* key, int key_len); bool HmacUpdate(const char* data, int len); - bool HmacDigest(unsigned char** md_value, unsigned int* md_len); static void New(const v8::FunctionCallbackInfo& args); static void HmacInit(const v8::FunctionCallbackInfo& args); diff --git a/test/parallel/test-crypto-hmac.js b/test/parallel/test-crypto-hmac.js index a10e8a731be023..6ddffbf56f23b2 100644 --- a/test/parallel/test-crypto-hmac.js +++ b/test/parallel/test-crypto-hmac.js @@ -379,3 +379,38 @@ for (let i = 0, l = rfc2202_sha1.length; i < l; i++) { assert.throws(function() { crypto.createHmac('sha256', 'w00t').digest('ucs2'); }, /^Error: hmac\.digest\(\) does not support UTF-16$/); + +// Check initialized -> uninitialized state transition after calling digest(). +{ + const expected = + '\u0010\u0041\u0052\u00c5\u00bf\u00dc\u00a0\u007b\u00c6\u0033' + + '\u00ee\u00bd\u0046\u0019\u009f\u0002\u0055\u00c9\u00f4\u009d'; + { + const h = crypto.createHmac('sha1', 'key').update('data'); + assert.deepStrictEqual(h.digest('buffer'), Buffer.from(expected, 'latin1')); + assert.deepStrictEqual(h.digest('buffer'), Buffer.from('')); + } + { + const h = crypto.createHmac('sha1', 'key').update('data'); + assert.deepStrictEqual(h.digest('latin1'), expected); + assert.deepStrictEqual(h.digest('latin1'), ''); + } +} + +// Check initialized -> uninitialized state transition after calling digest(). +// Calls to update() omitted intentionally. +{ + const expected = + '\u00f4\u002b\u00b0\u00ee\u00b0\u0018\u00eb\u00bd\u0045\u0097' + + '\u00ae\u0072\u0013\u0071\u001e\u00c6\u0007\u0060\u0084\u003f'; + { + const h = crypto.createHmac('sha1', 'key'); + assert.deepStrictEqual(h.digest('buffer'), Buffer.from(expected, 'latin1')); + assert.deepStrictEqual(h.digest('buffer'), Buffer.from('')); + } + { + const h = crypto.createHmac('sha1', 'key'); + assert.deepStrictEqual(h.digest('latin1'), expected); + assert.deepStrictEqual(h.digest('latin1'), ''); + } +} From 40807c1de24dd79b3fbd7ab7d5cb5cc3a0090a5d Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 10/18] src: remove extra heap allocations in DH functions Replace allocate + Encode() + free patterns by calls to Malloc + the Buffer::New() overload that takes ownership of the pointer. Avoids unnecessary heap allocations and copying around of data. DRY the accessor functions for the prime, generator, public key and private key properties; deletes about 40 lines of quadruplicated code. --- src/node_crypto.cc | 106 +++++++++++++-------------------------------- src/node_crypto.h | 2 + 2 files changed, 31 insertions(+), 77 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 573688e2511248..778efd2fcf4008 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4762,99 +4762,49 @@ void DiffieHellman::GenerateKeys(const FunctionCallbackInfo& args) { return ThrowCryptoError(env, ERR_get_error(), "Key generation failed"); } - int dataSize = BN_num_bytes(diffieHellman->dh->pub_key); - char* data = new char[dataSize]; - BN_bn2bin(diffieHellman->dh->pub_key, - reinterpret_cast(data)); - - args.GetReturnValue().Set(Encode(env->isolate(), data, dataSize, BUFFER)); - delete[] data; + size_t size = BN_num_bytes(diffieHellman->dh->pub_key); + char* data = Malloc(size); + BN_bn2bin(diffieHellman->dh->pub_key, reinterpret_cast(data)); + args.GetReturnValue().Set(Buffer::New(env, data, size).ToLocalChecked()); } -void DiffieHellman::GetPrime(const FunctionCallbackInfo& args) { +void DiffieHellman::GetField(const FunctionCallbackInfo& args, + BIGNUM* (DH::*field), const char* err_if_null) { Environment* env = Environment::GetCurrent(args); - DiffieHellman* diffieHellman; - ASSIGN_OR_RETURN_UNWRAP(&diffieHellman, args.Holder()); + DiffieHellman* dh; + ASSIGN_OR_RETURN_UNWRAP(&dh, args.Holder()); + if (!dh->initialised_) return env->ThrowError("Not initialized"); - if (!diffieHellman->initialised_) { - return ThrowCryptoError(env, ERR_get_error(), "Not initialized"); - } + const BIGNUM* num = (dh->dh)->*field; + if (num == nullptr) return env->ThrowError(err_if_null); - int dataSize = BN_num_bytes(diffieHellman->dh->p); - char* data = new char[dataSize]; - BN_bn2bin(diffieHellman->dh->p, reinterpret_cast(data)); + size_t size = BN_num_bytes(num); + char* data = Malloc(size); + BN_bn2bin(num, reinterpret_cast(data)); + args.GetReturnValue().Set(Buffer::New(env, data, size).ToLocalChecked()); +} - args.GetReturnValue().Set(Encode(env->isolate(), data, dataSize, BUFFER)); - delete[] data; +void DiffieHellman::GetPrime(const FunctionCallbackInfo& args) { + GetField(args, &DH::p, "p is null"); } void DiffieHellman::GetGenerator(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - - DiffieHellman* diffieHellman; - ASSIGN_OR_RETURN_UNWRAP(&diffieHellman, args.Holder()); - - if (!diffieHellman->initialised_) { - return ThrowCryptoError(env, ERR_get_error(), "Not initialized"); - } - - int dataSize = BN_num_bytes(diffieHellman->dh->g); - char* data = new char[dataSize]; - BN_bn2bin(diffieHellman->dh->g, reinterpret_cast(data)); - - args.GetReturnValue().Set(Encode(env->isolate(), data, dataSize, BUFFER)); - delete[] data; + GetField(args, &DH::g, "g is null"); } void DiffieHellman::GetPublicKey(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - - DiffieHellman* diffieHellman; - ASSIGN_OR_RETURN_UNWRAP(&diffieHellman, args.Holder()); - - if (!diffieHellman->initialised_) { - return ThrowCryptoError(env, ERR_get_error(), "Not initialized"); - } - - if (diffieHellman->dh->pub_key == nullptr) { - return env->ThrowError("No public key - did you forget to generate one?"); - } - - int dataSize = BN_num_bytes(diffieHellman->dh->pub_key); - char* data = new char[dataSize]; - BN_bn2bin(diffieHellman->dh->pub_key, - reinterpret_cast(data)); - - args.GetReturnValue().Set(Encode(env->isolate(), data, dataSize, BUFFER)); - delete[] data; + GetField(args, &DH::pub_key, + "No public key - did you forget to generate one?"); } void DiffieHellman::GetPrivateKey(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - - DiffieHellman* diffieHellman; - ASSIGN_OR_RETURN_UNWRAP(&diffieHellman, args.Holder()); - - if (!diffieHellman->initialised_) { - return ThrowCryptoError(env, ERR_get_error(), "Not initialized"); - } - - if (diffieHellman->dh->priv_key == nullptr) { - return env->ThrowError("No private key - did you forget to generate one?"); - } - - int dataSize = BN_num_bytes(diffieHellman->dh->priv_key); - char* data = new char[dataSize]; - BN_bn2bin(diffieHellman->dh->priv_key, - reinterpret_cast(data)); - - args.GetReturnValue().Set(Encode(env->isolate(), data, dataSize, BUFFER)); - delete[] data; + GetField(args, &DH::priv_key, + "No private key - did you forget to generate one?"); } @@ -4882,7 +4832,7 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo& args) { } int dataSize = DH_size(diffieHellman->dh); - char* data = new char[dataSize]; + char* data = Malloc(dataSize); int size = DH_compute_key(reinterpret_cast(data), key, @@ -4894,7 +4844,7 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo& args) { checked = DH_check_pub_key(diffieHellman->dh, key, &checkResult); BN_free(key); - delete[] data; + free(data); if (!checked) { return ThrowCryptoError(env, ERR_get_error(), "Invalid Key"); @@ -4909,6 +4859,8 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo& args) { } else { return env->ThrowError("Invalid key"); } + + UNREACHABLE(); } BN_free(key); @@ -4924,8 +4876,8 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo& args) { memset(data, 0, dataSize - size); } - args.GetReturnValue().Set(Encode(env->isolate(), data, dataSize, BUFFER)); - delete[] data; + auto rc = Buffer::New(env->isolate(), data, dataSize).ToLocalChecked(); + args.GetReturnValue().Set(rc); } diff --git a/src/node_crypto.h b/src/node_crypto.h index eb5ca2b770b2c8..01c799a8b12d5c 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -690,6 +690,8 @@ class DiffieHellman : public BaseObject { } private: + static void GetField(const v8::FunctionCallbackInfo& args, + BIGNUM* (DH::*field), const char* err_if_null); bool VerifyContext(); bool initialised_; From 5fd09df0c28157b45de76e947c6eb80c4bcd4190 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 11/18] src: refactor PBKDF2Request --- src/node_crypto.cc | 77 ++++++++++++++++++++++++++-------------------- 1 file changed, 43 insertions(+), 34 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 778efd2fcf4008..d7b123b73a694f 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5310,9 +5310,15 @@ class PBKDF2Request : public AsyncWrap { size_t self_size() const override { return sizeof(*this); } - uv_work_t work_req_; + static void Work(uv_work_t* work_req); + void Work(); + + static void After(uv_work_t* work_req, int status); + void After(Local argv[2]); + void After(); private: + uv_work_t work_req_; const EVP_MD* digest_; int error_; int passlen_; @@ -5325,48 +5331,52 @@ class PBKDF2Request : public AsyncWrap { }; -void EIO_PBKDF2(PBKDF2Request* req) { - req->set_error(PKCS5_PBKDF2_HMAC( - req->pass(), - req->passlen(), - reinterpret_cast(req->salt()), - req->saltlen(), - req->iter(), - req->digest(), - req->keylen(), - reinterpret_cast(req->key()))); - OPENSSL_cleanse(req->pass(), req->passlen()); - OPENSSL_cleanse(req->salt(), req->saltlen()); +void PBKDF2Request::Work() { + set_error(PKCS5_PBKDF2_HMAC( + pass(), + passlen(), + reinterpret_cast(salt()), + saltlen(), + iter(), + digest(), + keylen(), + reinterpret_cast(key()))); + OPENSSL_cleanse(pass(), passlen()); + OPENSSL_cleanse(salt(), saltlen()); } -void EIO_PBKDF2(uv_work_t* work_req) { +void PBKDF2Request::Work(uv_work_t* work_req) { PBKDF2Request* req = ContainerOf(&PBKDF2Request::work_req_, work_req); - EIO_PBKDF2(req); + req->Work(); } -void EIO_PBKDF2After(PBKDF2Request* req, Local argv[2]) { - if (req->error()) { - argv[0] = Undefined(req->env()->isolate()); - argv[1] = Encode(req->env()->isolate(), req->key(), req->keylen(), BUFFER); - OPENSSL_cleanse(req->key(), req->keylen()); +void PBKDF2Request::After(Local argv[2]) { + if (error()) { + argv[0] = Undefined(env()->isolate()); + argv[1] = Encode(env()->isolate(), key(), keylen(), BUFFER); + OPENSSL_cleanse(key(), keylen()); } else { - argv[0] = Exception::Error(req->env()->pbkdf2_error_string()); - argv[1] = Undefined(req->env()->isolate()); + argv[0] = Exception::Error(env()->pbkdf2_error_string()); + argv[1] = Undefined(env()->isolate()); } } -void EIO_PBKDF2After(uv_work_t* work_req, int status) { +void PBKDF2Request::After() { + HandleScope handle_scope(env()->isolate()); + Context::Scope context_scope(env()->context()); + Local argv[2]; + After(argv); + MakeCallback(env()->ondone_string(), arraysize(argv), argv); +} + + +void PBKDF2Request::After(uv_work_t* work_req, int status) { CHECK_EQ(status, 0); PBKDF2Request* req = ContainerOf(&PBKDF2Request::work_req_, work_req); - Environment* env = req->env(); - HandleScope handle_scope(env->isolate()); - Context::Scope context_scope(env->context()); - Local argv[2]; - EIO_PBKDF2After(req, argv); - req->MakeCallback(env->ondone_string(), arraysize(argv), argv); + req->After(); delete req; } @@ -5469,14 +5479,13 @@ void PBKDF2(const FunctionCallbackInfo& args) { obj->Set(env->domain_string(), env->domain_array()->Get(0)); uv_queue_work(env->event_loop(), req->work_req(), - EIO_PBKDF2, - EIO_PBKDF2After); + PBKDF2Request::Work, + PBKDF2Request::After); } else { env->PrintSyncTrace(); + req->Work(); Local argv[2]; - EIO_PBKDF2(req); - EIO_PBKDF2After(req, argv); - + req->After(argv); delete req; if (argv[0]->IsObject()) From 160b60f159f6032f32509322e0714d856143135f Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 12/18] src: make array arg length compile-time checkable --- src/node_crypto.cc | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d7b123b73a694f..c3f6e70f26f75b 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5314,7 +5314,7 @@ class PBKDF2Request : public AsyncWrap { void Work(); static void After(uv_work_t* work_req, int status); - void After(Local argv[2]); + void After(Local (*argv)[2]); void After(); private: @@ -5352,14 +5352,14 @@ void PBKDF2Request::Work(uv_work_t* work_req) { } -void PBKDF2Request::After(Local argv[2]) { +void PBKDF2Request::After(Local (*argv)[2]) { if (error()) { - argv[0] = Undefined(env()->isolate()); - argv[1] = Encode(env()->isolate(), key(), keylen(), BUFFER); + (*argv)[0] = Undefined(env()->isolate()); + (*argv)[1] = Encode(env()->isolate(), key(), keylen(), BUFFER); OPENSSL_cleanse(key(), keylen()); } else { - argv[0] = Exception::Error(env()->pbkdf2_error_string()); - argv[1] = Undefined(env()->isolate()); + (*argv)[0] = Exception::Error(env()->pbkdf2_error_string()); + (*argv)[1] = Undefined(env()->isolate()); } } @@ -5368,7 +5368,7 @@ void PBKDF2Request::After() { HandleScope handle_scope(env()->isolate()); Context::Scope context_scope(env()->context()); Local argv[2]; - After(argv); + After(&argv); MakeCallback(env()->ondone_string(), arraysize(argv), argv); } @@ -5485,7 +5485,7 @@ void PBKDF2(const FunctionCallbackInfo& args) { env->PrintSyncTrace(); req->Work(); Local argv[2]; - req->After(argv); + req->After(&argv); delete req; if (argv[0]->IsObject()) @@ -5595,21 +5595,21 @@ void RandomBytesWork(uv_work_t* work_req) { // don't call this function without a valid HandleScope -void RandomBytesCheck(RandomBytesRequest* req, Local argv[2]) { +void RandomBytesCheck(RandomBytesRequest* req, Local (*argv)[2]) { if (req->error()) { char errmsg[256] = "Operation not supported"; if (req->error() != static_cast(-1)) // NOLINT(runtime/int) ERR_error_string_n(req->error(), errmsg, sizeof errmsg); - argv[0] = Exception::Error(OneByteString(req->env()->isolate(), errmsg)); - argv[1] = Null(req->env()->isolate()); + (*argv)[0] = Exception::Error(OneByteString(req->env()->isolate(), errmsg)); + (*argv)[1] = Null(req->env()->isolate()); req->release(); } else { char* data = nullptr; size_t size; req->return_memory(&data, &size); - argv[0] = Null(req->env()->isolate()); + (*argv)[0] = Null(req->env()->isolate()); Local buffer = req->object()->Get(req->env()->context(), req->env()->buffer_string()).ToLocalChecked(); @@ -5618,9 +5618,9 @@ void RandomBytesCheck(RandomBytesRequest* req, Local argv[2]) { CHECK_LE(req->size(), Buffer::Length(buffer)); char* buf = Buffer::Data(buffer); memcpy(buf, data, req->size()); - argv[1] = buffer; + (*argv)[1] = buffer; } else { - argv[1] = Buffer::New(req->env(), data, size).ToLocalChecked(); + (*argv)[1] = Buffer::New(req->env(), data, size).ToLocalChecked(); } } } @@ -5634,7 +5634,7 @@ void RandomBytesAfter(uv_work_t* work_req, int status) { HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); Local argv[2]; - RandomBytesCheck(req, argv); + RandomBytesCheck(req, &argv); req->MakeCallback(env->ondone_string(), arraysize(argv), argv); delete req; } @@ -5642,14 +5642,14 @@ void RandomBytesAfter(uv_work_t* work_req, int status) { void RandomBytesProcessSync(Environment* env, RandomBytesRequest* req, - Local argv[2]) { + Local (*argv)[2]) { env->PrintSyncTrace(); RandomBytesWork(req->work_req()); RandomBytesCheck(req, argv); delete req; - if (!argv[0]->IsNull()) - env->isolate()->ThrowException(argv[0]); + if (!(*argv)[0]->IsNull()) + env->isolate()->ThrowException((*argv)[0]); } @@ -5686,7 +5686,7 @@ void RandomBytes(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(obj); } else { Local argv[2]; - RandomBytesProcessSync(env, req, argv); + RandomBytesProcessSync(env, req, &argv); if (argv[0]->IsNull()) args.GetReturnValue().Set(argv[1]); } @@ -5733,7 +5733,7 @@ void RandomBytesBuffer(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(obj); } else { Local argv[2]; - RandomBytesProcessSync(env, req, argv); + RandomBytesProcessSync(env, req, &argv); if (argv[0]->IsNull()) args.GetReturnValue().Set(argv[1]); } From 3818792bd1e350c40cd63077ed5e31cd5d5be9b7 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 13/18] src: avoid heap allocation in crypto.pbkdf2() --- src/node_crypto.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index c3f6e70f26f75b..082b400ef4e35c 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5355,8 +5355,9 @@ void PBKDF2Request::Work(uv_work_t* work_req) { void PBKDF2Request::After(Local (*argv)[2]) { if (error()) { (*argv)[0] = Undefined(env()->isolate()); - (*argv)[1] = Encode(env()->isolate(), key(), keylen(), BUFFER); - OPENSSL_cleanse(key(), keylen()); + (*argv)[1] = Buffer::New(env(), key(), keylen()).ToLocalChecked(); + key_ = nullptr; + keylen_ = 0; } else { (*argv)[0] = Exception::Error(env()->pbkdf2_error_string()); (*argv)[1] = Undefined(env()->isolate()); From 0ce90a94110d6d208ee2851ed36645ffdfb73012 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 14/18] src: remove PBKDF2Request::release() --- src/node_crypto.cc | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 082b400ef4e35c..25e2ed64fa5475 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5245,7 +5245,18 @@ class PBKDF2Request : public AsyncWrap { } ~PBKDF2Request() override { - release(); + free(pass_); + pass_ = nullptr; + passlen_ = 0; + + free(salt_); + salt_ = nullptr; + saltlen_ = 0; + + free(key_); + key_ = nullptr; + keylen_ = 0; + ClearWrap(object()); persistent().Reset(); } @@ -5286,20 +5297,6 @@ class PBKDF2Request : public AsyncWrap { return iter_; } - inline void release() { - free(pass_); - pass_ = nullptr; - passlen_ = 0; - - free(salt_); - salt_ = nullptr; - saltlen_ = 0; - - free(key_); - key_ = nullptr; - keylen_ = 0; - } - inline int error() const { return error_; } From 1cfd02e146eb8cd7474261653b79feb462b8d106 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 15/18] src: simplify PBKDF2Request This also renames a misnamed variable `error_` to `success_`. --- src/node_crypto.cc | 65 +++++++--------------------------------------- 1 file changed, 10 insertions(+), 55 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 25e2ed64fa5475..248a701cb31804 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5233,7 +5233,7 @@ class PBKDF2Request : public AsyncWrap { int keylen) : AsyncWrap(env, object, AsyncWrap::PROVIDER_PBKDF2REQUEST), digest_(digest), - error_(0), + success_(false), passlen_(passlen), pass_(pass), saltlen_(saltlen), @@ -5265,46 +5265,6 @@ class PBKDF2Request : public AsyncWrap { return &work_req_; } - inline const EVP_MD* digest() const { - return digest_; - } - - inline int passlen() const { - return passlen_; - } - - inline char* pass() const { - return pass_; - } - - inline int saltlen() const { - return saltlen_; - } - - inline char* salt() const { - return salt_; - } - - inline int keylen() const { - return keylen_; - } - - inline char* key() const { - return key_; - } - - inline int iter() const { - return iter_; - } - - inline int error() const { - return error_; - } - - inline void set_error(int err) { - error_ = err; - } - size_t self_size() const override { return sizeof(*this); } static void Work(uv_work_t* work_req); @@ -5317,7 +5277,7 @@ class PBKDF2Request : public AsyncWrap { private: uv_work_t work_req_; const EVP_MD* digest_; - int error_; + bool success_; int passlen_; char* pass_; int saltlen_; @@ -5329,17 +5289,12 @@ class PBKDF2Request : public AsyncWrap { void PBKDF2Request::Work() { - set_error(PKCS5_PBKDF2_HMAC( - pass(), - passlen(), - reinterpret_cast(salt()), - saltlen(), - iter(), - digest(), - keylen(), - reinterpret_cast(key()))); - OPENSSL_cleanse(pass(), passlen()); - OPENSSL_cleanse(salt(), saltlen()); + success_ = + PKCS5_PBKDF2_HMAC( + pass_, passlen_, reinterpret_cast(salt_), saltlen_, + iter_, digest_, keylen_, reinterpret_cast(key_)); + OPENSSL_cleanse(pass_, passlen_); + OPENSSL_cleanse(salt_, saltlen_); } @@ -5350,9 +5305,9 @@ void PBKDF2Request::Work(uv_work_t* work_req) { void PBKDF2Request::After(Local (*argv)[2]) { - if (error()) { + if (success_) { (*argv)[0] = Undefined(env()->isolate()); - (*argv)[1] = Buffer::New(env(), key(), keylen()).ToLocalChecked(); + (*argv)[1] = Buffer::New(env(), key_, keylen_).ToLocalChecked(); key_ = nullptr; keylen_ = 0; } else { From b8e3a3ecd7f18f2b3f1bf5b1db7a99b7b970e2ac Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 16/18] src: guard against double free in randomBytes() --- src/node_crypto.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 248a701cb31804..b9fd2446e28376 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5498,6 +5498,7 @@ class RandomBytesRequest : public AsyncWrap { size_ = 0; if (free_mode_ == FREE_DATA) { free(data_); + data_ = nullptr; } } From 083ccd201bfd7fa7eaf954046b10152d7d71f146 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 17/18] src: reduce allocations in exportPublicKey() --- src/node_crypto.cc | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index b9fd2446e28376..02cf4ef5187995 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5835,7 +5835,7 @@ void VerifySpkac(const FunctionCallbackInfo& args) { } -const char* ExportPublicKey(const char* data, int len) { +char* ExportPublicKey(const char* data, int len, size_t* size) { char* buf = nullptr; EVP_PKEY* pkey = nullptr; NETSCAPE_SPKI* spki = nullptr; @@ -5855,12 +5855,12 @@ const char* ExportPublicKey(const char* data, int len) { if (PEM_write_bio_PUBKEY(bio, pkey) <= 0) goto exit; - BIO_write(bio, "\0", 1); BUF_MEM* ptr; BIO_get_mem_ptr(bio, &ptr); - buf = new char[ptr->length]; - memcpy(buf, ptr->data, ptr->length); + *size = ptr->length; + buf = Malloc(*size); + memcpy(buf, ptr->data, *size); exit: if (pkey != nullptr) @@ -5891,14 +5891,12 @@ void ExportPublicKey(const FunctionCallbackInfo& args) { char* data = Buffer::Data(args[0]); CHECK_NE(data, nullptr); - const char* pkey = ExportPublicKey(data, length); + size_t pkey_size; + char* pkey = ExportPublicKey(data, length, &pkey_size); if (pkey == nullptr) return args.GetReturnValue().SetEmptyString(); - Local out = Encode(env->isolate(), pkey, strlen(pkey), BUFFER); - - delete[] pkey; - + Local out = Buffer::New(env, pkey, pkey_size).ToLocalChecked(); args.GetReturnValue().Set(out); } From e4ac1b6036179c528ebcad8c0c8d83690da29af4 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 10 Jul 2017 12:56:37 +0200 Subject: [PATCH 18/18] src: fix memory leak in DH key setters Fix a memory leak in dh.setPublicKey() and dh.setPrivateKey() where the old keys weren't freed. Fixes: https://github.com/nodejs/node/issues/8377 --- src/node_crypto.cc | 54 +++++++++++++--------------- src/node_crypto.h | 2 ++ test/parallel/test-crypto-dh-leak.js | 26 ++++++++++++++ 3 files changed, 53 insertions(+), 29 deletions(-) create mode 100644 test/parallel/test-crypto-dh-leak.js diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 02cf4ef5187995..84f006b00f67eb 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4881,44 +4881,40 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo& args) { } -void DiffieHellman::SetPublicKey(const FunctionCallbackInfo& args) { - DiffieHellman* diffieHellman; - ASSIGN_OR_RETURN_UNWRAP(&diffieHellman, args.Holder()); - Environment* env = diffieHellman->env(); +void DiffieHellman::SetKey(const v8::FunctionCallbackInfo& args, + BIGNUM* (DH::*field), const char* what) { + Environment* env = Environment::GetCurrent(args); - if (!diffieHellman->initialised_) { - return ThrowCryptoError(env, ERR_get_error(), "Not initialized"); - } + DiffieHellman* dh; + ASSIGN_OR_RETURN_UNWRAP(&dh, args.Holder()); + if (!dh->initialised_) return env->ThrowError("Not initialized"); + + BIGNUM** num = &((dh->dh)->*field); + char errmsg[64]; if (args.Length() == 0) { - return env->ThrowError("Public key argument is mandatory"); - } else { - THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Public key"); - diffieHellman->dh->pub_key = BN_bin2bn( - reinterpret_cast(Buffer::Data(args[0])), - Buffer::Length(args[0]), 0); + snprintf(errmsg, sizeof(errmsg), "%s argument is mandatory", what); + return env->ThrowError(errmsg); + } + + if (!Buffer::HasInstance(args[0])) { + snprintf(errmsg, sizeof(errmsg), "%s must be a buffer", what); + return env->ThrowTypeError(errmsg); } + + *num = BN_bin2bn(reinterpret_cast(Buffer::Data(args[0])), + Buffer::Length(args[0]), *num); + CHECK_NE(*num, nullptr); } -void DiffieHellman::SetPrivateKey(const FunctionCallbackInfo& args) { - DiffieHellman* diffieHellman; - ASSIGN_OR_RETURN_UNWRAP(&diffieHellman, args.Holder()); - Environment* env = diffieHellman->env(); +void DiffieHellman::SetPublicKey(const FunctionCallbackInfo& args) { + SetKey(args, &DH::pub_key, "Public key"); +} - if (!diffieHellman->initialised_) { - return ThrowCryptoError(env, ERR_get_error(), "Not initialized"); - } - if (args.Length() == 0) { - return env->ThrowError("Private key argument is mandatory"); - } else { - THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Private key"); - diffieHellman->dh->priv_key = BN_bin2bn( - reinterpret_cast(Buffer::Data(args[0])), - Buffer::Length(args[0]), - 0); - } +void DiffieHellman::SetPrivateKey(const FunctionCallbackInfo& args) { + SetKey(args, &DH::priv_key, "Private key"); } diff --git a/src/node_crypto.h b/src/node_crypto.h index 01c799a8b12d5c..1d823bcb359a6a 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -692,6 +692,8 @@ class DiffieHellman : public BaseObject { private: static void GetField(const v8::FunctionCallbackInfo& args, BIGNUM* (DH::*field), const char* err_if_null); + static void SetKey(const v8::FunctionCallbackInfo& args, + BIGNUM* (DH::*field), const char* what); bool VerifyContext(); bool initialised_; diff --git a/test/parallel/test-crypto-dh-leak.js b/test/parallel/test-crypto-dh-leak.js new file mode 100644 index 00000000000000..2ca26d3e9bdb90 --- /dev/null +++ b/test/parallel/test-crypto-dh-leak.js @@ -0,0 +1,26 @@ +// Flags: --expose-gc +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const crypto = require('crypto'); + +const before = process.memoryUsage().rss; +{ + const dh = crypto.createDiffieHellman(common.hasFipsCrypto ? 1024 : 256); + const publicKey = dh.generateKeys(); + const privateKey = dh.getPrivateKey(); + for (let i = 0; i < 5e4; i += 1) { + dh.setPublicKey(publicKey); + dh.setPrivateKey(privateKey); + } +} +global.gc(); +const after = process.memoryUsage().rss; + +// RSS should stay the same, ceteris paribus, but allow for +// some slop because V8 mallocs memory during execution. +assert(after - before < 5 << 20);