From b3eb281436c2c463c168fd2184cd1cd81726bd3a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 19 Nov 2019 20:06:50 +0100 Subject: [PATCH 1/3] tls: add memory tracking support to SSLWrap --- src/node_crypto.cc | 7 +++++++ src/node_crypto.h | 2 ++ src/tls_wrap.cc | 1 + 3 files changed, 10 insertions(+) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 20cc52fbff7910..b4766eabb57ac3 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -144,6 +144,7 @@ template void SSLWrap::AddMethods(Environment* env, template void SSLWrap::ConfigureSecureContext(SecureContext* sc); template void SSLWrap::SetSNIContext(SecureContext* sc); template int SSLWrap::SetCACerts(SecureContext* sc); +template void SSLWrap::MemoryInfo(MemoryTracker* tracker) const; template SSL_SESSION* SSLWrap::GetSessionCallback( SSL* s, const unsigned char* key, @@ -3074,6 +3075,12 @@ int SSLWrap::SetCACerts(SecureContext* sc) { return 1; } +template +void SSLWrap::MemoryInfo(MemoryTracker* tracker) const { + tracker->TrackField("ocsp_response", ocsp_response_); + tracker->TrackField("sni_context", sni_context_); +} + int VerifyCallback(int preverify_ok, X509_STORE_CTX* ctx) { // From https://www.openssl.org/docs/man1.1.1/man3/SSL_verify_cb: // diff --git a/src/node_crypto.h b/src/node_crypto.h index 777ba5d302a536..327e8ec8fecc4a 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -222,6 +222,8 @@ class SSLWrap { inline bool is_awaiting_new_session() const { return awaiting_new_session_; } inline bool is_waiting_cert_cb() const { return cert_cb_ != nullptr; } + void MemoryInfo(MemoryTracker* tracker) const; + protected: typedef void (*CertCb)(void* arg); diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 4ec6dda6df70d7..626662c9a5ef8e 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -1089,6 +1089,7 @@ void TLSWrap::GetWriteQueueSize(const FunctionCallbackInfo& info) { void TLSWrap::MemoryInfo(MemoryTracker* tracker) const { + SSLWrap::MemoryInfo(tracker); tracker->TrackField("error", error_); tracker->TrackFieldWithSize("pending_cleartext_input", pending_cleartext_input_.size(), From fe5784a9f0e9535e45b739eb1cbd35d5af105c97 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 19 Nov 2019 20:07:00 +0100 Subject: [PATCH 2/3] src: use BaseObjectPtr to store SNI context Rather than relying on a link to the JS object, store a pointer to the C++ object directly. --- src/node_crypto.cc | 12 +++++++++--- src/node_crypto.h | 2 +- src/tls_wrap.cc | 3 +-- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index b4766eabb57ac3..1121b148552487 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2991,9 +2991,15 @@ void SSLWrap::CertCbDone(const FunctionCallbackInfo& args) { goto fire_cb; if (cons->HasInstance(ctx)) { - SecureContext* sc; - ASSIGN_OR_RETURN_UNWRAP(&sc, ctx.As()); - w->sni_context_.Reset(env->isolate(), ctx); + SecureContext* sc = Unwrap(ctx.As()); + CHECK_NOT_NULL(sc); + // XXX: There is a method w->SetSNIContext(sc), and you might think that + // it makes sense to call that here and make setting w->sni_context_ part + // of it. In fact, that passes the test suite, although SetSNIContext() + // performs a lot more operations. + // If anybody is familiar enough with the TLS code to know whether it makes + // sense, please do so or document why it doesn't. + w->sni_context_ = BaseObjectPtr(sc); int rv; diff --git a/src/node_crypto.h b/src/node_crypto.h index 327e8ec8fecc4a..b9ad5d801c9deb 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -310,7 +310,7 @@ class SSLWrap { ClientHelloParser hello_parser_; v8::Global ocsp_response_; - v8::Global sni_context_; + BaseObjectPtr sni_context_; friend class SecureContext; }; diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 626662c9a5ef8e..bacb1a0f27ee79 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -1065,10 +1065,9 @@ int TLSWrap::SelectSNIContextCallback(SSL* s, int* ad, void* arg) { return SSL_TLSEXT_ERR_NOACK; } - p->sni_context_.Reset(env->isolate(), ctx); - SecureContext* sc = Unwrap(ctx.As()); CHECK_NOT_NULL(sc); + p->sni_context_ = BaseObjectPtr(sc); p->SetSNIContext(sc); return SSL_TLSEXT_ERR_OK; } From 03251c56531b0c1dc4387bb51008c29e14e53b51 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 19 Nov 2019 22:15:17 +0100 Subject: [PATCH 3/3] src: inline SetSNICallback Refs: https://github.com/nodejs/node/pull/30548#discussion_r348168855 --- src/node_crypto.cc | 17 +---------------- src/node_crypto.h | 1 - src/tls_wrap.cc | 6 +++++- 3 files changed, 6 insertions(+), 18 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 1121b148552487..82439604e5143b 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -142,7 +142,6 @@ static bool extra_root_certs_loaded = false; template void SSLWrap::AddMethods(Environment* env, Local t); template void SSLWrap::ConfigureSecureContext(SecureContext* sc); -template void SSLWrap::SetSNIContext(SecureContext* sc); template int SSLWrap::SetCACerts(SecureContext* sc); template void SSLWrap::MemoryInfo(MemoryTracker* tracker) const; template SSL_SESSION* SSLWrap::GetSessionCallback( @@ -2993,12 +2992,7 @@ void SSLWrap::CertCbDone(const FunctionCallbackInfo& args) { if (cons->HasInstance(ctx)) { SecureContext* sc = Unwrap(ctx.As()); CHECK_NOT_NULL(sc); - // XXX: There is a method w->SetSNIContext(sc), and you might think that - // it makes sense to call that here and make setting w->sni_context_ part - // of it. In fact, that passes the test suite, although SetSNIContext() - // performs a lot more operations. - // If anybody is familiar enough with the TLS code to know whether it makes - // sense, please do so or document why it doesn't. + // Store the SNI context for later use. w->sni_context_ = BaseObjectPtr(sc); int rv; @@ -3057,15 +3051,6 @@ void SSLWrap::DestroySSL() { } -template -void SSLWrap::SetSNIContext(SecureContext* sc) { - ConfigureSecureContext(sc); - CHECK_EQ(SSL_set_SSL_CTX(ssl_.get(), sc->ctx_.get()), sc->ctx_.get()); - - SetCACerts(sc); -} - - template int SSLWrap::SetCACerts(SecureContext* sc) { int err = SSL_set1_verify_cert_store(ssl_.get(), diff --git a/src/node_crypto.h b/src/node_crypto.h index b9ad5d801c9deb..2638f81d9f72c1 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -288,7 +288,6 @@ class SSLWrap { void DestroySSL(); void WaitForCertCb(CertCb cb, void* arg); - void SetSNIContext(SecureContext* sc); int SetCACerts(SecureContext* sc); inline Environment* ssl_env() const { diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index bacb1a0f27ee79..cd7a5d59ebd842 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -1068,7 +1068,11 @@ int TLSWrap::SelectSNIContextCallback(SSL* s, int* ad, void* arg) { SecureContext* sc = Unwrap(ctx.As()); CHECK_NOT_NULL(sc); p->sni_context_ = BaseObjectPtr(sc); - p->SetSNIContext(sc); + + p->ConfigureSecureContext(sc); + CHECK_EQ(SSL_set_SSL_CTX(p->ssl_.get(), sc->ctx_.get()), sc->ctx_.get()); + p->SetCACerts(sc); + return SSL_TLSEXT_ERR_OK; }