diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 1237157da3f775..e1442b19218d09 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); @@ -5157,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 094bb9cc1f8822..baa90204f2f97a 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; @@ -251,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 b4aa85f8fa36fa..0c61f19d0189d2 100644 --- a/src/node_crypto_bio.h +++ b/src/node_crypto_bio.h @@ -24,6 +24,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include "node_crypto.h" #include "openssl/bio.h" #include "env-inl.h" #include "util-inl.h" @@ -32,25 +33,21 @@ 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,14 @@ 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; + + friend void node::crypto::InitCryptoOnce(); }; } // namespace crypto diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 3efa6adb4edb0e..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; } @@ -112,11 +108,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_);