diff --git a/packages/bun-usockets/src/crypto/openssl.c b/packages/bun-usockets/src/crypto/openssl.c index c8528b0016e2..ce6828e93196 100644 --- a/packages/bun-usockets/src/crypto/openssl.c +++ b/packages/bun-usockets/src/crypto/openssl.c @@ -1903,7 +1903,43 @@ struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, i } if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) { + /* ssl_park_fatal_reason handles the handshake-pending side and always + * clears the queue, so a completed handshake must capture the reason + * on the stack first and dispatch it right here instead. */ + char reason[US_SSL_FATAL_ERROR_REASON_MAX]; + reason[0] = 0; + if (s->ssl_handshake_state == HANDSHAKE_COMPLETED) { + unsigned long ssl_queue_err = ERR_peek_last_error(); + if (ssl_queue_err != 0) { + ERR_error_string_n(ssl_queue_err, reason, sizeof(reason)); + } + } ssl_park_fatal_reason(s); + if (reason[0]) { + /* The SSL library failed once the handshake was already done: a + * received fatal alert (TLS 1.3 delivers the server's mTLS + * rejection here, because the client finished one flight earlier) + * or a protocol violation such as a bad record MAC. Node reports + * these through TLSWrap's onerror; dropping them leaves the peer's + * "you are not authenticated" indistinguishable from a clean + * end-of-connection. */ + /* Deliver what this read already decrypted first: a peer can put + * application data and the fatal record in one segment, and both + * the ZERO_RETURN sibling above and Node's ClearOut hand the + * plaintext to the consumer before reporting. */ + ssl_flush_pending_session(s); + ssl_flush_pending_keylog(s); + if (ssl_gone(s)) return NULL; + if (read) { + s = us_dispatch_data(s, loop_ssl_data->ssl_read_output + LIBUS_RECV_BUFFER_PADDING, read); + if (!s || ssl_gone(s)) return NULL; + } + struct us_bun_verify_error_t verify_error = { + .error = -71, .code = "EPROTO", .reason = reason}; + us_dispatch_ssl_error(s, verify_error); + /* The JS error handler may have destroyed the socket. */ + if (ssl_gone(s)) return NULL; + } } ssl_close(s, 0, NULL); loop_ssl_data->ssl_last_fatal_error[0] = 0; diff --git a/packages/bun-usockets/src/internal/internal.h b/packages/bun-usockets/src/internal/internal.h index a6bace29201c..a595faa785c7 100644 --- a/packages/bun-usockets/src/internal/internal.h +++ b/packages/bun-usockets/src/internal/internal.h @@ -133,6 +133,10 @@ extern struct us_socket_t *us_dispatch_end(us_socket_r s); extern struct us_socket_t *us_dispatch_connect_error(us_socket_r s, int code); extern struct us_connecting_socket_t *us_dispatch_connecting_error(struct us_connecting_socket_t *c, int code); extern void us_dispatch_handshake(us_socket_r s, int success, struct us_bun_verify_error_t err); +/* A fatal SSL error surfaced after the handshake completed (a received fatal + * alert, a protocol violation). Reported to the socket's JS error handler right + * before the connection is torn down. */ +extern void us_dispatch_ssl_error(us_socket_r s, struct us_bun_verify_error_t err); extern void us_dispatch_session(us_socket_r s, const unsigned char *data, int length); extern void us_dispatch_keylog(us_socket_r s, const unsigned char *data, int length); extern struct us_socket_t *us_dispatch_ssl_raw_tap(us_socket_r s, char *data, int length); diff --git a/src/js/node/net.ts b/src/js/node/net.ts index 8d2305b5ef50..83e4863c3213 100644 --- a/src/js/node/net.ts +++ b/src/js/node/net.ts @@ -254,6 +254,19 @@ function tlsHandshakeError(verifyError) { return new ConnResetException("socket hang up"); } +/** + * A fatal SSL error the native layer reports after the handshake completed (a + * received fatal alert, a protocol violation) arrives as EPROTO carrying the + * OpenSSL error string. Node's TLSWrap onerror emits those on the socket rather + * than destroying it; the close that follows still delivers 'end' and 'close'. + * Returns true when the error was handled here. + */ +function emitPostHandshakeTLSError(self, error): boolean { + if (error?.code !== "EPROTO" || !self._secureEstablished) return false; + self.emit("error", tlsHandshakeError(error)); + return true; +} + const SocketHandlers: SocketHandler = { close(socket, err) { const self = socket.data; @@ -329,6 +342,7 @@ const SocketHandlers: SocketHandler = { callback(error); } + if (emitPostHandshakeTLSError(self, error)) return; self.emit("error", error); }, open(socket) { @@ -829,6 +843,7 @@ const ServerHandlers: SocketHandler = { if (data._hadError) return; data._hadError = true; + if (emitPostHandshakeTLSError(data, error)) return; const bunTLS = this[bunTlsSymbol]; if (typeof bunTLS === "function") { @@ -1198,6 +1213,7 @@ const SocketHandlers2: SocketHandler NewSocket { Ok(()) } + /// A fatal SSL error surfaced after the handshake completed (a received + /// fatal alert, a protocol violation). Reports it through the `error` + /// handler the way Node's `TLSWrap::ClearOut` reports it through `onerror`, + /// before the close that follows. The handler may destroy the socket, so + /// `openssl.c` re-checks liveness after this returns. + /// + /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`. + pub fn on_ssl_error( + this: bun_ptr::ThisPtr, + ssl_error: uws::us_bun_verify_error_t, + ) -> JsResult<()> { + jsc::mark_binding!(); + if this.socket.get().is_detached() { + return Ok(()); + } + // Same late-event guard as the other dispatch entry points: the + // socket may already have released its Handlers. + if !this.has_handlers() { + return Ok(()); + } + let handlers = this.get_handlers(); + if handlers.vm.is_shutting_down() || this.flags.get().contains(Flags::FINALIZING) { + return Ok(()); + } + let scope = handlers.enter(); + let global = handlers.global_object; + let this_value = this.get_this_value(&global); + let err_value = match super::uws_jsc::verify_error_to_js(&ssl_error, &global) { + Ok(v) => v, + Err(e) => { + this.exit_scope(scope); + return Err(e); + } + }; + let _ = handlers.call_error_handler(this_value, &[this_value, err_value]); + this.exit_scope(scope); + Ok(()) + } + /// A new resumable TLS session arrived (the peer's NewSessionTicket was /// processed during an earlier `SSL_read`). Hands the serialized session /// to the JS `session` handler, mirroring Node's `onnewsession` callback. diff --git a/src/runtime/socket/uws_dispatch.rs b/src/runtime/socket/uws_dispatch.rs index 508b43ab029c..e8395fcda212 100644 --- a/src/runtime/socket/uws_dispatch.rs +++ b/src/runtime/socket/uws_dispatch.rs @@ -221,6 +221,30 @@ pub(crate) unsafe extern "C" fn us_dispatch_ssl_raw_tap( s } +/// A fatal SSL error surfaced after the handshake already completed: a received +/// fatal alert (under TLS 1.3 a server's mTLS rejection lands here, because the +/// client finishes its handshake one flight earlier) or a protocol violation. +/// Mirrors Node's `TLSWrap::ClearOut` → `onerror`, which reports these on the +/// socket instead of letting them pass as a clean end-of-connection. Only +/// `bun_socket_tls` sockets reach this; every other kind already tears the +/// connection down through its own error path. +/// +/// # Safety +/// `openssl.c` must pass a live, non-null `s` whose ext slot holds a valid +/// `*mut TLSSocket`. +#[unsafe(no_mangle)] +pub unsafe extern "C" fn us_dispatch_ssl_error(s: *mut us_socket_t, err: us_bun_verify_error_t) { + let s_ref = us_socket_t::opaque_mut(s); + if s_ref.kind() != SocketKind::BunSocketTls { + return; + } + type TLSSocket = super::NewSocket; + let Some(tls) = *s_ref.ext::>>() else { + return; + }; + let _ = TLSSocket::on_ssl_error(tls, err); +} + /// A new (resumable) TLS session is ready. BoringSSL's new-session callback /// parks the serialized session while `SSL_read`/`SSL_do_handshake` runs; /// `ssl_flush_pending_session()` dispatches it here once that stack has diff --git a/test/js/node/tls/node-tls-connect.test.ts b/test/js/node/tls/node-tls-connect.test.ts index 096299fbcc22..fb5e983049d4 100644 --- a/test/js/node/tls/node-tls-connect.test.ts +++ b/test/js/node/tls/node-tls-connect.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "bun:test"; import { once } from "events"; +import { readFileSync } from "fs"; import { bunEnv, bunExe, tls as COMMON_CERT_, isASAN } from "harness"; import https from "https"; import net from "net"; @@ -747,3 +748,57 @@ it("https.request reports an impossible version window as a TLS error, not a cer await once(response, "end"); expect(body).toBe("ok"); }); + +it("reports the server's fatal alert rejecting a missing client certificate", async () => { + // TLS1.3 finishes the client's handshake one flight before the server has + // validated the client certificate, so the server's fatal + // certificate_required alert arrives after 'secureConnect'. Node reports it + // through TLSWrap's onerror; swallowing it leaves an mTLS rejection + // indistinguishable from a clean end-of-connection. + const server = tls.createServer({ + key: readFileSync(join(import.meta.dir, "fixtures", "agent1-key.pem")), + cert: readFileSync(join(import.meta.dir, "fixtures", "agent1-cert.pem")), + ca: readFileSync(join(import.meta.dir, "fixtures", "ca1-cert.pem")), + requestCert: true, + rejectUnauthorized: true, + minVersion: "TLSv1.3", + }); + server.on("tlsClientError", () => {}); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const port = (server.address() as AddressInfo).port; + + // The client holds no certificate to offer. + const client = tlsConnect({ port, host: "127.0.0.1", rejectUnauthorized: false }); + const events: string[] = []; + let error: any; + // `once(client, "close")` would reject on the 'error' this test is about. + const closed = Promise.withResolvers(); + client.on("secureConnect", () => events.push("secureConnect")); + client.on("error", e => { + events.push("error"); + error = e; + }); + client.on("end", () => events.push("end")); + client.on("close", hadError => closed.resolve(hadError)); + const hadError = await closed.promise; + + // BoringSSL names alert 116 TLSV1_ALERT_CERTIFICATE_REQUIRED; OpenSSL builds + // of Node spell the same alert TLSV13_ALERT_CERTIFICATE_REQUIRED. + expect({ + events, + hadError, + code: error?.code, + library: error?.library, + reason: error?.reason, + }).toEqual({ + events: ["secureConnect", "error", "end"], + hadError: false, + code: "ERR_SSL_TLSV1_ALERT_CERTIFICATE_REQUIRED", + library: "SSL routines", + reason: "TLSV1_ALERT_CERTIFICATE_REQUIRED", + }); + + server.close(); + await once(server, "close"); +}); diff --git a/test/js/node/tls/node-tls-server.test.ts b/test/js/node/tls/node-tls-server.test.ts index 5c60031fc8dc..2d20dfeb49ce 100644 --- a/test/js/node/tls/node-tls-server.test.ts +++ b/test/js/node/tls/node-tls-server.test.ts @@ -1,7 +1,7 @@ import crypto from "crypto"; import { readFileSync, realpathSync } from "fs"; import { tls as cert1, isDebug } from "harness"; -import { AddressInfo } from "net"; +import { AddressInfo, connect as netConnect, createServer as netCreateServer } from "net"; import { createTest } from "node-harness"; import { once } from "node:events"; import { tmpdir } from "os"; @@ -1114,9 +1114,11 @@ it("SNICallback runs even when the requested servername matches the bind hostnam }); server.listen(0, "localhost"); await once(server, "listening"); - const port = (server.address() as AddressInfo).port; - // host: "localhost" defaults servername to "localhost" - the bind hostname. - const client = connect({ port, host: "localhost", rejectUnauthorized: false }); + // Dial the address the listener actually bound to: on a dual-stack host + // `localhost` can resolve to a different family for connect() than it did for + // listen(). `servername` still carries the bind hostname, which is the point. + const { address, port } = server.address() as AddressInfo; + const client = connect({ port, host: address, servername: "localhost", rejectUnauthorized: false }); await once(client, "secureConnect"); expect(sniCalls).toBe(1); // The peer certificate must be the SNICallback's RSA cert, not COMMON_CERT. @@ -1318,3 +1320,108 @@ it("tls.connect honors secureOptions when negotiating the protocol version", asy } await once(server, "close"); }); + +describe("tls.Server post-handshake TLS errors", () => { + it("reports a corrupted record on the accepted socket instead of closing cleanly", async () => { + // A record that cannot be authenticated fails SSL_read after the handshake + // completed. Node reports those through TLSWrap's onerror; swallowing one + // hides a protocol failure behind an ordinary end-of-connection. + const { promise, resolve, reject } = Promise.withResolvers(); + const server = createServer(COMMON_CERT, socket => { + socket.on("error", resolve); + socket.on("close", () => reject(new Error("accepted socket closed without an 'error'"))); + // The client waits for this before corrupting the stream, so the server's + // handshake has provably completed by then. + socket.write("hello"); + }); + server.on("error", reject); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + + const raw = netConnect((server.address() as AddressInfo).port, "127.0.0.1"); + const client = connect({ socket: raw, rejectUnauthorized: false }); + raw.on("error", () => {}); + client.on("error", () => {}); + client.once("data", () => { + // A TLS1.3 application_data record whose payload cannot authenticate. + const payload = Buffer.alloc(32, 0xab); + raw.write(Buffer.concat([Buffer.from([0x17, 0x03, 0x03, 0x00, payload.length]), payload])); + }); + + try { + const error = await promise; + expect({ code: error?.code, library: error?.library, reason: error?.reason }).toEqual({ + code: "ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", + library: "SSL routines", + reason: "DECRYPTION_FAILED_OR_BAD_RECORD_MAC", + }); + } finally { + client.destroy(); + raw.destroy(); + server.close(); + } + await once(server, "close"); + }); + + it("delivers data decrypted alongside a fatal record before reporting the error", async () => { + // A peer can put application data and the record that fails in one segment. + // Node's ClearOut pushes each decrypted chunk to the consumer before it + // reports, so the plaintext must not be dropped on the way to the error. + const events: string[] = []; + const { promise, resolve, reject } = Promise.withResolvers(); + const server = createServer(COMMON_CERT, socket => { + socket.on("data", chunk => events.push(`data:${chunk}`)); + socket.on("error", err => { + events.push(`error:${err.code}`); + resolve(events); + }); + socket.on("close", () => reject(new Error(`closed without an 'error': ${events}`))); + // The client waits for this, so the server's handshake has provably + // completed and its flight is flushed before the relay splices anything. + socket.write("hello"); + }); + server.on("error", reject); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const serverPort = (server.address() as AddressInfo).port; + + // A plain TCP relay, so one client record and one corrupt record reach the + // server in a single recv() and the server's SSL_read loop decrypts the + // first before the second fails. + let spliceCorruptRecord = false; + const relay = netCreateServer(downstream => { + const upstream = netConnect(serverPort, "127.0.0.1"); + upstream.pipe(downstream); + downstream.on("data", chunk => { + if (!spliceCorruptRecord) return void upstream.write(chunk); + spliceCorruptRecord = false; + const payload = Buffer.alloc(32, 0xab); + const corrupt = Buffer.concat([Buffer.from([0x17, 0x03, 0x03, 0x00, payload.length]), payload]); + upstream.write(Buffer.concat([chunk, corrupt])); + }); + downstream.on("error", () => {}); + upstream.on("error", () => {}); + }); + relay.listen(0, "127.0.0.1"); + await once(relay, "listening"); + + const client = connect({ + port: (relay.address() as AddressInfo).port, + host: "127.0.0.1", + rejectUnauthorized: false, + }); + client.on("error", () => {}); + await once(client, "data"); + spliceCorruptRecord = true; + client.write("ping"); + + try { + expect(await promise).toEqual(["data:ping", "error:ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC"]); + } finally { + client.destroy(); + relay.close(); + server.close(); + } + await once(server, "close"); + }); +});