From 5954b9ec8ca0bc935dad98a36452dac403fb1350 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 26 Sep 2026 00:30:26 +0000 Subject: [PATCH 1/4] tls: report a fatal post-handshake SSL error over a Duplex or a named pipe SSLWrapper::handle_reading cleared the OpenSSL error queue right after a fatal SSL_read and closed with no error. node:tls over a generic Duplex (tls.connect({ socket })), over a Windows named pipe, and on the http2 raw-socket upgrade then ended cleanly where node emits the socket's ERR_SSL_ error. Take the error off the queue before it is cleared, send the fatal alert BoringSSL sealed into the write BIO, and report the error through the new Handlers::on_ssl_error into TLSSocket::on_ssl_error just before the close. TLSSocket::on_ssl_error calls the socket's error handler with a third argument, and node:net then emits the error and keeps the socket for the close that follows, like node's onerror. --- packages/bun-usockets/src/crypto/openssl.c | 14 ++ src/http/ProxyTunnel.rs | 1 + .../websocket_client/WebSocketProxyTunnel.rs | 1 + src/js/node/net.ts | 13 +- src/runtime/socket/UpgradedDuplex.rs | 10 ++ src/runtime/socket/WindowsNamedPipe.rs | 12 ++ src/runtime/socket/WindowsNamedPipeContext.rs | 8 ++ src/runtime/socket/socket_body.rs | 47 +++++++ src/uws/lib.rs | 42 +++++- .../js/node/http2/node-http2-upgrade.test.mts | 49 +++++++ test/js/node/tls/node-tls-connect.test.ts | 124 ++++++++++++++++++ test/js/node/tls/node-tls-namedpipes.test.ts | 75 +++++++++++ 12 files changed, 392 insertions(+), 4 deletions(-) diff --git a/packages/bun-usockets/src/crypto/openssl.c b/packages/bun-usockets/src/crypto/openssl.c index 529d81169e37..a25c59ad68d6 100644 --- a/packages/bun-usockets/src/crypto/openssl.c +++ b/packages/bun-usockets/src/crypto/openssl.c @@ -1796,6 +1796,20 @@ static long us_internal_verify_peer_certificate(const SSL *ssl, long def) { return err; } +/* The packed error that names a fatal SSL_read failure the way node reports + * it: the first SSL-library entry on the thread's queue (for a bad record + * BoringSSL queues the cipher's BAD_DECRYPT ahead of the TLS reason), else the + * oldest entry, else 0. Drains the queue up to the entry it returns. */ +uint32_t us_ssl_take_fatal_error(void) { + uint32_t oldest = ERR_peek_error(); + for (uint32_t queued; (queued = ERR_get_error()) != 0;) { + if (ERR_GET_LIB(queued) == ERR_LIB_SSL) { + return queued; + } + } + return oldest; +} + struct us_bun_verify_error_t us_ssl_socket_verify_error_from_ssl(SSL *ssl) { long x509_verify_error = us_internal_verify_peer_certificate(ssl, X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT); diff --git a/src/http/ProxyTunnel.rs b/src/http/ProxyTunnel.rs index abdb981d4541..75d2d04c75ad 100644 --- a/src/http/ProxyTunnel.rs +++ b/src/http/ProxyTunnel.rs @@ -610,6 +610,7 @@ impl ProxyTunnel { // opting out keeps its SSL off the parked queues entirely. on_session: None, on_keylog: None, + on_ssl_error: None, server_identity: Some(server_identity), ctx: this.as_erased_ptr().as_ptr(), }, diff --git a/src/http_jsc/websocket_client/WebSocketProxyTunnel.rs b/src/http_jsc/websocket_client/WebSocketProxyTunnel.rs index 38f1e22846a2..ae6866be52f2 100644 --- a/src/http_jsc/websocket_client/WebSocketProxyTunnel.rs +++ b/src/http_jsc/websocket_client/WebSocketProxyTunnel.rs @@ -188,6 +188,7 @@ impl WebSocketProxyTunnel { // SSL off the parked session/keylog queues entirely. on_session: None, on_keylog: None, + on_ssl_error: None, server_identity: Some(Self::server_identity), }, ) diff --git a/src/js/node/net.ts b/src/js/node/net.ts index 4900f52f83e0..8acc965ebbf5 100644 --- a/src/js/node/net.ts +++ b/src/js/node/net.ts @@ -1240,7 +1240,8 @@ const ServerHandlers = { reportError(err); } }, - error(socket, error) { + // See SocketHandlers2.error for `tlsFatal`. + error(socket, error, tlsFatal?: boolean) { const data = this.data; if (!data) return; @@ -1269,6 +1270,9 @@ const ServerHandlers = { ) { // Ignore server's authorization errors data.destroy(); + } else if (tlsFatal && !callback) { + // The native close that follows ends the socket, so emit it the way Node does. + data._emitTLSError(error); } else { // Node emits through _emitTLSError and leaves the socket alive. Bun // still destroys here: its tls.Server completes the handshake for a @@ -1596,7 +1600,8 @@ const SocketHandlers2 = { const { self } = socket.data; onClientHandshake(self, socket, success, verifyError); }, - error(socket, error) { + // `tlsFatal`: a fatal TLS error on the established session. The native close follows it. + error(socket, error, tlsFatal?: boolean) { $debug("Bun.Socket error"); if (socket.data === undefined) return; const { self } = socket.data; @@ -1607,6 +1612,10 @@ const SocketHandlers2 = { if (callback) { self[kwriteCallback] = null; callback(error); + } else if (tlsFatal && self._secureEstablished) { + // No destroy, like https://github.com/nodejs/node/blob/v26.3.0/lib/internal/tls/wrap.js#L467-L498 + self._emitTLSError(error); + return; } if (!self.destroyed) process.nextTick(destroyNT, self, error); diff --git a/src/runtime/socket/UpgradedDuplex.rs b/src/runtime/socket/UpgradedDuplex.rs index a25d5b648497..6f4517bf0729 100644 --- a/src/runtime/socket/UpgradedDuplex.rs +++ b/src/runtime/socket/UpgradedDuplex.rs @@ -103,6 +103,8 @@ pub(crate) struct Handlers { pub(crate) on_handshake: fn(*mut (), bool, us_bun_verify_error_t), pub(crate) on_data: fn(*mut (), &[u8]), pub on_close: fn(*mut ()), + /// See `ssl_wrapper::Handlers::on_ssl_error`. + pub(crate) on_ssl_error: fn(*mut (), u32), pub(crate) on_end: fn(*mut ()), pub(crate) on_writable: fn(*mut ()), pub(crate) on_error: fn(*mut (), JSValue), @@ -211,6 +213,13 @@ impl UpgradedDuplex { } } + fn on_ssl_error(this: *mut Self, err: u32) { + bun_output::scoped_log!(UpgradedDuplex, "onSslError"); + // SAFETY: see handler note above. + let this = unsafe { &*this }; + (this.handlers.on_ssl_error)(this.handlers.ctx, err); + } + fn on_close(this: *mut Self) { bun_output::scoped_log!(UpgradedDuplex, "onClose"); // SAFETY: see handler note above. @@ -492,6 +501,7 @@ impl UpgradedDuplex { on_handshake: Self::on_handshake, on_data: Self::on_data, on_close: Self::on_close, + on_ssl_error: Some(Self::on_ssl_error), write: Self::internal_write, on_session: Some(Self::on_session), on_keylog: Some(Self::on_keylog), diff --git a/src/runtime/socket/WindowsNamedPipe.rs b/src/runtime/socket/WindowsNamedPipe.rs index 1e7239863e4a..851093295597 100644 --- a/src/runtime/socket/WindowsNamedPipe.rs +++ b/src/runtime/socket/WindowsNamedPipe.rs @@ -136,6 +136,8 @@ pub(crate) struct Handlers { pub(crate) on_handshake: fn(*mut c_void, bool, us_bun_verify_error_t), pub(crate) on_data: fn(*mut c_void, &[u8]), pub on_close: fn(*mut c_void), + /// See `ssl_wrapper::Handlers::on_ssl_error`. + pub(crate) on_ssl_error: fn(*mut c_void, u32), pub(crate) on_end: fn(*mut c_void), pub(crate) on_writable: fn(*mut c_void), pub(crate) on_error: fn(*mut c_void, bun_sys::Error), @@ -362,6 +364,11 @@ impl WindowsNamedPipe { (self.handlers.on_keylog)(self.handlers.ctx, line); } + fn on_ssl_error(&self, err: u32) { + bun_output::scoped_log!(WindowsNamedPipe, "onSslError"); + (self.handlers.on_ssl_error)(self.handlers.ctx, err); + } + // ── SSLWrapper trampolines ─────────────────────────────────────────────── // `ssl_wrapper::Handlers<*mut Self>` carries `fn(*mut Self, ..)` slots. // SAFETY (all): `this` is the `ctx` set in `wrapper_handlers`; the engine @@ -394,6 +401,10 @@ impl WindowsNamedPipe { // SAFETY: see block note above. unsafe { &*this }.on_keylog(d) } + fn ssl_on_ssl_error(this: *mut Self, err: u32) { + // SAFETY: see block note above. + unsafe { &*this }.on_ssl_error(err) + } fn ssl_on_close(this: *mut Self) { // SAFETY: see block note above. unsafe { &*this }.on_close() @@ -411,6 +422,7 @@ impl WindowsNamedPipe { on_handshake: Self::ssl_on_handshake, on_data: Self::ssl_on_data, on_close: Self::ssl_on_close, + on_ssl_error: Some(Self::ssl_on_ssl_error), write: Self::ssl_write, on_session: Some(Self::ssl_on_session), on_keylog: Some(Self::ssl_on_keylog), diff --git a/src/runtime/socket/WindowsNamedPipeContext.rs b/src/runtime/socket/WindowsNamedPipeContext.rs index a26276be6aa3..cc33f18dd9ad 100644 --- a/src/runtime/socket/WindowsNamedPipeContext.rs +++ b/src/runtime/socket/WindowsNamedPipeContext.rs @@ -232,6 +232,13 @@ impl WindowsNamedPipeContext { } } + fn on_ssl_error(this: *mut Self, err: u32) { + // SAFETY: see `on_open`. + if let SocketType::Tls(s) = unsafe { (*this).socket } { + crate::dispatch::fold(TLSSocket::on_ssl_error(s, err)); + } + } + fn on_handshake(this: *mut Self, success: bool, ssl_error: us_bun_verify_error_t) { // SAFETY: see `on_open`. let (socket, pipe) = unsafe { ((*this).socket, ptr::addr_of_mut!((*this).named_pipe)) }; @@ -393,6 +400,7 @@ impl WindowsNamedPipeContext { on_error: |p, e| Self::on_error(p.cast::(), &e), on_timeout: |p| Self::on_timeout(p.cast::()), on_close: |p| Self::on_close(p.cast::()), + on_ssl_error: |p, err| Self::on_ssl_error(p.cast::(), err), on_session: |p, d| Self::on_session(p.cast::(), d), on_keylog: |p, d| Self::on_keylog(p.cast::(), d), server_identity: |p, ssl| Self::server_identity(p.cast::(), ssl), diff --git a/src/runtime/socket/socket_body.rs b/src/runtime/socket/socket_body.rs index d0de384d037d..54f510b89ca1 100644 --- a/src/runtime/socket/socket_body.rs +++ b/src/runtime/socket/socket_body.rs @@ -2142,6 +2142,43 @@ impl NewSocket { Ok(()) } + /// A fatal TLS error before the close. The third argument tells node:net that no handler threw it. + pub(crate) fn on_ssl_error(this: bun_ptr::ThisPtr, err_code: u32) -> JsResult<()> { + jsc::mark_binding!(); + if !this.has_handlers() || this.flags.get().contains(Flags::FINALIZING) { + return Ok(()); + } + if this.socket.get().is_detached() { + return Ok(()); + } + let handlers = this.get_handlers(); + if handlers.vm.script_execution_status() != jsc::ScriptExecutionStatus::Running { + return Ok(()); + } + let callback = handlers.on_error(); + if callback.is_empty() { + return Ok(()); + } + let global = handlers.global_object; + if global.has_exception() { + return Err(jsc::JsError::Thrown); + } + let _scope = ScopeExit { + socket: this, + scope: Some(handlers.enter()), + }; + let this_value = this.get_this_value(&global); + let err_value = boringssl_err_to_js(&global, err_code); + global.bun_vm().event_loop_mut().run_callback( + bun_event_loop::ContextId::NONE, + callback, + &global, + this_value, + &[this_value, err_value, JSValue::TRUE], + ); + Ok(()) + } + /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`. pub(crate) fn on_close( this: bun_ptr::ThisPtr, @@ -4484,6 +4521,12 @@ impl DuplexUpgradeContext { } } + fn on_ssl_error(this: bun_ptr::ThisPtr, err: u32) { + if let Some(tls) = this.tls_this_ptr() { + crate::dispatch::fold(TLSSocket::on_ssl_error(tls, err)); + } + } + fn on_close(this: bun_ptr::ThisPtr) { let socket = this.duplex_socket(); if let Some(tls) = this.tls.replace(None) { @@ -4880,6 +4923,10 @@ pub(crate) fn js_upgrade_duplex_to_tls( DuplexUpgradeContext::on_close(bun_ptr::ThisPtr::new(c.cast())) }, // SAFETY: `c` is `ctx` below — the live `DuplexUpgradeContext` heap allocation. + on_ssl_error: |c: *mut (), err| { + DuplexUpgradeContext::on_ssl_error(bun_ptr::ThisPtr::new(c.cast()), err) + }, + // SAFETY: `c` is `ctx` below — the live `DuplexUpgradeContext` heap allocation. on_end: |c: *mut ()| DuplexUpgradeContext::on_end(bun_ptr::ThisPtr::new(c.cast())), // SAFETY: `c` is `ctx` below — the live `DuplexUpgradeContext` heap allocation. on_writable: |c: *mut ()| { diff --git a/src/uws/lib.rs b/src/uws/lib.rs index 31ee23f5b94f..0dffc5e9813d 100644 --- a/src/uws/lib.rs +++ b/src/uws/lib.rs @@ -406,6 +406,9 @@ pub mod ssl_wrapper { pub write: fn(T, &[u8]), pub on_data: fn(T, &[u8]), pub on_close: fn(T), + /// A fatal `SSL_read` failure (a packed BoringSSL error), reported just + /// before `on_close`. `None`: the owner only needs the close. + pub on_ssl_error: Option, /// A new resumable TLS session arrived (serialized SSL_SESSION bytes) /// - node's `'session'` event. `None` opts the SSL out of session /// parking entirely (fetch / WebSocket tunnels have no consumer). @@ -979,6 +982,16 @@ pub mod ssl_wrapper { (handlers.on_close)(handlers.ctx); } + fn trigger_ssl_error_callback(&self, err: u32) { + if self.flags.closed_notified() { + return; + } + let handlers = self.handlers.get(); + if let Some(on_ssl_error) = handlers.on_ssl_error { + on_ssl_error(handlers.ctx, err); + } + } + /// The SSL's X509 verdict. Shutdown state does not change it. fn verify_error(&self) -> us_bun_verify_error_t { let Some(ssl) = self.ssl.get() else { @@ -1140,6 +1153,14 @@ pub mod ssl_wrapper { if just_read <= 0 { // SAFETY: ssl is still valid. let err = unsafe { boring_sys::SSL_get_error(ssl.as_ptr(), just_read) }; + let is_fatal = + err == boring_sys::SSL_ERROR_SSL || err == boring_sys::SSL_ERROR_SYSCALL; + // Take the error before the queue is cleared. + let fatal_error = if is_fatal { + us_ssl_take_fatal_error() + } else { + 0 + }; boring_sys::ERR_clear_error(); if err != boring_sys::SSL_ERROR_WANT_READ @@ -1186,8 +1207,7 @@ pub mod ssl_wrapper { self.flags.set_received_ssl_shutdown(true); self.handle_end_of_renegotiation(); } - if err == boring_sys::SSL_ERROR_SSL || err == boring_sys::SSL_ERROR_SYSCALL - { + if is_fatal { self.flags.set_fatal_error(true); } @@ -1201,6 +1221,14 @@ pub mod ssl_wrapper { return false; } } + if is_fatal { + // Send the alert BoringSSL sealed into the write BIO + // before the close frees it, so the peer gets its error. + self.handle_writing(buffer); + if self.ssl.get().is_none() || self.flags.closed_notified() { + return false; + } + } // A NewSessionTicket/keylog line that rode in ahead of the // peer's close_notify is still parked; deliver it before the // close tears the wrapper down (mirrors the C ZERO_RETURN path). @@ -1208,6 +1236,13 @@ pub mod ssl_wrapper { if self.ssl.get().is_none() || self.flags.closed_notified() { return false; } + if fatal_error != 0 { + // Like node's ClearOut: the error goes to the owner, then the close. + self.trigger_ssl_error_callback(fatal_error); + if self.ssl.get().is_none() || self.flags.closed_notified() { + return false; + } + } if err == boring_sys::SSL_ERROR_ZERO_RETURN { // 2-step shutdown, last: write_data fails once our close_notify is out. let _ = self.shutdown(false); @@ -1500,6 +1535,9 @@ pub mod ssl_wrapper { /// Implemented in uSockets C; reads /// `SSL_get_verify_result` and maps it onto the C `us_bun_verify_error_t`. fn us_ssl_socket_verify_error_from_ssl(ssl: *mut boring_sys::SSL) -> us_bun_verify_error_t; + /// openssl.c: the packed error that names a fatal `SSL_read` failure, taken off the thread's error queue (0 if none). + // safe: no args; reads the calling thread's own queue. + safe fn us_ssl_take_fatal_error() -> u32; fn SSL_SESSION_up_ref(session: *mut boring_sys::SSL_SESSION) -> c_int; /// openssl.c: 1 when the verify step of this handshake asked the owner for the server's name. fn us_ssl_identity_checked(ssl: *mut boring_sys::SSL) -> c_int; diff --git a/test/js/node/http2/node-http2-upgrade.test.mts b/test/js/node/http2/node-http2-upgrade.test.mts index 57a8b73d93bc..4c9bc767a58e 100644 --- a/test/js/node/http2/node-http2-upgrade.test.mts +++ b/test/js/node/http2/node-http2-upgrade.test.mts @@ -461,6 +461,55 @@ describe("HTTP/2 upgrade — server TLS options", () => { }); }); +describe("HTTP/2 upgrade — fatal TLS error after the handshake", () => { + test("a record that fails to decrypt reaches the server as sessionError", async () => { + const h2Server = http2.createSecureServer(TLS, (_req, res) => { + res.writeHead(200); + res.end("ok"); + }); + h2Server.on("error", () => {}); + // A clean session 'close' with no error before it is the bug. + const outcome = new Promise<{ event: string; code?: string }>(resolve => { + h2Server.on("sessionError", (err: NodeJS.ErrnoException) => resolve({ event: "sessionError", code: err.code })); + h2Server.on("session", session => session.on("close", () => resolve({ event: "close" }))); + }); + const netServer = net.createServer(socket => { + socket.on("error", () => {}); + h2Server.emit("connection", socket); + }); + // A plain proxy in front of the net.Server, to inject bytes toward it. + let toServer: net.Socket | undefined; + const proxy = net.createServer(c => { + toServer = net.connect((netServer.address() as net.AddressInfo).port, "127.0.0.1"); + c.pipe(toServer); + toServer.pipe(c); + c.on("error", () => {}); + toServer.on("error", () => {}); + }); + let client: http2.ClientHttp2Session | undefined; + try { + await once(netServer.listen(0, "127.0.0.1"), "listening"); + await once(proxy.listen(0, "127.0.0.1"), "listening"); + client = connectClient((proxy.address() as net.AddressInfo).port); + const first = await request(client, "GET", "/"); + assert.strictEqual(first.status, 200); + + // application_data, 32 bytes of ciphertext that cannot authenticate. + toServer!.write(Buffer.concat([Buffer.from([0x17, 0x03, 0x03, 0x00, 0x20]), Buffer.alloc(32, 0x42)])); + + assert.deepStrictEqual(await outcome, { + event: "sessionError", + code: "ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", + }); + } finally { + client?.destroy(); + toServer?.destroy(); + proxy.close(); + netServer.close(); + } + }); +}); + if (typeof Bun !== "undefined") { describe("Node.js compatibility", () => { test("tests should run on node.js", async () => { diff --git a/test/js/node/tls/node-tls-connect.test.ts b/test/js/node/tls/node-tls-connect.test.ts index 532fc746cbae..13e75cf9f28a 100644 --- a/test/js/node/tls/node-tls-connect.test.ts +++ b/test/js/node/tls/node-tls-connect.test.ts @@ -586,6 +586,130 @@ for (const { name, connect } of tests) { }); } +// A TLS record that fails after the handshake is a fatal protocol error. Node +// surfaces it as the socket's ERR_SSL_ 'error' (and leaves the socket +// open). Over a generic Duplex the TLS engine is SSLWrapper, a separate +// SSL_read driver from the uSockets one, so it gets its own coverage. A TCP +// proxy between the Duplex and the server injects the bytes once the +// handshake has completed on both sides. Every assertion here also holds on +// Node.js; only the alert's code name depends on the SSL library. +describe("tls.connect over a Duplex reports a fatal post-handshake SSL error", () => { + // application_data, legacy version TLS 1.2, 32 bytes of ciphertext that + // cannot authenticate: the receiver fails the AEAD open and alerts + // bad_record_mac. + const BAD_RECORD = Buffer.concat([Buffer.from([0x17, 0x03, 0x03, 0x00, 0x20]), Buffer.alloc(32, 0x42)]); + // BoringSSL names the bad_record_mac alert SSLV3_ALERT_BAD_RECORD_MAC, + // OpenSSL 3 names it SSL/TLS_ALERT_BAD_RECORD_MAC. + const ALERT_BAD_RECORD_MAC = (process.features as { openssl_is_boringssl?: boolean }).openssl_is_boringssl + ? "ERR_SSL_SSLV3_ALERT_BAD_RECORD_MAC" + : "ERR_SSL_SSL/TLS_ALERT_BAD_RECORD_MAC"; + + type Scenario = { + toClient: net.Socket; + toServer: net.Socket; + serverSocket: TLSSocket; + client: TLSSocket; + // Append `bytes` to the next chunk the proxy forwards from the server. + appendToNextServerChunk(bytes: Buffer): void; + }; + + async function run(inject: (s: Scenario) => void | Promise) { + let toClient: net.Socket | undefined; + let toServer: net.Socket | undefined; + let raw: net.Socket | undefined; + let client: TLSSocket | undefined; + let serverSocket: TLSSocket | undefined; + let appendOnce: Buffer | undefined; + const server = tls.createServer(COMMON_CERT_); + server.on("secureConnection", s => s.on("error", () => {})); + const proxy = net.createServer(c => { + toClient = c; + toServer = net.connect((server.address() as AddressInfo).port, "127.0.0.1"); + c.pipe(toServer); + toServer.on("data", chunk => { + if (appendOnce) { + chunk = Buffer.concat([chunk, appendOnce]); + appendOnce = undefined; + } + c.write(chunk); + }); + toServer.on("end", () => c.end()); + c.on("error", () => {}); + toServer.on("error", () => {}); + }); + try { + await once(server.listen(0, "127.0.0.1"), "listening"); + const serverSecure = once(server, "secureConnection") as Promise<[TLSSocket]>; + await once(proxy.listen(0, "127.0.0.1"), "listening"); + + raw = net.connect((proxy.address() as AddressInfo).port, "127.0.0.1"); + await once(raw, "connect"); + client = tls.connect({ socket: new SocketProxy(raw), rejectUnauthorized: false }); + // Settle on whichever comes first: the 'error' (expected), or a 'close' + // with no 'error' before it (the bug). Node emits no 'close' at all here. + const outcome = new Promise<{ event: string; code?: string; library?: string }>(resolve => { + client!.once("error", (err: NodeJS.ErrnoException & { library?: string }) => + resolve({ event: "error", code: err.code, library: err.library }), + ); + client!.once("close", () => resolve({ event: "close" })); + }); + await once(client, "secureConnect"); + [serverSocket] = await serverSecure; + + await inject({ + toClient: toClient!, + toServer: toServer!, + serverSocket, + client, + appendToNextServerChunk: bytes => (appendOnce = bytes), + }); + return await outcome; + } finally { + for (const s of [client, raw, serverSocket, toClient, toServer]) s?.destroy(); + proxy.close(); + server.close(); + } + } + + it("a record that fails to decrypt surfaces as ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", async () => { + const result = await run(({ toClient }) => void toClient.write(BAD_RECORD)); + expect(result).toEqual({ + event: "error", + code: "ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", + library: "SSL routines", + }); + }); + + it("the peer's bad_record_mac alert surfaces as an ERR_SSL_*_ALERT_BAD_RECORD_MAC error", async () => { + // The bad record goes to the server, whose SSL_read fails and sends a + // bad_record_mac alert back. The client's SSL_read fails on that alert. + const result = await run(({ toServer }) => void toServer.write(BAD_RECORD)); + expect(result).toEqual({ + event: "error", + code: ALERT_BAD_RECORD_MAC, + library: "SSL routines", + }); + }); + + it("a 'data' listener that writes back does not hide the error", async () => { + // Good data and the bad record arrive in one chunk. The engine delivers + // the data first, the listener's write hits the now-fatal SSL, and the + // read's reason must still be the error that surfaces. + const result = await run(async ({ serverSocket, client, appendToNextServerChunk }) => { + client.on("data", () => client.write("back")); + serverSocket.write("first"); + await once(client, "data"); + appendToNextServerChunk(BAD_RECORD); + serverSocket.write("second"); + }); + expect(result).toEqual({ + event: "error", + code: "ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", + library: "SSL routines", + }); + }); +}); + it("setSession() should not leak the SSL_SESSION returned by d2i_SSL_SESSION", async () => { // d2i_SSL_SESSION returns an owned SSL_SESSION; SSL_set_session takes its own // reference ("the caller retains ownership"), so the caller's reference must diff --git a/test/js/node/tls/node-tls-namedpipes.test.ts b/test/js/node/tls/node-tls-namedpipes.test.ts index 674c39ddac01..95dafaa831de 100644 --- a/test/js/node/tls/node-tls-namedpipes.test.ts +++ b/test/js/node/tls/node-tls-namedpipes.test.ts @@ -206,3 +206,78 @@ it.if(isWindows)("should be able to upgrade a named pipe connection to TLS", asy await test(`\\\\.\\pipe\\test\\${randomUUID()}`); await expectMaxObjectTypeCount(expect, "TLSSocket", 3); }); + +// Same contract as "tls.connect over a Duplex reports a fatal post-handshake +// SSL error" in node-tls-connect.test.ts, with both TLS peers on a named pipe. +// A plain pipe proxy between them injects a record that cannot authenticate +// once the handshake has completed on both sides. +describe("a fatal post-handshake SSL error over a named pipe", () => { + const BAD_RECORD = Buffer.concat([Buffer.from([0x17, 0x03, 0x03, 0x00, 0x20]), Buffer.alloc(32, 0x42)]); + // BoringSSL and OpenSSL 3 name the bad_record_mac alert differently. + const ALERT_BAD_RECORD_MAC = (process.features as { openssl_is_boringssl?: boolean }).openssl_is_boringssl + ? "ERR_SSL_SSLV3_ALERT_BAD_RECORD_MAC" + : "ERR_SSL_SSL/TLS_ALERT_BAD_RECORD_MAC"; + + type Outcome = { event: string; code?: string; library?: string }; + // Settles on the 'error' (expected), or on a 'close' with no 'error' before it (the bug). + function firstErrorOrClose(socket: net.Socket): Promise { + return new Promise(resolve => { + socket.once("error", (err: NodeJS.ErrnoException & { library?: string }) => + resolve({ event: "error", code: err.code, library: err.library }), + ); + socket.once("close", () => resolve({ event: "close" })); + }); + } + + async function run(inject: (toClient: net.Socket, toServer: net.Socket) => void) { + let toClient: net.Socket | undefined; + let toServer: net.Socket | undefined; + let client: ReturnType | undefined; + let serverSocket: ReturnType | undefined; + const serverPipe = `\\\\.\\pipe\\test\\${randomUUID()}`; + const proxyPipe = `\\\\.\\pipe\\test\\${randomUUID()}`; + const server = createServer(tls); + const serverOutcome = Promise.withResolvers(); + server.on("secureConnection", s => firstErrorOrClose(s).then(serverOutcome.resolve)); + const proxy = net.createServer(c => { + toClient = c; + toServer = net.connect(serverPipe); + c.pipe(toServer); + toServer.pipe(c); + c.on("error", () => {}); + toServer.on("error", () => {}); + }); + try { + await once(server.listen(serverPipe), "listening"); + const serverSecure = once(server, "secureConnection"); + await once(proxy.listen(proxyPipe), "listening"); + + client = connect({ path: proxyPipe, rejectUnauthorized: false }); + const clientOutcome = firstErrorOrClose(client); + await once(client, "secureConnect"); + [serverSocket] = await serverSecure; + + inject(toClient!, toServer!); + return { client: await clientOutcome, server: await serverOutcome.promise }; + } finally { + for (const s of [client, serverSocket, toClient, toServer]) s?.destroy(); + proxy.close(); + server.close(); + } + } + + it.if(isWindows)("a record that fails to decrypt on the client", async () => { + expect(await run(toClient => void toClient.write(BAD_RECORD))).toEqual({ + client: { event: "error", code: "ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", library: "SSL routines" }, + // The client's bad_record_mac alert reaches the server as its own error. + server: { event: "error", code: ALERT_BAD_RECORD_MAC, library: "SSL routines" }, + }); + }); + + it.if(isWindows)("a record that fails to decrypt on the server", async () => { + expect(await run((_toClient, toServer) => void toServer.write(BAD_RECORD))).toEqual({ + client: { event: "error", code: ALERT_BAD_RECORD_MAC, library: "SSL routines" }, + server: { event: "error", code: "ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", library: "SSL routines" }, + }); + }); +}); From 7a4088ed62b766633825e070607202e7ab1e10e3 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 26 Sep 2026 19:07:35 +0000 Subject: [PATCH 2/4] test: assert the event order on both TLS engines, cover a server socket over a Duplex --- test/js/node/tls/node-tls-connect.test.ts | 283 +++++++++++++--------- 1 file changed, 171 insertions(+), 112 deletions(-) diff --git a/test/js/node/tls/node-tls-connect.test.ts b/test/js/node/tls/node-tls-connect.test.ts index 13e75cf9f28a..9542eb7a8e6a 100644 --- a/test/js/node/tls/node-tls-connect.test.ts +++ b/test/js/node/tls/node-tls-connect.test.ts @@ -211,8 +211,155 @@ it("should be able to grab the JSStreamSocket constructor", () => { //@ts-ignore expect(socket._handle._parentWrap.constructor).toBeFunction(); }); +// A TLS record that fails after the handshake is a fatal protocol error. Node +// reports it as the socket's ERR_SSL_ 'error' and keeps the socket, so +// a clean 'end' follows when the connection goes away. The cases that use +// these helpers put a TCP proxy between the two TLS peers and inject a record +// that cannot authenticate once both handshakes are done. Every assertion also +// holds on Node.js; only the alert's code name depends on the SSL library. + +// application_data, legacy version TLS 1.2, 32 bytes of ciphertext that cannot +// authenticate: the receiver fails the AEAD open and alerts bad_record_mac. +const BAD_RECORD = Buffer.concat([Buffer.from([0x17, 0x03, 0x03, 0x00, 0x20]), Buffer.alloc(32, 0x42)]); +// BoringSSL and OpenSSL 3 name the bad_record_mac alert differently. +const ALERT_BAD_RECORD_MAC = (process.features as { openssl_is_boringssl?: boolean }).openssl_is_boringssl + ? "ERR_SSL_SSLV3_ALERT_BAD_RECORD_MAC" + : "ERR_SSL_SSL/TLS_ALERT_BAD_RECORD_MAC"; + +type FaultedPeers = { + client: TLSSocket; + server: TLSSocket; + toClient: net.Socket; + toServer: net.Socket; + // Append `bytes` to the next chunk the proxy forwards from the server. + appendToNextServerChunk(bytes: Buffer): void; +}; + +async function withFaultProxy( + engines: { connect: typeof tlsConnect; serverOverDuplex?: boolean }, + body: (peers: FaultedPeers) => Promise, +): Promise { + const sockets: (net.Socket | undefined)[] = []; + const servers: net.Server[] = []; + try { + const server = tls.createServer(COMMON_CERT_); + servers.push(server); + const accepted = once(server, "secureConnection") as Promise<[TLSSocket]>; + let upstream: net.Server = server; + if (engines.serverOverDuplex) { + // This server never listens. A TCP server hands it each connection as a generic Duplex. + upstream = net.createServer(raw => { + sockets.push(raw); + server.emit("connection", new SocketProxy(raw)); + }); + servers.push(upstream); + } + await once(upstream.listen(0, "127.0.0.1"), "listening"); + + let toClient: net.Socket | undefined; + let toServer: net.Socket | undefined; + let appendOnce: Buffer | undefined; + const proxy = net.createServer(c => { + toClient = c; + toServer = net.connect((upstream.address() as AddressInfo).port, "127.0.0.1"); + sockets.push(c, toServer); + c.on("data", chunk => toServer!.write(chunk)); + toServer.on("data", chunk => { + if (appendOnce) { + chunk = Buffer.concat([chunk, appendOnce]); + appendOnce = undefined; + } + c.write(chunk); + }); + // However one side goes away, the other side gets a clean FIN. + c.on("close", () => toServer!.end()); + toServer.on("close", () => c.end()); + c.on("error", () => {}); + toServer.on("error", () => {}); + }); + servers.push(proxy); + await once(proxy.listen(0, "127.0.0.1"), "listening"); + + const client = engines.connect({ + host: "127.0.0.1", + port: (proxy.address() as AddressInfo).port, + servername: "localhost", + rejectUnauthorized: false, + }); + sockets.push(client); + await once(client, "secureConnect"); + const [accepted_] = await accepted; + sockets.push(accepted_); + return await body({ + client, + server: accepted_, + toClient: toClient!, + toServer: toServer!, + appendToNextServerChunk: bytes => (appendOnce = bytes), + }); + } finally { + for (const socket of sockets) socket?.destroy(); + for (const server of servers) server.close(); + } +} + +// The 'error', 'end' and 'close' events of `socket`, in order, up to the first +// 'end' or 'close'. `peer` goes away once it has seen the fault, which is what +// ends `socket` on Node. +function faultEvents(socket: TLSSocket, peer: TLSSocket) { + const events: string[] = []; + let library: string | undefined; + const { promise: settled, resolve } = Promise.withResolvers(); + socket.on("error", (err: NodeJS.ErrnoException & { library?: string }) => { + events.push(`error ${err.code}`); + library = err.library; + }); + socket.on("end", () => { + events.push("end"); + resolve(); + }); + socket.on("close", hadError => { + events.push(`close ${hadError}`); + resolve(); + }); + socket.resume(); + peer.on("error", () => peer.destroy()); + peer.resume(); + return settled.then(() => ({ events: [...events], library })); +} + for (const { name, connect } of tests) { describe(name, () => { + // The uSockets engine (plain tls.connect) does not report these yet. Its rows + // are marked failing until it does. + const itReported = connect === tlsConnect ? it.failing : it; + + itReported("a record that fails to decrypt after the handshake is the socket's ERR_SSL_* error", async () => { + const result = await withFaultProxy({ connect }, ({ client, server, toClient }) => { + const events = faultEvents(client, server); + toClient.write(BAD_RECORD); + return events; + }); + expect(result).toEqual({ + events: ["error ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", "end"], + library: "SSL routines", + }); + }); + + itReported("the peer's fatal alert after the handshake is the socket's ERR_SSL_* error", async () => { + // The bad record goes to the server, whose SSL_read fails and sends a + // bad_record_mac alert back. The client's SSL_read fails on that alert. + const result = await withFaultProxy({ connect }, ({ client, server, toServer }) => { + const events = faultEvents(client, server); + toServer.write(BAD_RECORD); + return events; + }); + expect(result).toEqual({ + events: [`error ${ALERT_BAD_RECORD_MAC}`, "end"], + library: "SSL routines", + }); + }); + itNetwork("should work with alpnProtocols", done => { try { let socket: TLSSocket | null = connect({ @@ -586,125 +733,37 @@ for (const { name, connect } of tests) { }); } -// A TLS record that fails after the handshake is a fatal protocol error. Node -// surfaces it as the socket's ERR_SSL_ 'error' (and leaves the socket -// open). Over a generic Duplex the TLS engine is SSLWrapper, a separate -// SSL_read driver from the uSockets one, so it gets its own coverage. A TCP -// proxy between the Duplex and the server injects the bytes once the -// handshake has completed on both sides. Every assertion here also holds on -// Node.js; only the alert's code name depends on the SSL library. -describe("tls.connect over a Duplex reports a fatal post-handshake SSL error", () => { - // application_data, legacy version TLS 1.2, 32 bytes of ciphertext that - // cannot authenticate: the receiver fails the AEAD open and alerts - // bad_record_mac. - const BAD_RECORD = Buffer.concat([Buffer.from([0x17, 0x03, 0x03, 0x00, 0x20]), Buffer.alloc(32, 0x42)]); - // BoringSSL names the bad_record_mac alert SSLV3_ALERT_BAD_RECORD_MAC, - // OpenSSL 3 names it SSL/TLS_ALERT_BAD_RECORD_MAC. - const ALERT_BAD_RECORD_MAC = (process.features as { openssl_is_boringssl?: boolean }).openssl_is_boringssl - ? "ERR_SSL_SSLV3_ALERT_BAD_RECORD_MAC" - : "ERR_SSL_SSL/TLS_ALERT_BAD_RECORD_MAC"; - - type Scenario = { - toClient: net.Socket; - toServer: net.Socket; - serverSocket: TLSSocket; - client: TLSSocket; - // Append `bytes` to the next chunk the proxy forwards from the server. - appendToNextServerChunk(bytes: Buffer): void; - }; - - async function run(inject: (s: Scenario) => void | Promise) { - let toClient: net.Socket | undefined; - let toServer: net.Socket | undefined; - let raw: net.Socket | undefined; - let client: TLSSocket | undefined; - let serverSocket: TLSSocket | undefined; - let appendOnce: Buffer | undefined; - const server = tls.createServer(COMMON_CERT_); - server.on("secureConnection", s => s.on("error", () => {})); - const proxy = net.createServer(c => { - toClient = c; - toServer = net.connect((server.address() as AddressInfo).port, "127.0.0.1"); - c.pipe(toServer); - toServer.on("data", chunk => { - if (appendOnce) { - chunk = Buffer.concat([chunk, appendOnce]); - appendOnce = undefined; - } - c.write(chunk); - }); - toServer.on("end", () => c.end()); - c.on("error", () => {}); - toServer.on("error", () => {}); - }); - try { - await once(server.listen(0, "127.0.0.1"), "listening"); - const serverSecure = once(server, "secureConnection") as Promise<[TLSSocket]>; - await once(proxy.listen(0, "127.0.0.1"), "listening"); - - raw = net.connect((proxy.address() as AddressInfo).port, "127.0.0.1"); - await once(raw, "connect"); - client = tls.connect({ socket: new SocketProxy(raw), rejectUnauthorized: false }); - // Settle on whichever comes first: the 'error' (expected), or a 'close' - // with no 'error' before it (the bug). Node emits no 'close' at all here. - const outcome = new Promise<{ event: string; code?: string; library?: string }>(resolve => { - client!.once("error", (err: NodeJS.ErrnoException & { library?: string }) => - resolve({ event: "error", code: err.code, library: err.library }), - ); - client!.once("close", () => resolve({ event: "close" })); - }); - await once(client, "secureConnect"); - [serverSocket] = await serverSecure; - - await inject({ - toClient: toClient!, - toServer: toServer!, - serverSocket, - client, - appendToNextServerChunk: bytes => (appendOnce = bytes), - }); - return await outcome; - } finally { - for (const s of [client, raw, serverSocket, toClient, toServer]) s?.destroy(); - proxy.close(); - server.close(); - } - } - - it("a record that fails to decrypt surfaces as ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", async () => { - const result = await run(({ toClient }) => void toClient.write(BAD_RECORD)); - expect(result).toEqual({ - event: "error", - code: "ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", - library: "SSL routines", - }); - }); - - it("the peer's bad_record_mac alert surfaces as an ERR_SSL_*_ALERT_BAD_RECORD_MAC error", async () => { - // The bad record goes to the server, whose SSL_read fails and sends a - // bad_record_mac alert back. The client's SSL_read fails on that alert. - const result = await run(({ toServer }) => void toServer.write(BAD_RECORD)); - expect(result).toEqual({ - event: "error", - code: ALERT_BAD_RECORD_MAC, - library: "SSL routines", - }); - }); - - it("a 'data' listener that writes back does not hide the error", async () => { +describe("a fatal post-handshake SSL error over a Duplex", () => { + it("is still reported when a 'data' listener writes back", async () => { // Good data and the bad record arrive in one chunk. The engine delivers // the data first, the listener's write hits the now-fatal SSL, and the - // read's reason must still be the error that surfaces. - const result = await run(async ({ serverSocket, client, appendToNextServerChunk }) => { + // read's error must still be the one that surfaces. + const result = await withFaultProxy({ connect: duplexProxy }, async peers => { + const { client, server, appendToNextServerChunk } = peers; + const events = faultEvents(client, server); client.on("data", () => client.write("back")); - serverSocket.write("first"); + server.write("first"); await once(client, "data"); appendToNextServerChunk(BAD_RECORD); - serverSocket.write("second"); + server.write("second"); + return events; + }); + // What follows the error depends on what the runtime does with the write. + expect({ first: result.events[0], library: result.library }).toEqual({ + first: "error ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", + library: "SSL routines", + }); + }); + + it("is reported on a server socket", async () => { + const result = await withFaultProxy({ connect: tlsConnect, serverOverDuplex: true }, peers => { + const { client, server, toServer } = peers; + const events = faultEvents(server, client); + toServer.write(BAD_RECORD); + return events; }); expect(result).toEqual({ - event: "error", - code: "ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", + events: ["error ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", "end"], library: "SSL routines", }); }); From da0f51d640b2217f1cfc5d0d4987f537c8268d51 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 26 Sep 2026 21:09:08 +0000 Subject: [PATCH 3/4] test: end the peer after the socket under test reports its error --- test/js/node/tls/node-tls-connect.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/js/node/tls/node-tls-connect.test.ts b/test/js/node/tls/node-tls-connect.test.ts index 9542eb7a8e6a..0877d2eca947 100644 --- a/test/js/node/tls/node-tls-connect.test.ts +++ b/test/js/node/tls/node-tls-connect.test.ts @@ -304,8 +304,8 @@ async function withFaultProxy( } // The 'error', 'end' and 'close' events of `socket`, in order, up to the first -// 'end' or 'close'. `peer` goes away once it has seen the fault, which is what -// ends `socket` on Node. +// 'end' or 'close'. Node keeps `socket` open after the error, so `peer` ends +// then: its FIN is what ends `socket` there. function faultEvents(socket: TLSSocket, peer: TLSSocket) { const events: string[] = []; let library: string | undefined; @@ -313,6 +313,7 @@ function faultEvents(socket: TLSSocket, peer: TLSSocket) { socket.on("error", (err: NodeJS.ErrnoException & { library?: string }) => { events.push(`error ${err.code}`); library = err.library; + peer.end(); }); socket.on("end", () => { events.push("end"); @@ -323,7 +324,7 @@ function faultEvents(socket: TLSSocket, peer: TLSSocket) { resolve(); }); socket.resume(); - peer.on("error", () => peer.destroy()); + peer.on("error", () => {}); peer.resume(); return settled.then(() => ({ events: [...events], library })); } From 6c43aed8f7530baa5ea523408274f8b85530a0d1 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 26 Sep 2026 22:30:35 +0000 Subject: [PATCH 4/4] tls: clear the error queue before each SSL_read in SSLWrapper SSL_get_error and the error taken after a fatal read both need a queue that holds only this read's entries. openssl.c clears it at the start of us_internal_ssl_on_data for the same reason. --- src/uws/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/uws/lib.rs b/src/uws/lib.rs index 0dffc5e9813d..d4631876dfac 100644 --- a/src/uws/lib.rs +++ b/src/uws/lib.rs @@ -1141,6 +1141,8 @@ pub mod ssl_wrapper { // SAFETY: write-only view of the unfilled tail; SSL_read only stores into it. let available = unsafe { &mut buffer.as_bytes_mut()[read..] }; + // An entry another operation on this thread left must not be taken for this read's. + boring_sys::ERR_clear_error(); // SAFETY: ssl is a live SSL*; available is a valid mutable slice. let just_read = unsafe { boring_sys::SSL_read(