diff --git a/doc/api/dtls.md b/doc/api/dtls.md index 444e5cbf0d7bb1..df0e5b50a42840 100644 --- a/doc/api/dtls.md +++ b/doc/api/dtls.md @@ -123,8 +123,15 @@ added: REPLACEME * `ca` {string|Buffer|string\[]|Buffer\[]} CA certificates in PEM format. * `cert` {string|Buffer} Client certificate in PEM format. * `key` {string|Buffer} Client private key in PEM format. - * `rejectUnauthorized` {boolean} Reject connections with unverifiable - certificates. **Default:** `true`. + * `rejectUnauthorized` {boolean} When `true`, the server's certificate must + both chain to a trusted CA and match the expected identity (`servername`, + or `host` when `servername` is not set); otherwise the handshake is + aborted and `session.opened` rejects. When `false`, the certificate is not + verified. **Default:** `true`. + * `servername` {string} Server name used for the SNI (Server Name + Indication) extension and as the identity checked during certificate + verification. **Default:** the `host` argument. Set to `''` to disable SNI. + SNI is never sent for IP address literals. * `bindHost` {string} Local bind address. **Default:** `'0.0.0.0'`. * `bindPort` {number} Local bind port. **Default:** `0` (ephemeral). * `alpn` {string\[]|Buffer} ALPN protocol names. diff --git a/lib/internal/dtls/dtls.js b/lib/internal/dtls/dtls.js index c4aab52e6e76f3..e6017e3e876194 100644 --- a/lib/internal/dtls/dtls.js +++ b/lib/internal/dtls/dtls.js @@ -7,6 +7,7 @@ const { ArrayIsArray, FunctionPrototypeBind, + PromisePrototypeThen, PromiseWithResolvers, SafeSet, SymbolAsyncDispose, @@ -41,6 +42,10 @@ const { Buffer, } = require('buffer'); +const { + isIP, +} = require('internal/net'); + const { DTLSEndpointState, DTLSSessionState, @@ -102,6 +107,12 @@ class DTLSSession { kPrivateConstructor, handle.getStats()); this.#pendingOpen = PromiseWithResolvers(); this.#pendingClose = PromiseWithResolvers(); + // opened/closed may reject (handshake error, destroy(error)). Attach a + // no-op rejection handler so a caller that uses the callback API and never + // awaits them does not trigger an unhandled rejection; an explicit + // await/then/catch on opened/closed still observes the rejection. + PromisePrototypeThen(this.#pendingOpen.promise, undefined, () => {}); + PromisePrototypeThen(this.#pendingClose.promise, undefined, () => {}); } // --- Callback setters --- @@ -261,6 +272,22 @@ class DTLSSession { this.#onerror(error); } this.#pendingOpen.reject(error); + + // The session has failed and cannot continue. Tear it down so it does not + // linger in the endpoint's table, and -- for a client session that owns + // its internal endpoint -- close the endpoint too so the event loop can + // drain. destroy() removes the session from the C++ table first, so the + // endpoint.close() below won't try to re-close it. Reentrant destroy from + // within the error emit is safe: Cycle()/the timer hold a strong ref. + const endpoint = this.#endpoint; + const ownsEndpoint = this.#ownsEndpoint; + this.destroy(); + if (endpoint) { + endpoint.sessions.delete(this); + if (ownsEndpoint) { + endpoint.close(); + } + } } [kSessionClose]() { @@ -314,6 +341,9 @@ class DTLSEndpoint { this.#stats = new DTLSEndpointStats( kPrivateConstructor, this.#handle.getStats()); this.#pendingClose = PromiseWithResolvers(); + // See DTLSSession: keep an unobserved closed rejection from surfacing as an + // unhandled rejection. + PromisePrototypeThen(this.#pendingClose.promise, undefined, () => {}); if (options.mtu !== undefined) { validateInteger(options.mtu, 'options.mtu', 256, 65535); @@ -359,10 +389,24 @@ class DTLSEndpoint { // --- Client mode --- connect(context, host, port, servername) { - const sessionHandle = this.#handle.connect(context, host, port); - if (servername) { - sessionHandle.setServername(servername); + // Resolve SNI and the expected peer identity here so that every caller of + // the endpoint API -- not only the top-level dtls.connect() -- gets safe + // defaults. The identity is always bound to the requested servername (or, + // failing that, the host). OpenSSL only *enforces* it when the context is + // in a verifying mode, so binding it is a no-op for non-verifying + // (rejectUnauthorized: false) contexts. + // + // These are applied to the client SSL inside the binding, before the + // handshake's ClientHello is emitted; they cannot be set afterwards. + let sni = servername !== undefined ? (servername || undefined) : host; + if (sni !== undefined && isIP(sni) !== 0) { + sni = undefined; // SNI is never sent for IP literals (matching TLS). } + const verifyHost = servername || host; + const verifyIsIp = isIP(verifyHost) !== 0; + + const sessionHandle = this.#handle.connect( + context, host, port, sni, verifyHost, verifyIsIp); const session = new DTLSSession( kPrivateConstructor, sessionHandle, this); this.#sessions.add(session); @@ -604,7 +648,12 @@ function listen(onsession, options = kEmptyObject) { * @param {string|Buffer|Array} [options.ca] CA certificates (PEM). * @param {string|Buffer} [options.cert] Client certificate (PEM). * @param {string|Buffer} [options.key] Client private key (PEM). - * @param {boolean} [options.rejectUnauthorized] Reject unauthorized. + * @param {boolean} [options.rejectUnauthorized] When true (default), verify + * the server certificate against the trusted CAs and check its identity + * against servername (or host); aborts the handshake on failure. + * @param {string} [options.servername] Server name for the SNI extension and + * the identity checked during certificate verification. Defaults to host; + * set to '' to disable SNI. Never sent for IP address literals. * @param {string} [options.bindHost] Local bind address. * @param {number} [options.bindPort] Local bind port (0 = ephemeral). * @param {number} [options.mtu] MTU for DTLS records. @@ -632,13 +681,11 @@ function connect(host, port, options = kEmptyObject) { endpoint.bind(bindHost, bindPort); - // Default SNI servername to the host argument (matching Node.js TLS). - // Can be overridden with options.servername, or disabled with '' or false. - const servername = options.servername !== undefined ? - (options.servername || undefined) : - host; - - const session = endpoint.connect(context, host, port, servername); + // SNI and peer-identity verification are resolved inside + // DTLSEndpoint.connect(), which defaults both to the host argument (matching + // Node.js TLS). The identity is enforced whenever the context verifies, i.e. + // unless rejectUnauthorized is false. + const session = endpoint.connect(context, host, port, options.servername); // Mark that this session owns the endpoint so it gets closed // automatically when the session closes, allowing process exit. session.ownsEndpoint = true; diff --git a/src/dtls/dtls_context.cc b/src/dtls/dtls_context.cc index ca5003df46c295..d59ee74feeb9e0 100644 --- a/src/dtls/dtls_context.cc +++ b/src/dtls/dtls_context.cc @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -35,6 +36,11 @@ namespace dtls { namespace { // The cookie secret is 32 bytes (256 bits). constexpr size_t kCookieSecretLen = 32; +// Cookies are bound to a coarse time window so they expire. A cookie is +// accepted for the window it was minted in and the immediately preceding one, +// giving ~30-60s of validity -- ample for the cookie exchange while bounding +// how long a captured cookie can be replayed. +constexpr uint64_t kCookieWindowNs = 30ull * 1000 * 1000 * 1000; } // namespace DTLSContext::DTLSContext(Environment* env, @@ -317,8 +323,11 @@ void DTLSContext::SetALPN(const FunctionCallbackInfo& args) { ctx->alpn_protos_.assign(data, data + len); SSL_CTX_set_alpn_select_cb(ctx->ctx_.get(), ALPNSelectCallback, ctx); } else { - // Client: advertise protocols to the server. - SSL_CTX_set_alpn_protos(ctx->ctx_.get(), data, len); + // Client: advertise protocols to the server. Returns 0 on success. + if (SSL_CTX_set_alpn_protos(ctx->ctx_.get(), data, len) != 0) { + return THROW_ERR_CRYPTO_OPERATION_FAILED( + env, "SSL_CTX_set_alpn_protos failed"); + } } } @@ -368,64 +377,77 @@ void DTLSContext::SetECDHCurve(const FunctionCallbackInfo& args) { } } -// HMAC-SHA256 based cookie generation using the peer's address. -// During DTLSv1_listen(), the peer address is taken from -// DTLSContext::current_cookie_peer_ (set synchronously before the call). -// During session handshake, the peer address is taken from the +// HMAC-SHA256 cookie derived from the peer's address and a coarse time window +// so cookies expire (see kCookieWindowNs). During DTLSv1_listen() the peer +// address comes from DTLSContext::current_cookie_peer_ (set synchronously +// before the call); during the session handshake it comes from the // DTLSSession stored in SSL app_data. -int DTLSContext::CookieGenerateCallback(SSL* ssl, - unsigned char* cookie, - unsigned int* cookie_len) { +bool DTLSContext::ComputeCookie(SSL* ssl, + uint64_t window, + unsigned char* out, + unsigned int* out_len) { SSL_CTX* ctx = SSL_get_SSL_CTX(ssl); DTLSContext* dtls_ctx = static_cast(SSL_CTX_get_app_data(ctx)); CHECK_NOT_NULL(dtls_ctx); - unsigned char addr_buf[sizeof(struct sockaddr_storage)]; + // Message = peer address bytes followed by the 8-byte window counter. + unsigned char msg[sizeof(struct sockaddr_storage) + sizeof(uint64_t)]; size_t addr_len = 0; void* app_data = SSL_get_app_data(ssl); if (app_data != nullptr) { // Session handshake path. - auto* session = static_cast(app_data); - const sockaddr* sa = session->remote_address().data(); + const sockaddr* sa = + static_cast(app_data)->remote_address().data(); addr_len = SocketAddress::GetLength(sa); - memcpy(addr_buf, sa, addr_len); + memcpy(msg, sa, addr_len); } else { - // DTLSv1_listen path — use the peer address stored on the context. + // DTLSv1_listen path -- use the peer address stored on the context. const sockaddr* sa = dtls_ctx->current_cookie_peer_.data(); addr_len = SocketAddress::GetLength(sa); - memcpy(addr_buf, sa, addr_len); + memcpy(msg, sa, addr_len); + } + + // Append the window counter in a fixed byte order. + for (size_t i = 0; i < sizeof(uint64_t); i++) { + msg[addr_len + i] = static_cast((window >> (8 * i)) & 0xff); } - unsigned int hmac_len = 0; unsigned char* result = HMAC(EVP_sha256(), dtls_ctx->cookie_secret_.data(), dtls_ctx->cookie_secret_.size(), - addr_buf, - addr_len, - cookie, - &hmac_len); - - if (result == nullptr) return 0; + msg, + addr_len + sizeof(uint64_t), + out, + out_len); + return result != nullptr; +} - *cookie_len = hmac_len; - return 1; +int DTLSContext::CookieGenerateCallback(SSL* ssl, + unsigned char* cookie, + unsigned int* cookie_len) { + const uint64_t window = uv_hrtime() / kCookieWindowNs; + return ComputeCookie(ssl, window, cookie, cookie_len) ? 1 : 0; } int DTLSContext::CookieVerifyCallback(SSL* ssl, const unsigned char* cookie, unsigned int cookie_len) { - // Generate the expected cookie and compare. + const uint64_t window = uv_hrtime() / kCookieWindowNs; + + // Accept a cookie minted in the current window or the immediately preceding + // one, so a handshake that straddles a window boundary still succeeds. unsigned char expected[EVP_MAX_MD_SIZE]; unsigned int expected_len = 0; - - if (CookieGenerateCallback(ssl, expected, &expected_len) != 1) { - return 0; + for (int i = 0; i < 2; i++) { + if (i == 1 && window == 0) break; + if (ComputeCookie(ssl, window - i, expected, &expected_len) && + cookie_len == expected_len && + CRYPTO_memcmp(cookie, expected, expected_len) == 0) { + return 1; + } } - - if (cookie_len != expected_len) return 0; - - return CRYPTO_memcmp(cookie, expected, expected_len) == 0 ? 1 : 0; + return 0; } int DTLSContext::ALPNSelectCallback(SSL* ssl, diff --git a/src/dtls/dtls_context.h b/src/dtls/dtls_context.h index 11d8113d308125..5c994ea769de21 100644 --- a/src/dtls/dtls_context.h +++ b/src/dtls/dtls_context.h @@ -57,6 +57,14 @@ class DTLSContext final : public BaseObject { static void LoadDefaultCAs(const v8::FunctionCallbackInfo& args); static void SetECDHCurve(const v8::FunctionCallbackInfo& args); + // Compute the address-and-time-window-bound cookie for |window| into |out| + // (which must have room for EVP_MAX_MD_SIZE bytes). Shared by the cookie + // generate/verify callbacks. + static bool ComputeCookie(SSL* ssl, + uint64_t window, + unsigned char* out, + unsigned int* out_len); + // Automatic DTLS cookie callbacks static int CookieGenerateCallback(SSL* ssl, unsigned char* cookie, diff --git a/src/dtls/dtls_endpoint.cc b/src/dtls/dtls_endpoint.cc index 9433241a2d1433..366c779ed1374e 100644 --- a/src/dtls/dtls_endpoint.cc +++ b/src/dtls/dtls_endpoint.cc @@ -155,7 +155,10 @@ int DTLSEndpoint::Listen(DTLSContext* context) { } BaseObjectPtr DTLSEndpoint::Connect(DTLSContext* context, - const SocketAddress& remote) { + const SocketAddress& remote, + const char* servername, + const char* verify_host, + bool verify_is_ip) { if (IsHandleClosing()) { THROW_ERR_INVALID_STATE(env(), "Endpoint is closing"); return {}; @@ -168,8 +171,14 @@ BaseObjectPtr DTLSEndpoint::Connect(DTLSContext* context, return {}; } - auto session = DTLSSession::Create( - env(), this, context->ssl_ctx(), remote, false /* is_server */); + auto session = DTLSSession::Create(env(), + this, + context->ssl_ctx(), + remote, + false /* is_server */, + servername, + verify_host, + verify_is_ip); if (!session) return {}; @@ -331,8 +340,16 @@ void DTLSEndpoint::SetCallbacks(Local callbacks) { void DTLSEndpoint::OnAlloc(uv_handle_t* handle, size_t suggested_size, uv_buf_t* buf) { - buf->base = new char[65536]; - buf->len = 65536; + DTLSEndpoint* endpoint = static_cast(handle->data); + // Reuse a single receive buffer. libuv delivers datagrams one at a time on + // this thread, and OnRecv fully consumes each datagram (copying it into the + // session's BIO) before the next OnAlloc, so a per-endpoint buffer suffices + // and avoids a heap allocation on every packet. + if (endpoint->recv_buf_.empty()) { + endpoint->recv_buf_.resize(65536); + } + buf->base = endpoint->recv_buf_.data(); + buf->len = endpoint->recv_buf_.size(); } void DTLSEndpoint::OnRecv(uv_udp_t* handle, @@ -342,13 +359,12 @@ void DTLSEndpoint::OnRecv(uv_udp_t* handle, unsigned int flags) { DTLSEndpoint* endpoint = static_cast(handle->data); + // buf->base is the endpoint's reusable recv_buf_; it is not freed here. if (nread == 0 && addr == nullptr) { - delete[] buf->base; return; } if (nread < 0) { - delete[] buf->base; HandleScope handle_scope(endpoint->env()->isolate()); Context::Scope context_scope(endpoint->env()->context()); Local argv[] = { @@ -363,7 +379,6 @@ void DTLSEndpoint::OnRecv(uv_udp_t* handle, } if (addr == nullptr) { - delete[] buf->base; return; } @@ -375,8 +390,6 @@ void DTLSEndpoint::OnRecv(uv_udp_t* handle, SocketAddress remote(addr); endpoint->ProcessDatagram( reinterpret_cast(buf->base), nread, remote); - - delete[] buf->base; } void DTLSEndpoint::OnSend(uv_udp_send_t* req, int status) { @@ -532,6 +545,9 @@ void DTLSEndpoint::DoBind(const FunctionCallbackInfo& args) { return THROW_ERR_INVALID_ARG_VALUE(env, "Invalid address"); } + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kNet, addr.ToString()); + int err = endpoint->Bind(addr); if (err != 0) { return THROW_ERR_INVALID_STATE(env, uv_strerror(err)); @@ -576,7 +592,17 @@ void DTLSEndpoint::DoConnect(const FunctionCallbackInfo& args) { THROW_IF_INSUFFICIENT_PERMISSIONS( env, permission::PermissionScope::kNet, remote.ToString()); - auto session = endpoint->Connect(context, remote); + // Optional: servername (SNI), verifyHost (expected peer identity), and + // whether verifyHost is an IP literal. These are resolved in JS and applied + // to the client SSL before the handshake starts. + Utf8Value servername(env->isolate(), args[3]); + Utf8Value verify_host(env->isolate(), args[4]); + const char* servername_ptr = args[3]->IsString() ? *servername : nullptr; + const char* verify_host_ptr = args[4]->IsString() ? *verify_host : nullptr; + bool verify_is_ip = args[5]->IsTrue(); + + auto session = endpoint->Connect( + context, remote, servername_ptr, verify_host_ptr, verify_is_ip); if (session) { args.GetReturnValue().Set(session->object()); } @@ -641,6 +667,7 @@ void DTLSEndpoint::DoSetCallbacks(const FunctionCallbackInfo& args) { void DTLSEndpoint::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("sessions", sessions_.size()); + tracker->TrackFieldWithSize("recv_buf", recv_buf_.size()); } } // namespace dtls diff --git a/src/dtls/dtls_endpoint.h b/src/dtls/dtls_endpoint.h index a6fe94fff5b8cc..d0a1e5652dcb1d 100644 --- a/src/dtls/dtls_endpoint.h +++ b/src/dtls/dtls_endpoint.h @@ -14,6 +14,7 @@ #include #include +#include #include "dtls.h" #include "dtls_context.h" @@ -59,9 +60,15 @@ class DTLSEndpoint final : public HandleWrap { int Listen(DTLSContext* context); // Initiate a client connection to the given address. + // |servername|/|verify_host|/|verify_is_ip| configure SNI and peer identity + // verification on the client SSL before the handshake begins; see + // DTLSSession::Create. // Returns the created DTLSSession. BaseObjectPtr Connect(DTLSContext* context, - const SocketAddress& remote); + const SocketAddress& remote, + const char* servername = nullptr, + const char* verify_host = nullptr, + bool verify_is_ip = false); // Send a raw UDP datagram to the given address. // Called by DTLSSession to send encrypted packets. @@ -129,6 +136,11 @@ class DTLSEndpoint final : public HandleWrap { uv_udp_t handle_; + // Reusable receive buffer for uv_udp_recv (see OnAlloc). libuv delivers one + // datagram at a time and OnRecv consumes each before the next OnAlloc, so a + // single buffer per endpoint avoids a heap allocation on every packet. + std::vector recv_buf_; + // Session table: maps remote address -> session. std::unordered_map, diff --git a/src/dtls/dtls_session.cc b/src/dtls/dtls_session.cc index 8bcd06a4ae71d9..6f510a94efc232 100644 --- a/src/dtls/dtls_session.cc +++ b/src/dtls/dtls_session.cc @@ -18,6 +18,8 @@ #include #include #include +#include +#include #include @@ -53,6 +55,10 @@ DTLSSession::DTLSSession(Environment* env, retransmit_timer_(env, [this] { if (destroyed_) return; + // Keep ourselves alive across the callback: emitting + // an error or running Cycle() below can synchronously + // destroy this session, and this timer lives on it. + BaseObjectPtr strong_ref{this}; DTLS_STAT_INCREMENT(DTLSSessionStats, retransmit_count); int ret = DTLSv1_handle_timeout(ssl_.get()); @@ -115,7 +121,6 @@ Local DTLSSession::GetConstructorTemplate(Environment* env) { SetProtoMethod(isolate, tmpl, "getALPNProtocol", GetALPNProtocol); SetProtoMethod(isolate, tmpl, "exportKeyingMaterial", ExportKeyingMaterial); SetProtoMethod(isolate, tmpl, "getSRTPProfile", GetSRTPProfile); - SetProtoMethod(isolate, tmpl, "setServername", SetServername); SetProtoMethod(isolate, tmpl, "getServername", GetServername); env->set_dtls_session_constructor_template(tmpl); @@ -145,7 +150,6 @@ void DTLSSession::RegisterExternalReferences( registry->Register(GetALPNProtocol); registry->Register(ExportKeyingMaterial); registry->Register(GetSRTPProfile); - registry->Register(SetServername); registry->Register(GetServername); } @@ -153,7 +157,10 @@ BaseObjectPtr DTLSSession::Create(Environment* env, DTLSEndpoint* endpoint, SSL_CTX* ssl_ctx, const SocketAddress& remote, - bool is_server) { + bool is_server, + const char* servername, + const char* verify_host, + bool verify_is_ip) { // Create the SSL object. SSL* ssl_raw = SSL_new(ssl_ctx); if (ssl_raw == nullptr) { @@ -188,6 +195,43 @@ BaseObjectPtr DTLSSession::Create(Environment* env, SSL_set_accept_state(ssl.get()); } else { SSL_set_connect_state(ssl.get()); + + // Configure SNI and peer identity verification BEFORE the handshake + // starts. The caller (DTLSEndpoint::Connect) runs Cycle() immediately + // after Create() returns, which emits the ClientHello, so anything that + // must appear in that flight (SNI) has to be set here rather than via a + // post-construction setter. + if (servername != nullptr && servername[0] != '\0') { + if (!SSL_set_tlsext_host_name(ssl.get(), servername)) { + THROW_ERR_CRYPTO_OPERATION_FAILED(env, + "Failed to set servername (SNI)"); + return {}; + } + } + + // When identity verification is requested, bind the expected peer name + // (or IP) into the verification parameters. Combined with the context's + // SSL_VERIFY_PEER mode this makes a name mismatch fail the handshake, + // rather than accepting any certificate that merely chains to a trusted + // CA. A failure to configure it is fatal: proceeding would silently skip + // the identity check. + if (verify_host != nullptr && verify_host[0] != '\0') { + if (verify_is_ip) { + if (!X509_VERIFY_PARAM_set1_ip_asc(SSL_get0_param(ssl.get()), + verify_host)) { + THROW_ERR_CRYPTO_OPERATION_FAILED( + env, "Failed to set peer IP address for verification"); + return {}; + } + } else { + SSL_set_hostflags(ssl.get(), X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS); + if (!SSL_set1_host(ssl.get(), verify_host)) { + THROW_ERR_CRYPTO_OPERATION_FAILED( + env, "Failed to set peer hostname for verification"); + return {}; + } + } + } } // Create the JS wrapper object. @@ -246,6 +290,12 @@ void DTLSSession::Receive(const uint8_t* data, size_t len) { void DTLSSession::Cycle() { if (destroyed_) return; + // Pin a strong reference to ourselves for the duration of the pump. A JS + // callback dispatched below (message/handshake/error) can synchronously + // destroy this session, which removes the endpoint's only strong reference + // and would otherwise free `this` while we are still using ssl_/state_. + BaseObjectPtr strong_ref{this}; + // Prevent infinite recursion. if (++cycle_depth_ > 1) { cycle_depth_--; @@ -264,6 +314,9 @@ void DTLSSession::Cycle() { unsigned long ossl_err = ERR_get_error(); // NOLINT(runtime/int) char err_buf[256]; ERR_error_string_n(ossl_err, err_buf, sizeof(err_buf)); + // Flush any fatal alert OpenSSL queued for the peer before emitting the + // error, which tears the session down and detaches the endpoint. + EncOut(); Local argv[] = { String::NewFromUtf8(env()->isolate(), err_buf).ToLocalChecked(), }; @@ -303,8 +356,9 @@ void DTLSSession::Cycle() { void DTLSSession::ClearOut() { if (destroyed_) return; - // Try to read decrypted application data from OpenSSL. - uint8_t buf[65536]; + // Try to read decrypted application data from OpenSSL. A DTLS record's + // plaintext is at most 2^14 bytes, so one SSL_read yields at most that much. + uint8_t buf[16384]; int read; while ((read = SSL_read(ssl_.get(), buf, sizeof(buf))) > 0) { @@ -316,6 +370,9 @@ void DTLSSession::ClearOut() { .ToLocalChecked(), }; EmitCallback(DTLS_CB_SESSION_MESSAGE, 1, argv); + // The message handler may have destroyed the session synchronously; stop + // reading if so (Cycle()'s strong reference keeps `this` itself alive). + if (destroyed_) return; } int err = SSL_get_error(ssl_.get(), read); @@ -336,6 +393,10 @@ void DTLSSession::ClearOut() { EncOut(); Local argv[] = {}; EmitCallback(DTLS_CB_SESSION_CLOSE, 0, argv); + // Drop ourselves from the endpoint's session table now that the peer + // has closed. Safe here: Cycle() holds a strong reference for the + // duration of the pump. + Destroy(); } break; @@ -344,6 +405,9 @@ void DTLSSession::ClearOut() { unsigned long ossl_err = ERR_get_error(); // NOLINT(runtime/int) char err_buf[256]; ERR_error_string_n(ossl_err, err_buf, sizeof(err_buf)); + // Flush any fatal alert OpenSSL queued for the peer before emitting the + // error, which tears the session down and detaches the endpoint. + EncOut(); Local argv[] = { String::NewFromUtf8(env()->isolate(), err_buf).ToLocalChecked(), }; @@ -404,6 +468,12 @@ int DTLSSession::Send(const uint8_t* data, size_t len) { void DTLSSession::Close() { if (destroyed_ || closed_) return; + // Emitting the close below can synchronously free this session (a client + // session that owns its endpoint tears the endpoint -- and thus itself -- + // down from the close callback), and we call Destroy() afterwards. Pin a + // strong reference so `this` survives until we return. + BaseObjectPtr strong_ref{this}; + closed_ = true; state_->closing = 1; DTLS_STAT_RECORD_TIMESTAMP(DTLSSessionStats, closing_at); @@ -425,6 +495,12 @@ void DTLSSession::Close() { Context::Scope context_scope(env()->context()); Local argv[] = {}; EmitCallback(DTLS_CB_SESSION_CLOSE, 0, argv); + + // Remove ourselves from the endpoint's session table and release resources. + // Without this, a gracefully-closed session would linger in the table for + // the life of the endpoint, leaking memory and blocking reuse of its + // address. + Destroy(); } void DTLSSession::Destroy() { @@ -659,15 +735,6 @@ void DTLSSession::GetSRTPProfile(const FunctionCallbackInfo& args) { } } -void DTLSSession::SetServername(const FunctionCallbackInfo& args) { - DTLSSession* session; - ASSIGN_OR_RETURN_UNWRAP(&session, args.This()); - - CHECK(args[0]->IsString()); - Utf8Value servername(session->env()->isolate(), args[0]); - SSL_set_tlsext_host_name(session->ssl_.get(), *servername); -} - void DTLSSession::GetServername(const FunctionCallbackInfo& args) { DTLSSession* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.This()); diff --git a/src/dtls/dtls_session.h b/src/dtls/dtls_session.h index d64d0e4d48736a..162752b6eb4c8c 100644 --- a/src/dtls/dtls_session.h +++ b/src/dtls/dtls_session.h @@ -54,11 +54,19 @@ class DTLSSession final : public AsyncWrap { // |ssl_ctx| - the SSL_CTX to create the SSL* from // |remote| - the peer address // |is_server| - true if this is a server-side session + // |servername| - SNI to advertise (client only); nullptr to omit. + // |verify_host| - expected peer identity to verify (client only); + // nullptr disables identity checking. + // |verify_is_ip| - true if |verify_host| is an IP literal (verified + // against iPAddress SANs) rather than a DNS name. static BaseObjectPtr Create(Environment* env, DTLSEndpoint* endpoint, SSL_CTX* ssl_ctx, const SocketAddress& remote, - bool is_server); + bool is_server, + const char* servername = nullptr, + const char* verify_host = nullptr, + bool verify_is_ip = false); // Create a session from an already-initialized SSL object. // Used by the server after DTLSv1_listen() returns 1 — the SSL @@ -119,7 +127,6 @@ class DTLSSession final : public AsyncWrap { static void ExportKeyingMaterial( const v8::FunctionCallbackInfo& args); static void GetSRTPProfile(const v8::FunctionCallbackInfo& args); - static void SetServername(const v8::FunctionCallbackInfo& args); static void GetServername(const v8::FunctionCallbackInfo& args); public: diff --git a/test/parallel/test-dtls-connect-error-cleanup.mjs b/test/parallel/test-dtls-connect-error-cleanup.mjs new file mode 100644 index 00000000000000..63297f499b942d --- /dev/null +++ b/test/parallel/test-dtls-connect-error-cleanup.mjs @@ -0,0 +1,48 @@ +// Flags: --experimental-dtls --no-warnings + +// Test: a client connect() whose handshake fails must tear down its internally +// owned endpoint, so the event loop can drain. Regression test for a failed +// connect leaking the endpoint (and hanging the process). + +import { hasCrypto, skip, mustCall } from '../common/index.mjs'; +import assert from 'node:assert'; +import * as fixtures from '../common/fixtures.mjs'; + +const { rejects } = assert; +const { readKey } = fixtures; + +if (!hasCrypto) { + skip('missing crypto'); +} + +if (!process.features.dtls) { + skip('DTLS is not enabled'); +} + +const { listen, connect } = await import('node:dtls'); + +const cert = readKey('agent1-cert.pem').toString(); +const key = readKey('agent1-key.pem').toString(); +const ca = readKey('ca1-cert.pem').toString(); + +const server = listen(mustCall((session) => { + // The client rejects the certificate mid-handshake, so this server session + // never opens; ignore its unsettled promise. + session.opened.catch(() => {}); +}), { cert, key, port: 0, host: '127.0.0.1' }); + +// A servername that does not match the certificate, under rejectUnauthorized, +// makes the client's handshake fail during verification. +const session = connect('127.0.0.1', server.address.port, { + ca: [ca], + rejectUnauthorized: true, + servername: 'wrong.example.com', +}); + +await rejects(session.opened, /certificate verify failed/i); + +// The failed connect must have closed its internally-owned endpoint. Without +// that, this await never settles and the test times out. +await session.endpoint.closed; + +await server.close(); diff --git a/test/parallel/test-dtls-destroy-in-callback.mjs b/test/parallel/test-dtls-destroy-in-callback.mjs new file mode 100644 index 00000000000000..e8fa0f5b3eed34 --- /dev/null +++ b/test/parallel/test-dtls-destroy-in-callback.mjs @@ -0,0 +1,84 @@ +// Flags: --experimental-dtls --no-warnings + +// Test: destroying a DTLS session synchronously from within a callback that is +// dispatched from the session's own I/O pump must not crash. The endpoint's +// session table holds the only strong reference to the session, so a reentrant +// destroy() removes it mid-pump; the implementation must keep the object alive +// until the pump unwinds (regression test for a use-after-free). + +import { hasCrypto, skip, mustCall } from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; + +const { readKey } = fixtures; + +if (!hasCrypto) { + skip('missing crypto'); +} + +if (!process.features.dtls) { + skip('DTLS is not enabled'); +} + +const { listen, connect } = await import('node:dtls'); + +const cert = readKey('agent1-cert.pem').toString(); +const key = readKey('agent1-key.pem').toString(); +const ca = readKey('ca1-cert.pem').toString(); + +// --------------------------------------------------------------------------- +// Case 1: destroy the (server) session from inside onmessage. The datagram +// carrying the message drives receive -> pump -> onmessage -> destroy(), which +// frees the session's map entry while ClearOut() is still looping over ssl_. +{ + const destroyed = Promise.withResolvers(); + + const server = listen(mustCall((session) => { + session.onmessage = mustCall(() => { + session.destroy(); + destroyed.resolve(); + }); + }), { cert, key, port: 0, host: '127.0.0.1' }); + + const client = connect('127.0.0.1', server.address.port, { + ca: [ca], + rejectUnauthorized: false, + }); + + await client.opened; + client.send('destroy me from onmessage'); + + await destroyed.promise; + + await client.close(); + await server.close(); +} + +// --------------------------------------------------------------------------- +// Case 2: destroy the (server) session from inside onhandshake. Handshake +// completion is emitted from the middle of the pump (Cycle), so destroying +// there must not free the session before the pump finishes unwinding. +{ + const destroyed = Promise.withResolvers(); + + const server = listen(mustCall((session) => { + session.onhandshake = mustCall(() => { + session.destroy(); + destroyed.resolve(); + }); + }), { cert, key, port: 0, host: '127.0.0.1' }); + + const client = connect('127.0.0.1', server.address.port, { + ca: [ca], + rejectUnauthorized: false, + }); + + // The server tears its session down the moment its handshake completes. The + // client's own handshake normally resolves, but guard against it rejecting + // (racing the teardown) so it can't surface as an unhandled rejection. + client.opened.catch(() => {}); + + await destroyed.promise; + + await client.close(); + await server.close(); +} diff --git a/test/parallel/test-dtls-servername-invalid.mjs b/test/parallel/test-dtls-servername-invalid.mjs new file mode 100644 index 00000000000000..a15752345cc761 --- /dev/null +++ b/test/parallel/test-dtls-servername-invalid.mjs @@ -0,0 +1,27 @@ +// Flags: --experimental-dtls --no-warnings + +// Test: connect() surfaces a failure to set the SNI servername (for example a +// name longer than the TLS maximum of 255 bytes) as an error, rather than +// silently ignoring it. + +import { hasCrypto, skip } from '../common/index.mjs'; +import assert from 'node:assert'; + +if (!hasCrypto) { + skip('missing crypto'); +} + +if (!process.features.dtls) { + skip('DTLS is not enabled'); +} + +const { connect } = await import('node:dtls'); + +// SNI host names are limited to 255 bytes; a longer one must throw. +assert.throws( + () => connect('127.0.0.1', 12345, { + servername: 'a'.repeat(256), + rejectUnauthorized: false, + }), + { code: 'ERR_CRYPTO_OPERATION_FAILED' }, +); diff --git a/test/parallel/test-dtls-session-table-cleanup.mjs b/test/parallel/test-dtls-session-table-cleanup.mjs new file mode 100644 index 00000000000000..5e8ff89f5d6dde --- /dev/null +++ b/test/parallel/test-dtls-session-table-cleanup.mjs @@ -0,0 +1,81 @@ +// Flags: --experimental-dtls --no-warnings + +// Test: a session is removed from its endpoint's session table when it closes, +// whether closed locally or by the peer. Regression test for closed sessions +// leaking in the table (which also blocks reuse of the peer's address). + +import { hasCrypto, skip, mustCall } from '../common/index.mjs'; +import assert from 'node:assert'; +import * as fixtures from '../common/fixtures.mjs'; + +const { strictEqual } = assert; +const { readKey } = fixtures; + +if (!hasCrypto) { + skip('missing crypto'); +} + +if (!process.features.dtls) { + skip('DTLS is not enabled'); +} + +const { listen, connect } = await import('node:dtls'); + +const cert = readKey('agent1-cert.pem').toString(); +const key = readKey('agent1-key.pem').toString(); +const ca = readKey('ca1-cert.pem').toString(); + +// --------------------------------------------------------------------------- +// Case 1: the server closes its own session -> removed from the table. +{ + const gotSession = Promise.withResolvers(); + + const server = listen(mustCall((session) => { + gotSession.resolve(session); + }), { cert, key, port: 0, host: '127.0.0.1' }); + + const client = connect('127.0.0.1', server.address.port, { + ca: [ca], + rejectUnauthorized: false, + }); + + await client.opened; + const session = await gotSession.promise; + + strictEqual(server.state.sessionCount, 1); + await session.close(); + strictEqual(server.state.sessionCount, 0); + + await client.close(); + await server.close(); +} + +// --------------------------------------------------------------------------- +// Case 2: the client closes -> the server sees the peer close_notify and +// removes the session from the table. +{ + const gotSession = Promise.withResolvers(); + + const server = listen(mustCall((session) => { + gotSession.resolve(session); + }), { cert, key, port: 0, host: '127.0.0.1' }); + + const client = connect('127.0.0.1', server.address.port, { + ca: [ca], + rejectUnauthorized: false, + }); + + await client.opened; + const session = await gotSession.promise; + + strictEqual(server.state.sessionCount, 1); + await client.close(); // Sends close_notify to the server + await session.closed; // Server processes it and emits its close + // The session is dropped from the table immediately after its close callback + // runs (which can execute synchronously during the close emit's microtask + // drain); yield to the event loop so teardown settles before reading count. + await new Promise((resolve) => setImmediate(resolve)); + strictEqual(server.state.sessionCount, 0); + + await server.close(); +} diff --git a/test/parallel/test-dtls-unhandled-rejection.mjs b/test/parallel/test-dtls-unhandled-rejection.mjs new file mode 100644 index 00000000000000..721afee7d5982d --- /dev/null +++ b/test/parallel/test-dtls-unhandled-rejection.mjs @@ -0,0 +1,49 @@ +// Flags: --experimental-dtls --no-warnings + +// Test: using the callback style (onerror) without awaiting opened/closed must +// not produce an unhandled promise rejection when the session errors. + +import { hasCrypto, skip, mustCall, mustNotCall } from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; + +const { readKey } = fixtures; + +if (!hasCrypto) { + skip('missing crypto'); +} + +if (!process.features.dtls) { + skip('DTLS is not enabled'); +} + +// Any unhandled rejection fails the test. +process.on('unhandledRejection', mustNotCall()); + +const { listen, connect } = await import('node:dtls'); + +const cert = readKey('agent1-cert.pem').toString(); +const key = readKey('agent1-key.pem').toString(); +const ca = readKey('ca1-cert.pem').toString(); + +const server = listen(mustCall(), { + cert, key, port: 0, host: '127.0.0.1', +}); + +const errored = Promise.withResolvers(); + +// A verification failure makes connect() reject its opened promise. Handle the +// error only through the callback API and never touch opened/closed. +const session = connect('127.0.0.1', server.address.port, { + ca: [ca], + rejectUnauthorized: true, + servername: 'wrong.example.com', +}); +session.onerror = mustCall(() => errored.resolve()); + +await errored.promise; + +// Give the rejected (but internally handled) opened promise a chance to be +// reported as unhandled, if it were, before the process exits. +await new Promise((resolve) => setImmediate(resolve)); + +await server.close(); diff --git a/test/parallel/test-dtls-verify-identity.mjs b/test/parallel/test-dtls-verify-identity.mjs new file mode 100644 index 00000000000000..43d8c73a25678e --- /dev/null +++ b/test/parallel/test-dtls-verify-identity.mjs @@ -0,0 +1,86 @@ +// Flags: --experimental-dtls --no-warnings + +// Test: the DTLS client verifies the server certificate's identity (hostname +// or IP address) against the expected name, not merely that the certificate +// chains to a trusted CA. Also exercises that SNI is actually carried in the +// ClientHello (it is applied before the handshake begins). + +import { hasCrypto, skip, mustCall, mustNotCall } from '../common/index.mjs'; +import assert from 'node:assert'; +import * as fixtures from '../common/fixtures.mjs'; + +const { match, rejects, strictEqual } = assert; +const { readKey } = fixtures; + +if (!hasCrypto) { + skip('missing crypto'); +} + +if (!process.features.dtls) { + skip('DTLS is not enabled'); +} + +const { listen, connect } = await import('node:dtls'); + +// The agent1 certificate has CN=agent1 and no subjectAltName, so OpenSSL +// matches the identity "agent1" (via the subject CN) and rejects other names. +const cert = readKey('agent1-cert.pem').toString(); +const key = readKey('agent1-key.pem').toString(); +const ca = readKey('ca1-cert.pem').toString(); + +// --------------------------------------------------------------------------- +// Case 1: a matching servername with rejectUnauthorized succeeds, and the +// server observes the SNI the client sent (proving it is in the ClientHello). +{ + const sawServername = Promise.withResolvers(); + + const server = listen(mustCall((session) => { + session.onhandshake = mustCall(() => { + sawServername.resolve(session.servername); + }); + }), { cert, key, port: 0, host: '127.0.0.1' }); + + const { port } = server.address; + + const session = connect('127.0.0.1', port, { + ca: [ca], + rejectUnauthorized: true, + servername: 'agent1', + }); + + const { protocol } = await session.opened; + match(protocol, /DTLS/i); + + strictEqual(await sawServername.promise, 'agent1'); + + await session.close(); + await server.close(); +} + +// --------------------------------------------------------------------------- +// Case 2: a servername that does not match the certificate is rejected during +// the handshake, even though the certificate chain is otherwise valid. +{ + const server = listen(mustCall((session) => { + // The client aborts after verifying the certificate, so the server-side + // handshake never completes. + session.onhandshake = mustNotCall(); + // The server session's `opened` is never awaited here; ensure it can't + // surface as an unhandled rejection if the stalled handshake later errors. + session.opened.catch(() => {}); + }), { cert, key, port: 0, host: '127.0.0.1' }); + + const { port } = server.address; + + const session = connect('127.0.0.1', port, { + ca: [ca], + rejectUnauthorized: true, + servername: 'wrong.example.com', + }); + + await rejects(session.opened, /certificate verify failed/i); + + // Closing the session tears down the internally-owned client endpoint. + await session.close(); + await server.close(); +} diff --git a/test/parallel/test-permission-net-dtls.mjs b/test/parallel/test-permission-net-dtls.mjs index f2f2b7af2d2c3b..54c6235b3df1c4 100644 --- a/test/parallel/test-permission-net-dtls.mjs +++ b/test/parallel/test-permission-net-dtls.mjs @@ -51,9 +51,16 @@ const ca = readFileSync(join(fixturesDir, 'ca1-cert.pem')).toString(); ); } -// Test: Creating a DTLSEndpoint without connect/listen is allowed -// since no network I/O occurs at construction time. +// Test: Creating a DTLSEndpoint is allowed (no network I/O at construction +// time), but bind() requires net permission. { const endpoint = new DTLSEndpoint(); assert.ok(endpoint); + assert.throws( + () => endpoint.bind('127.0.0.1', 0), + { + code: 'ERR_ACCESS_DENIED', + permission: 'Net', + }, + ); } diff --git a/test/sequential/test-dtls-interop-openssl-client.mjs b/test/sequential/test-dtls-interop-openssl-client.mjs index 683d725e76698c..580cc0401a18e1 100644 --- a/test/sequential/test-dtls-interop-openssl-client.mjs +++ b/test/sequential/test-dtls-interop-openssl-client.mjs @@ -8,7 +8,7 @@ import { hasCrypto, skip, mustCall } from '../common/index.mjs'; import { createRequire } from 'module'; import assert from 'node:assert'; import { spawn } from 'node:child_process'; -import { setTimeout } from 'node:timers/promises'; +import { setTimeout as sleep } from 'node:timers/promises'; import * as fixtures from '../common/fixtures.mjs'; if (!hasCrypto) { @@ -73,13 +73,13 @@ const timeout = setTimeout(() => { // Wait for the handshake to start (s_client writes TLS info to stdout), // then send data. await new Promise((resolve) => client.stdout.once('data', resolve)); -await setTimeout(500); +await sleep(500); client.stdin.write('hello from openssl\n'); // Wait for the server to receive and reply. await serverReceivedData.promise; -await setTimeout(500); +await sleep(500); // Close stdin so s_client exits. client.stdin.end();