From 415b42fbdfa01c9073767e85759fa1ea4ac5077f Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sat, 13 Nov 2021 03:17:53 +0530 Subject: [PATCH] src,crypto: refactor `crypto_tls.*` By the design of `GetSSLError()`, the V8 API was unnecessarily being accessed in places where it eventually didn't get used. So this refactor inlines the function appropriately in places where it was being used. Also, this replaces uses of `AllocatedBuffers` with `BackingStore`s. Signed-off-by: Darshan Sen PR-URL: https://github.com/nodejs/node/pull/40675 Reviewed-By: Anna Henningsen --- src/crypto/crypto_tls.cc | 229 +++++++++++++++++---------------------- src/crypto/crypto_tls.h | 5 +- 2 files changed, 104 insertions(+), 130 deletions(-) diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index bca554d5db4cef..8efbdeed1d9fc6 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -37,10 +37,11 @@ namespace node { using v8::Array; +using v8::ArrayBuffer; using v8::ArrayBufferView; +using v8::BackingStore; using v8::Context; using v8::DontDelete; -using v8::EscapableHandleScope; using v8::Exception; using v8::False; using v8::Function; @@ -313,6 +314,17 @@ inline bool Set( OneByteString(env->isolate(), value)) .IsNothing(); } + +std::string GetBIOError() { + std::string ret; + ERR_print_errors_cb( + [](const char* str, size_t len, void* opaque) { + static_cast(opaque)->assign(str, len); + return 0; + }, + static_cast(&ret)); + return ret; +} } // namespace TLSWrap::TLSWrap(Environment* env, @@ -572,7 +584,8 @@ void TLSWrap::EncOut() { // No encrypted output ready to write to the underlying stream. if (BIO_pending(enc_out_) == 0) { Debug(this, "No pending encrypted output"); - if (pending_cleartext_input_.size() == 0) { + if (!pending_cleartext_input_ || + pending_cleartext_input_->ByteLength() == 0) { if (!in_dowrite_) { Debug(this, "No pending cleartext input, not inside DoWrite()"); InvokeQueued(0); @@ -665,84 +678,9 @@ void TLSWrap::OnStreamAfterWrite(WriteWrap* req_wrap, int status) { EncOut(); } -MaybeLocal TLSWrap::GetSSLError(int status, int* err, std::string* msg) { - EscapableHandleScope scope(env()->isolate()); - - // ssl_ is already destroyed in reading EOF by close notify alert. - if (ssl_ == nullptr) - return MaybeLocal(); - - *err = SSL_get_error(ssl_.get(), status); - switch (*err) { - case SSL_ERROR_NONE: - case SSL_ERROR_WANT_READ: - case SSL_ERROR_WANT_WRITE: - case SSL_ERROR_WANT_X509_LOOKUP: - return MaybeLocal(); - - case SSL_ERROR_ZERO_RETURN: - return scope.Escape(env()->zero_return_string()); - - case SSL_ERROR_SSL: - case SSL_ERROR_SYSCALL: - { - unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int) - BIO* bio = BIO_new(BIO_s_mem()); - ERR_print_errors(bio); - - BUF_MEM* mem; - BIO_get_mem_ptr(bio, &mem); - - Isolate* isolate = env()->isolate(); - Local context = isolate->GetCurrentContext(); - - Local message = OneByteString(isolate, mem->data, mem->length); - Local exception = Exception::Error(message); - Local obj = - exception->ToObject(context).FromMaybe(Local()); - if (UNLIKELY(obj.IsEmpty())) - return MaybeLocal(); - - const char* ls = ERR_lib_error_string(ssl_err); - const char* fs = ERR_func_error_string(ssl_err); - const char* rs = ERR_reason_error_string(ssl_err); - - if (!Set(env(), obj, env()->library_string(), ls) || - !Set(env(), obj, env()->function_string(), fs)) { - return MaybeLocal(); - } - - if (rs != nullptr) { - if (!Set(env(), obj, env()->reason_string(), rs)) - return MaybeLocal(); - - // SSL has no API to recover the error name from the number, so we - // transform reason strings like "this error" to "ERR_SSL_THIS_ERROR", - // which ends up being close to the original error macro name. - std::string code(rs); - - for (auto& c : code) - c = (c == ' ') ? '_' : ToUpper(c); - - if (!Set(env(), obj, - env()->code_string(), - ("ERR_SSL_" + code).c_str())) { - return MaybeLocal(); - } - } - - if (msg != nullptr) - msg->assign(mem->data, mem->data + mem->length); - - BIO_free_all(bio); - - return scope.Escape(exception); - } - - default: - UNREACHABLE(); - } - UNREACHABLE(); +int TLSWrap::GetSSLError(int status) const { + // ssl_ might already be destroyed for reading EOF from a close notify alert. + return ssl_ != nullptr ? SSL_get_error(ssl_.get(), status) : 0; } void TLSWrap::ClearOut() { @@ -809,24 +747,61 @@ void TLSWrap::ClearOut() { // See node#1642 and SSL_read(3SSL) for details. if (read <= 0) { HandleScope handle_scope(env()->isolate()); - int err; + Local error; + int err = GetSSLError(read); + switch (err) { + case SSL_ERROR_ZERO_RETURN: + // Ignore ZERO_RETURN after EOF, it is basically not an error. + if (eof_) return; + error = env()->zero_return_string(); + break; - Local arg = GetSSLError(read, &err, nullptr) - .FromMaybe(Local()); + case SSL_ERROR_SSL: + case SSL_ERROR_SYSCALL: + { + unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int) + + Local context = env()->isolate()->GetCurrentContext(); + if (UNLIKELY(context.IsEmpty())) return; + const std::string error_str = GetBIOError(); + Local message = OneByteString( + env()->isolate(), error_str.c_str(), error_str.size()); + if (UNLIKELY(message.IsEmpty())) return; + error = Exception::Error(message); + if (UNLIKELY(error.IsEmpty())) return; + Local obj; + if (UNLIKELY(!error->ToObject(context).ToLocal(&obj))) return; + + const char* ls = ERR_lib_error_string(ssl_err); + const char* fs = ERR_func_error_string(ssl_err); + const char* rs = ERR_reason_error_string(ssl_err); + if (!Set(env(), obj, env()->library_string(), ls) || + !Set(env(), obj, env()->function_string(), fs) || + !Set(env(), obj, env()->reason_string(), rs, false)) return; + // SSL has no API to recover the error name from the number, so we + // transform reason strings like "this error" to "ERR_SSL_THIS_ERROR", + // which ends up being close to the original error macro name. + std::string code(rs); + // TODO(RaisinTen): Pass an appropriate execution policy when it is + // implemented in our supported compilers. + std::transform(code.begin(), code.end(), code.begin(), + [](char c) { return c == ' ' ? '_' : ToUpper(c); }); + if (!Set(env(), obj, + env()->code_string(), ("ERR_SSL_" + code).c_str())) return; + } + break; - // Ignore ZERO_RETURN after EOF, it is basically not a error - if (err == SSL_ERROR_ZERO_RETURN && eof_) - return; + default: + return; + } - if (LIKELY(!arg.IsEmpty())) { - Debug(this, "Got SSL error (%d), calling onerror", err); - // When TLS Alert are stored in wbio, - // it should be flushed to socket before destroyed. - if (BIO_pending(enc_out_) != 0) - EncOut(); + Debug(this, "Got SSL error (%d), calling onerror", err); + // When TLS Alert are stored in wbio, + // it should be flushed to socket before destroyed. + if (BIO_pending(enc_out_) != 0) + EncOut(); - MakeCallback(env()->onerror_string(), 1, &arg); - } + MakeCallback(env()->onerror_string(), 1, &error); } } @@ -843,18 +818,19 @@ void TLSWrap::ClearIn() { return; } - if (pending_cleartext_input_.size() == 0) { + if (!pending_cleartext_input_ || + pending_cleartext_input_->ByteLength() == 0) { Debug(this, "Returning from ClearIn(), no pending data"); return; } - AllocatedBuffer data = std::move(pending_cleartext_input_); + std::unique_ptr bs = std::move(pending_cleartext_input_); MarkPopErrorOnReturn mark_pop_error_on_return; - NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(data.size()); - int written = SSL_write(ssl_.get(), data.data(), data.size()); - Debug(this, "Writing %zu bytes, written = %d", data.size(), written); - CHECK(written == -1 || written == static_cast(data.size())); + NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(bs->ByteLength()); + int written = SSL_write(ssl_.get(), bs->Data(), bs->ByteLength()); + Debug(this, "Writing %zu bytes, written = %d", bs->ByteLength(), written); + CHECK(written == -1 || written == static_cast(bs->ByteLength())); // All written if (written != -1) { @@ -863,24 +839,20 @@ void TLSWrap::ClearIn() { } // Error or partial write - HandleScope handle_scope(env()->isolate()); - Context::Scope context_scope(env()->context()); - - int err; - std::string error_str; - MaybeLocal arg = GetSSLError(written, &err, &error_str); - if (!arg.IsEmpty()) { + int err = GetSSLError(written); + if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) { Debug(this, "Got SSL error (%d)", err); write_callback_scheduled_ = true; // TODO(@sam-github) Should forward an error object with // .code/.function/.etc, if possible. - return InvokeQueued(UV_EPROTO, error_str.c_str()); + InvokeQueued(UV_EPROTO, GetBIOError().c_str()); + return; } Debug(this, "Pushing data back"); // Push back the not-yet-written data. This can be skipped in the error // case because no further writes would succeed anyway. - pending_cleartext_input_ = std::move(data); + pending_cleartext_input_ = std::move(bs); } std::string TLSWrap::diagnostic_name() const { @@ -998,7 +970,7 @@ int TLSWrap::DoWrite(WriteWrap* w, return 0; } - AllocatedBuffer data; + std::unique_ptr bs; MarkPopErrorOnReturn mark_pop_error_on_return; int written = 0; @@ -1012,15 +984,19 @@ int TLSWrap::DoWrite(WriteWrap* w, // and copying it when it could just be used. if (nonempty_count != 1) { - data = AllocatedBuffer::AllocateManaged(env(), length); + { + NoArrayBufferZeroFillScope no_zero_fill_scope(env()->isolate_data()); + bs = ArrayBuffer::NewBackingStore(env()->isolate(), length); + } size_t offset = 0; for (i = 0; i < count; i++) { - memcpy(data.data() + offset, bufs[i].base, bufs[i].len); + memcpy(static_cast(bs->Data()) + offset, + bufs[i].base, bufs[i].len); offset += bufs[i].len; } NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(length); - written = SSL_write(ssl_.get(), data.data(), length); + written = SSL_write(ssl_.get(), bs->Data(), length); } else { // Only one buffer: try to write directly, only store if it fails uv_buf_t* buf = &bufs[nonempty_i]; @@ -1028,8 +1004,9 @@ int TLSWrap::DoWrite(WriteWrap* w, written = SSL_write(ssl_.get(), buf->base, buf->len); if (written == -1) { - data = AllocatedBuffer::AllocateManaged(env(), length); - memcpy(data.data(), buf->base, buf->len); + NoArrayBufferZeroFillScope no_zero_fill_scope(env()->isolate_data()); + bs = ArrayBuffer::NewBackingStore(env()->isolate(), length); + memcpy(bs->Data(), buf->base, buf->len); } } @@ -1037,11 +1014,9 @@ int TLSWrap::DoWrite(WriteWrap* w, Debug(this, "Writing %zu bytes, written = %d", length, written); if (written == -1) { - int err; - MaybeLocal arg = GetSSLError(written, &err, &error_); - // If we stopped writing because of an error, it's fatal, discard the data. - if (!arg.IsEmpty()) { + int err = GetSSLError(written); + if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) { // TODO(@jasnell): What are we doing with the error? Debug(this, "Got SSL error (%d), returning UV_EPROTO", err); current_write_.reset(); @@ -1050,8 +1025,9 @@ int TLSWrap::DoWrite(WriteWrap* w, Debug(this, "Saving data for later write"); // Otherwise, save unwritten data so it can be written later by ClearIn(). - CHECK_EQ(pending_cleartext_input_.size(), 0); - pending_cleartext_input_ = std::move(data); + CHECK(!pending_cleartext_input_ || + pending_cleartext_input_->ByteLength() == 0); + pending_cleartext_input_ = std::move(bs); } // Write any encrypted/handshake output that may be ready. @@ -1491,9 +1467,8 @@ void TLSWrap::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("ocsp_response", ocsp_response_); tracker->TrackField("sni_context", sni_context_); tracker->TrackField("error", error_); - tracker->TrackFieldWithSize("pending_cleartext_input", - pending_cleartext_input_.size(), - "AllocatedBuffer"); + if (pending_cleartext_input_) + tracker->TrackField("pending_cleartext_input", pending_cleartext_input_); if (enc_in_ != nullptr) tracker->TrackField("enc_in", NodeBIO::FromBIO(enc_in_)); if (enc_out_ != nullptr) @@ -1724,13 +1699,13 @@ void TLSWrap::VerifyError(const FunctionCallbackInfo& args) { const char* reason = X509_verify_cert_error_string(x509_verify_error); const char* code = X509ErrorCode(x509_verify_error); - Local exception = + Local error = Exception::Error(OneByteString(env->isolate(), reason)) ->ToObject(env->isolate()->GetCurrentContext()) .FromMaybe(Local()); - if (Set(env, exception, env->code_string(), code)) - args.GetReturnValue().Set(exception); + if (Set(env, error, env->code_string(), code)) + args.GetReturnValue().Set(error); } void TLSWrap::GetCipher(const FunctionCallbackInfo& args) { diff --git a/src/crypto/crypto_tls.h b/src/crypto/crypto_tls.h index c4cf4c60e8e51d..765c6c476b12e7 100644 --- a/src/crypto/crypto_tls.h +++ b/src/crypto/crypto_tls.h @@ -27,7 +27,6 @@ #include "crypto/crypto_context.h" #include "crypto/crypto_clienthello.h" -#include "allocated_buffer.h" #include "async_wrap.h" #include "stream_wrap.h" #include "v8.h" @@ -167,7 +166,7 @@ class TLSWrap : public AsyncWrap, int SetCACerts(SecureContext* sc); - v8::MaybeLocal GetSSLError(int status, int* err, std::string* msg); + int GetSSLError(int status) const; static int SelectSNIContextCallback(SSL* s, int* ad, void* arg); @@ -254,7 +253,7 @@ class TLSWrap : public AsyncWrap, BIO* enc_in_ = nullptr; // StreamListener fills this for SSL_read(). BIO* enc_out_ = nullptr; // SSL_write()/handshake fills this for EncOut(). // Waiting for ClearIn() to pass to SSL_write(). - AllocatedBuffer pending_cleartext_input_; + std::unique_ptr pending_cleartext_input_; size_t write_size_ = 0; BaseObjectPtr current_write_; BaseObjectPtr current_empty_write_;