net: improve dtls cert verification

Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Claude/Opus
PR-URL: #64314
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Author: jasnell

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.

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;

src/dtls/dtls.h

@@ -58,16 +58,18 @@ void RecordTimestampStat(Stats* stats) {
   V(MESSAGES_SENT, messages_sent)                                              \
   V(RETRANSMIT_COUNT, retransmit_count)
 
-// State indices shared between C++ and JS via AliasedStruct/DataView.
+// State "indices" shared between C++ and JS via AliasedStruct/DataView. These
+// are BYTE OFFSETS into the state struct, not sequential indices: session_count
+// is a uint32, so `busy`, which follows it, sits at byte offset 8. The
+// static_asserts in dtls_endpoint.cc pin these to the actual struct layout.
 // Keep in sync with lib/internal/dtls/state.js.
 enum DTLSEndpointStateIndex {
   IDX_ENDPOINT_STATE_BOUND = 0,
-  IDX_ENDPOINT_STATE_LISTENING,
-  IDX_ENDPOINT_STATE_CLOSING,
-  IDX_ENDPOINT_STATE_DESTROYED,
-  IDX_ENDPOINT_STATE_SESSION_COUNT,
-  IDX_ENDPOINT_STATE_BUSY,
-  IDX_ENDPOINT_STATE_COUNT
+  IDX_ENDPOINT_STATE_LISTENING = 1,
+  IDX_ENDPOINT_STATE_CLOSING = 2,
+  IDX_ENDPOINT_STATE_DESTROYED = 3,
+  IDX_ENDPOINT_STATE_SESSION_COUNT = 4,
+  IDX_ENDPOINT_STATE_BUSY = 8,
 };
 
 enum DTLSSessionStateIndex {

src/dtls/dtls_context.cc

@@ -10,6 +10,7 @@
 #include <node_errors.h>
 #include <node_sockaddr-inl.h>
 #include <util-inl.h>
+#include <uv.h>
 
 #include <openssl/err.h>
 #include <openssl/evp.h>
@@ -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<Value>& 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<Value>& 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<DTLSContext*>(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<DTLSSession*>(app_data);
-    const sockaddr* sa = session->remote_address().data();
+    const sockaddr* sa =
+        static_cast<DTLSSession*>(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<unsigned char>((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,

src/dtls/dtls_context.h

@@ -57,6 +57,14 @@ class DTLSContext final : public BaseObject {
   static void LoadDefaultCAs(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void SetECDHCurve(const v8::FunctionCallbackInfo<v8::Value>& 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,