diff --git a/packages/bun-usockets/src/crypto/openssl.c b/packages/bun-usockets/src/crypto/openssl.c index cb5d8706a08a..1b7ac5d6c4b0 100644 --- a/packages/bun-usockets/src/crypto/openssl.c +++ b/packages/bun-usockets/src/crypto/openssl.c @@ -2616,6 +2616,53 @@ struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, i /* Spill-deferred close: the socket is still live; report it so the * caller's bookkeeping does not treat it as destroyed. */ return s; + } else if ((err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) && + (s->ssl_handshake_state == HANDSHAKE_COMPLETED || + (s->ssl_handshake_state == HANDSHAKE_PENDING && SSL_is_init_finished(s_ssl(s))))) { + /* A fatal error on an established session: a peer alert (a TLS 1.3 + * server that rejects the client certificate sends certificate_required + * after the client's handshake is done) or a record that does not + * decrypt. No handshake dispatch reports it, so hand the reason to + * the socket before the close, like node's ClearOut calls onerror. + * Report the first SSL-library entry: for a bad record BoringSSL + * queues the cipher's BAD_DECRYPT ahead of the TLS reason. */ + uint32_t ssl_err = ERR_peek_error(); + for (uint32_t queued; (queued = ERR_get_error()) != 0;) { + if (ERR_GET_LIB(queued) == ERR_LIB_SSL) { + ssl_err = queued; + break; + } + } + /* Everything that came before the failing record goes first, in wire + * order, while the socket is not yet fatal: a fatal socket gives the + * handshake dispatch no verify result, and data handlers assert that + * the socket is not shut down. The JS this runs may close the socket. */ + if (s->ssl_handshake_state == HANDSHAKE_PENDING) { + /* This read finished the handshake, then failed on the next record. + * Report the handshake, as the no-data completion below does. */ + ssl_trigger_handshake(s, 1); + if (ssl_gone(s)) return NULL; + loop_ssl_data->ssl_socket = s; + if (loop_ssl_data->ssl_write_batch_len && + loop_ssl_data->ssl_write_batch_owner == s) { + ssl_flush_write_batch(loop_ssl_data, s); + } + } + /* The same order as the ZERO_RETURN branch above and node's ClearOut. */ + 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; + } + ssl_park_fatal_reason(s); + if (ssl_err) { + us_dispatch_ssl_error(s, ssl_err); + if (ssl_gone(s)) return NULL; + } + ssl_close(s, 0, NULL); + return NULL; } if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) { diff --git a/packages/bun-usockets/src/internal/internal.h b/packages/bun-usockets/src/internal/internal.h index c1ddb88af99b..f7e958473e69 100644 --- a/packages/bun-usockets/src/internal/internal.h +++ b/packages/bun-usockets/src/internal/internal.h @@ -142,6 +142,7 @@ extern struct us_connecting_socket_t *us_dispatch_connecting_error(struct us_con extern void us_dispatch_handshake(us_socket_r s, int success, 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 void us_dispatch_ssl_error(us_socket_r s, uint32_t err); extern struct us_socket_t *us_dispatch_ssl_raw_tap(us_socket_r s, char *data, int length); extern int Bun__addrinfo_get(struct us_loop_t* loop, const char* host, uint16_t port, struct addrinfo_request** ptr); diff --git a/src/js/node/net.ts b/src/js/node/net.ts index 0609cd36d896..0a551ee5801f 100644 --- a/src/js/node/net.ts +++ b/src/js/node/net.ts @@ -1021,7 +1021,8 @@ const ServerHandlers: SocketHandler = { reportError(err); } }, - error(socket, error) { + // See SocketHandlers2.error for `tlsFatal`. + error(socket, error, tlsFatal?: boolean) { const data = this.data; if (!data) return; @@ -1050,6 +1051,9 @@ const ServerHandlers: SocketHandler = { ) { // 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 @@ -1396,7 +1400,8 @@ const SocketHandlers2: SocketHandler 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( + callback, + &global, + this_value, + &[this_value, err_value, JSValue::TRUE], + ); + Ok(()) + } + /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`. pub fn on_close( this: bun_ptr::ThisPtr, diff --git a/src/runtime/socket/uws_dispatch.rs b/src/runtime/socket/uws_dispatch.rs index fc1276cdea9a..ba54ade58215 100644 --- a/src/runtime/socket/uws_dispatch.rs +++ b/src/runtime/socket/uws_dispatch.rs @@ -283,3 +283,17 @@ pub(crate) unsafe extern "C" fn us_dispatch_keylog( let slice = unsafe { core::slice::from_raw_parts(data, len) }; crate::dispatch::fold(TLSSocket::on_keylog(tls, slice)); } + +/// A fatal TLS error (a packed BoringSSL code) on a live, established `s`, just before its close. +#[unsafe(no_mangle)] +pub(crate) unsafe extern "C" fn us_dispatch_ssl_error(s: *mut us_socket_t, err: u32) { + 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; + }; + crate::dispatch::fold(TLSSocket::on_ssl_error(tls, err)); +} diff --git a/test/js/bun/net/socket.test.ts b/test/js/bun/net/socket.test.ts index 1ae11107fcce..f5180f57bb23 100644 --- a/test/js/bun/net/socket.test.ts +++ b/test/js/bun/net/socket.test.ts @@ -4706,3 +4706,53 @@ it("concurrent end() on two allowHalfOpen TLS peers closes both sockets", async await Promise.all([serverClosed.promise, clientClosed.promise]); }); + +describe("a fatal TLS alert that arrives after the handshake", () => { + // A TLS 1.3 server checks the client certificate after the client's + // handshake is done, so its certificate_required alert arrives after the + // client's handshake callback. Returns the client's events up to 'close'. + async function rejectedClientEvents(withErrorHandler: boolean) { + const events: string[] = []; + const closed = Promise.withResolvers(); + using server = Bun.listen({ + hostname: "127.0.0.1", + port: 0, + tls: { key: tls.key, cert: tls.cert, ca: tls.cert, requestCert: true, rejectUnauthorized: true }, + socket: { + data() {}, + error() {}, + }, + }); + await Bun.connect({ + hostname: "127.0.0.1", + port: server.port, + tls: { ca: tls.cert }, + socket: { + handshake: (socket, success) => events.push(`handshake ${success} ${socket.getTLSVersion()}`), + data: () => events.push("data"), + error: withErrorHandler + ? (_socket, err) => events.push(`error ${(err as Error & { code?: string }).code}`) + : undefined, + close: (_socket, err) => { + events.push(`close ${err}`); + closed.resolve(); + }, + }, + }); + await closed.promise; + return events; + } + + it("reaches the error handler before close", async () => { + expect(await rejectedClientEvents(true)).toEqual([ + "handshake true TLSv1.3", + "error ERR_SSL_TLSV1_ALERT_CERTIFICATE_REQUIRED", + "close undefined", + ]); + }); + + it("only closes a socket that has no error handler", async () => { + // The peer's alert must not become an uncaught exception in this process. + expect(await rejectedClientEvents(false)).toEqual(["handshake true TLSv1.3", "close undefined"]); + }); +}); diff --git a/test/js/node/tls/node-tls-cert.test.ts b/test/js/node/tls/node-tls-cert.test.ts index 3911a9ec0617..eb69b49a60c3 100644 --- a/test/js/node/tls/node-tls-cert.test.ts +++ b/test/js/node/tls/node-tls-cert.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from "bun:test"; import { once } from "events"; import { readFileSync } from "fs"; -import { bunEnv, bunExe, invalidTls, tmpdirSync } from "harness"; +import { bunEnv, bunExe, tls as harnessCert, invalidTls, tmpdirSync } from "harness"; import type { AddressInfo } from "node:net"; import type { Server, TLSSocket } from "node:tls"; import { join } from "path"; @@ -159,6 +159,51 @@ it("Request cert from TLS1.2 client that doesn't have one.", async () => { } }); +it("Request cert from TLS1.3 client that doesn't have one.", async () => { + // TLS 1.3 finishes the client's handshake before the server checks the + // certificate, so the alert arrives after 'secureConnect'. + // The agent10 fixtures above have a 1024-bit CA, which OpenSSL rejects as too + // weak, so this case uses the 2048-bit harness certificate instead. + const server = tls.createServer({ + key: harnessCert.key, + cert: harnessCert.cert, + ca: [harnessCert.cert], + requestCert: true, + }); + const serverError = once(server, "tlsClientError"); + await once(server.listen(0, "127.0.0.1"), "listening"); + + try { + const events: string[] = []; + const { promise: closed, resolve: onClose } = Promise.withResolvers(); + const client = tls.connect({ + host: "127.0.0.1", + port: (server.address() as AddressInfo).port, + minVersion: "TLSv1.3", + ca: harnessCert.cert, + }); + client.on("secureConnect", () => events.push("secureConnect")); + client.on("data", () => events.push("data")); + client.on("error", (err: any) => events.push(`error ${err.code}`)); + client.on("end", () => events.push("end")); + client.on("close", (hadError: boolean) => { + events.push(`close ${hadError}`); + onClose(); + }); + await closed; + + // BoringSSL and OpenSSL name the same alert differently. + const alert = process.features.openssl_is_boringssl + ? "ERR_SSL_TLSV1_ALERT_CERTIFICATE_REQUIRED" + : "ERR_SSL_TLSV13_ALERT_CERTIFICATE_REQUIRED"; + expect(events).toEqual(["secureConnect", `error ${alert}`, "end", "close false"]); + const [err] = await serverError; + expect(err.code).toBe("ERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATE"); + } finally { + server.close(); + } +}); + it("Typical configuration error, incomplete cert chains sent, we have to know the peer's subordinate CAs in order to verify the peer.", async () => { await connect({ client: { diff --git a/test/js/node/tls/node-tls-server.test.ts b/test/js/node/tls/node-tls-server.test.ts index 707474890f20..2549e5a513fe 100644 --- a/test/js/node/tls/node-tls-server.test.ts +++ b/test/js/node/tls/node-tls-server.test.ts @@ -1322,9 +1322,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 that the listener bound to: `localhost` can resolve to + // another address family for connect() than it did for listen(). The + // servername is still the bind hostname. + 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. @@ -2685,3 +2687,153 @@ describe("pauseOnConnect", () => { } }); }); + +describe("fatal TLS error after the handshake", () => { + // A TLS 1.3 application_data record whose payload does not authenticate. + function badRecord() { + const payload = Buffer.alloc(32, 0xab); + return Buffer.concat([Buffer.from([0x17, 0x03, 0x03, 0x00, payload.length]), payload]); + } + + // Each client below sends its bad record and the FIN together, the way a peer + // that gives up does. Node keeps an errored socket open until that FIN, so + // both runtimes reach 'end' and close(false) after the error. + function recordEvents(socket: TLSSocket, events: string[], closed: PromiseWithResolvers) { + socket.on("data", chunk => events.push(`data ${chunk}`)); + socket.on("error", (err: Error & { code?: string }) => events.push(`error ${err.code}`)); + socket.on("end", () => events.push("end")); + socket.on("close", hadError => { + events.push(`close ${hadError}`); + closed.resolve(); + }); + } + + // A TCP relay from the client to the server. `tamper` gets each chunk that + // the client sends, with its index, and returns the bytes to forward and + // whether to send the FIN with them. + async function startRelay(serverPort: number, tamper: (chunk: Buffer, index: number) => [Buffer, boolean]) { + const relay = net.createServer(downstream => { + const upstream = net.connect(serverPort, "127.0.0.1"); + upstream.pipe(downstream); + let index = 0; + downstream.on("data", chunk => { + const [bytes, fin] = tamper(chunk, index++); + if (fin) upstream.end(bytes); + else upstream.write(bytes); + }); + downstream.on("close", () => upstream.destroy()); + downstream.on("error", () => {}); + upstream.on("error", () => {}); + }); + await once(relay.listen(0, "127.0.0.1"), "listening"); + return relay; + } + + it("reports a record that does not decrypt as an 'error' on the accepted socket", async () => { + const events: string[] = []; + const closed = Promise.withResolvers(); + let error: (Error & { library?: string; reason?: string }) | undefined; + const server = createServer(COMMON_CERT, socket => { + recordEvents(socket, events, closed); + socket.once("error", err => (error = err)); + // The client sends the bad record when this arrives, so the server's + // handshake is complete by then. + socket.write("hello"); + }); + await once(server.listen(0, "127.0.0.1"), "listening"); + const raw = net.connect((server.address() as AddressInfo).port, "127.0.0.1"); + raw.on("error", () => {}); + const client = connect({ socket: raw, rejectUnauthorized: false }); + client.on("error", () => {}); + client.once("data", () => raw.end(badRecord())); + try { + await closed.promise; + expect(events).toEqual(["error ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", "end", "close false"]); + // BoringSSL names the reason DECRYPTION_FAILED_OR_BAD_RECORD_MAC, OpenSSL + // spells the same reason in words. + expect({ library: error?.library, reason: error?.reason?.toUpperCase().replaceAll(" ", "_") }).toEqual({ + library: "SSL routines", + reason: "DECRYPTION_FAILED_OR_BAD_RECORD_MAC", + }); + } finally { + client.destroy(); + raw.destroy(); + server.close(); + } + }); + + it("delivers the data that arrives with the bad record before the 'error'", async () => { + const events: string[] = []; + const closed = Promise.withResolvers(); + const server = createServer(COMMON_CERT, socket => { + recordEvents(socket, events, closed); + socket.write("hello"); + }); + await once(server.listen(0, "127.0.0.1"), "listening"); + // The relay appends the bad record to the client's next write, so the + // server reads both in one read. + let appendBadRecord = false; + const relay = await startRelay((server.address() as AddressInfo).port, chunk => { + if (!appendBadRecord) return [chunk, false]; + appendBadRecord = false; + return [Buffer.concat([chunk, badRecord()]), true]; + }); + const client = connect({ + port: (relay.address() as AddressInfo).port, + host: "127.0.0.1", + rejectUnauthorized: false, + }); + client.on("error", () => {}); + try { + await once(client, "data"); + appendBadRecord = true; + client.write("ping"); + await closed.promise; + expect(events).toEqual(["data ping", "error ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", "end", "close false"]); + } finally { + client.destroy(); + relay.close(); + server.close(); + } + }); + + it("reports the handshake and then the 'error' when one read holds the Finished and the bad record", async () => { + const events: string[] = []; + const closed = Promise.withResolvers(); + const server = createServer({ ...COMMON_CERT, minVersion: "TLSv1.3" }, socket => { + events.push("secureConnection"); + recordEvents(socket, events, closed); + }); + server.on("tlsClientError", (err: Error & { code?: string }) => { + events.push(`tlsClientError ${err.code}`); + closed.resolve(); + }); + await once(server.listen(0, "127.0.0.1"), "listening"); + // The client's second write is the flight that ends with its Finished. The + // relay appends the bad record to it, so the server's handshake completes + // in the same read that fails. + const relay = await startRelay((server.address() as AddressInfo).port, (chunk, index) => + index === 1 ? [Buffer.concat([chunk, badRecord()]), true] : [chunk, false], + ); + const client = connect({ + port: (relay.address() as AddressInfo).port, + host: "127.0.0.1", + rejectUnauthorized: false, + minVersion: "TLSv1.3", + }); + client.on("error", () => {}); + try { + await closed.promise; + expect(events).toEqual([ + "secureConnection", + "error ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC", + "end", + "close false", + ]); + } finally { + client.destroy(); + relay.close(); + server.close(); + } + }); +});