From 5f27653602043450651f23752e470ff579c32eab Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 26 Jul 2018 12:53:23 +0200 Subject: [PATCH 1/4] src: use smart pointers for NodeBIO --- src/node_crypto.cc | 11 +++++++---- src/node_crypto_bio.cc | 26 +++++++++++--------------- src/node_crypto_bio.h | 33 +++++++++++++++------------------ src/tls_wrap.cc | 8 +++----- 4 files changed, 36 insertions(+), 42 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 1237157da3f775..afe607b4d08a4c 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -483,7 +483,7 @@ void SecureContext::Init(const FunctionCallbackInfo& args) { // Takes a string or buffer and loads it into a BIO. // Caller responsible for BIO_free_all-ing the returned object. -static BIO* LoadBIO(Environment* env, Local v) { +static BIOPointer LoadBIO(Environment* env, Local v) { HandleScope scope(env->isolate()); if (v->IsString()) { @@ -738,9 +738,12 @@ static X509_STORE* NewRootCertStore() { if (root_certs_vector.empty()) { for (size_t i = 0; i < arraysize(root_certs); i++) { - BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i])); - X509* x509 = PEM_read_bio_X509(bp, nullptr, NoPasswordCallback, nullptr); - BIO_free(bp); + X509* x509 = + PEM_read_bio_X509(NodeBIO::NewFixed(root_certs[i], + strlen(root_certs[i])).get(), + nullptr, // no re-use of X509 structure + NoPasswordCallback, + nullptr); // no callback data // Parse errors from the built-in roots are fatal. CHECK_NOT_NULL(x509); diff --git a/src/node_crypto_bio.cc b/src/node_crypto_bio.cc index 094bb9cc1f8822..ab68c0a08111f2 100644 --- a/src/node_crypto_bio.cc +++ b/src/node_crypto_bio.cc @@ -38,36 +38,32 @@ namespace crypto { #endif -BIO* NodeBIO::New() { +BIOPointer NodeBIO::New(Environment* env) { // The const_cast doesn't violate const correctness. OpenSSL's usage of // BIO_METHOD is effectively const but BIO_new() takes a non-const argument. - return BIO_new(const_cast(GetMethod())); + BIOPointer bio(BIO_new(const_cast(GetMethod()))); + if (bio && env != nullptr) + NodeBIO::FromBIO(bio.get())->env_ = env; + return bio; } -BIO* NodeBIO::NewFixed(const char* data, size_t len) { - BIO* bio = New(); +BIOPointer NodeBIO::NewFixed(const char* data, size_t len, Environment* env) { + BIOPointer bio = New(env); - if (bio == nullptr || + if (!bio || len > INT_MAX || - BIO_write(bio, data, len) != static_cast(len) || - BIO_set_mem_eof_return(bio, 0) != 1) { - BIO_free(bio); - return nullptr; + BIO_write(bio.get(), data, len) != static_cast(len) || + BIO_set_mem_eof_return(bio.get(), 0) != 1) { + return BIOPointer(); } return bio; } -void NodeBIO::AssignEnvironment(Environment* env) { - env_ = env; -} - - int NodeBIO::New(BIO* bio) { BIO_set_data(bio, new NodeBIO()); - BIO_set_init(bio, 1); return 1; diff --git a/src/node_crypto_bio.h b/src/node_crypto_bio.h index b4aa85f8fa36fa..e2303fe7397b5b 100644 --- a/src/node_crypto_bio.h +++ b/src/node_crypto_bio.h @@ -28,29 +28,26 @@ #include "env-inl.h" #include "util-inl.h" #include "v8.h" +#include "node_crypto.h" namespace node { namespace crypto { +// This class represents buffers for OpenSSL I/O, implemented as a singly-linked +// list of chunks. It can be used both for writing data from Node to OpenSSL +// and back, but only one direction per instance. +// The structure is only accessed, and owned by, the OpenSSL BIOPointer +// (a.k.a. std::unique_ptr). class NodeBIO : public MemoryRetainer { public: - NodeBIO() : env_(nullptr), - initial_(kInitialBufferLength), - length_(0), - eof_return_(-1), - read_head_(nullptr), - write_head_(nullptr) { - } - ~NodeBIO(); - static BIO* New(); + static BIOPointer New(Environment* env = nullptr); // NewFixed takes a copy of `len` bytes from `data` and returns a BIO that, // when read from, returns those bytes followed by EOF. - static BIO* NewFixed(const char* data, size_t len); - - void AssignEnvironment(Environment* env); + static BIOPointer NewFixed(const char* data, size_t len, + Environment* env = nullptr); // Move read head to next buffer if needed void TryMoveReadHead(); @@ -161,12 +158,12 @@ class NodeBIO : public MemoryRetainer { char* data_; }; - Environment* env_; - size_t initial_; - size_t length_; - int eof_return_; - Buffer* read_head_; - Buffer* write_head_; + Environment* env_ = nullptr; + size_t initial_ = kInitialBufferLength; + size_t length_ = 0; + int eof_return_ = -1; + Buffer* read_head_ = nullptr; + Buffer* write_head_ = nullptr; }; } // namespace crypto diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 3efa6adb4edb0e..3e6c66d5e36d07 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -112,11 +112,9 @@ void TLSWrap::NewSessionDoneCb() { void TLSWrap::InitSSL() { - // Initialize SSL - enc_in_ = crypto::NodeBIO::New(); - enc_out_ = crypto::NodeBIO::New(); - crypto::NodeBIO::FromBIO(enc_in_)->AssignEnvironment(env()); - crypto::NodeBIO::FromBIO(enc_out_)->AssignEnvironment(env()); + // Initialize SSL – OpenSSL takes ownership of these. + enc_in_ = crypto::NodeBIO::New(env()).release(); + enc_out_ = crypto::NodeBIO::New(env()).release(); SSL_set_bio(ssl_.get(), enc_in_, enc_out_); From 61e3eb79e4fd3d52c6ff82f9395ea18500d11aef Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 26 Jul 2018 12:54:46 +0200 Subject: [PATCH 2/4] src: avoid possible race during NodeBIO initialization --- src/node_crypto.cc | 2 ++ src/node_crypto.h | 2 ++ src/node_crypto_bio.cc | 2 ++ src/node_crypto_bio.h | 2 ++ 4 files changed, 8 insertions(+) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index afe607b4d08a4c..e1442b19218d09 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5160,6 +5160,8 @@ void InitCryptoOnce() { ERR_load_ENGINE_strings(); ENGINE_load_builtin_engines(); #endif // !OPENSSL_NO_ENGINE + + NodeBIO::GetMethod(); } diff --git a/src/node_crypto.h b/src/node_crypto.h index ee069c9cf799b2..269bccbc03a1d7 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -97,6 +97,8 @@ extern int VerifyCallback(int preverify_ok, X509_STORE_CTX* ctx); extern void UseExtraCaCerts(const std::string& file); +void InitCryptoOnce(); + class SecureContext : public BaseObject { public: ~SecureContext() override { diff --git a/src/node_crypto_bio.cc b/src/node_crypto_bio.cc index ab68c0a08111f2..baa90204f2f97a 100644 --- a/src/node_crypto_bio.cc +++ b/src/node_crypto_bio.cc @@ -247,6 +247,8 @@ const BIO_METHOD* NodeBIO::GetMethod() { return &method; #else + // This is called from InitCryptoOnce() to avoid race conditions during + // initialization. static BIO_METHOD* method = nullptr; if (method == nullptr) { diff --git a/src/node_crypto_bio.h b/src/node_crypto_bio.h index e2303fe7397b5b..6a14340095198d 100644 --- a/src/node_crypto_bio.h +++ b/src/node_crypto_bio.h @@ -164,6 +164,8 @@ class NodeBIO : public MemoryRetainer { int eof_return_ = -1; Buffer* read_head_ = nullptr; Buffer* write_head_ = nullptr; + + friend void node::crypto::InitCryptoOnce(); }; } // namespace crypto From c134e75ed706df250ed6775aba216b1f19badd25 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 26 Jul 2018 12:55:11 +0200 Subject: [PATCH 3/4] src: remove unnecessary writes in tls_wrap.cc --- src/tls_wrap.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 3e6c66d5e36d07..eb40d856fdab4b 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -60,8 +60,6 @@ TLSWrap::TLSWrap(Environment* env, SSLWrap(env, sc, kind), StreamBase(env), sc_(sc), - enc_in_(nullptr), - enc_out_(nullptr), write_size_(0), started_(false), established_(false), @@ -86,8 +84,6 @@ TLSWrap::TLSWrap(Environment* env, TLSWrap::~TLSWrap() { - enc_in_ = nullptr; - enc_out_ = nullptr; sc_ = nullptr; } From fb1dac40720bc1b677521df4350c44fc0942bac6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 2 Aug 2018 22:34:48 +0200 Subject: [PATCH 4/4] fixup! src: use smart pointers for NodeBIO --- src/node_crypto_bio.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_crypto_bio.h b/src/node_crypto_bio.h index 6a14340095198d..0c61f19d0189d2 100644 --- a/src/node_crypto_bio.h +++ b/src/node_crypto_bio.h @@ -24,11 +24,11 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include "node_crypto.h" #include "openssl/bio.h" #include "env-inl.h" #include "util-inl.h" #include "v8.h" -#include "node_crypto.h" namespace node { namespace crypto {