Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/bun-usockets/src/crypto/openssl.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
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);
Expand Down
1 change: 1 addition & 0 deletions src/http/ProxyTunnel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
},
Expand Down
1 change: 1 addition & 0 deletions src/http_jsc/websocket_client/WebSocketProxyTunnel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
},
)
Expand Down
13 changes: 11 additions & 2 deletions src/js/node/net.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down
10 changes: 10 additions & 0 deletions src/runtime/socket/UpgradedDuplex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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),
Expand Down
12 changes: 12 additions & 0 deletions src/runtime/socket/WindowsNamedPipe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand All @@ -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),
Expand Down
8 changes: 8 additions & 0 deletions src/runtime/socket/WindowsNamedPipeContext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)) };
Expand Down Expand Up @@ -393,6 +400,7 @@ impl WindowsNamedPipeContext {
on_error: |p, e| Self::on_error(p.cast::<Self>(), &e),
on_timeout: |p| Self::on_timeout(p.cast::<Self>()),
on_close: |p| Self::on_close(p.cast::<Self>()),
on_ssl_error: |p, err| Self::on_ssl_error(p.cast::<Self>(), err),
on_session: |p, d| Self::on_session(p.cast::<Self>(), d),
on_keylog: |p, d| Self::on_keylog(p.cast::<Self>(), d),
server_identity: |p, ssl| Self::server_identity(p.cast::<Self>(), ssl),
Expand Down
47 changes: 47 additions & 0 deletions src/runtime/socket/socket_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2142,6 +2142,43 @@ impl<const SSL: bool> NewSocket<SSL> {
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<Self>, 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(());
}
Comment thread
robobun marked this conversation as resolved.
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<Self>` for the same re-entrancy reason as `on_writable`.
pub(crate) fn on_close(
this: bun_ptr::ThisPtr<Self>,
Expand Down Expand Up @@ -4484,6 +4521,12 @@ impl DuplexUpgradeContext {
}
}

fn on_ssl_error(this: bun_ptr::ThisPtr<Self>, 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<Self>) {
let socket = this.duplex_socket();
if let Some(tls) = this.tls.replace(None) {
Expand Down Expand Up @@ -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 ()| {
Expand Down
44 changes: 42 additions & 2 deletions src/uws/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment thread
robobun marked this conversation as resolved.
pub on_ssl_error: Option<fn(T, u32)>,
/// 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).
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -1128,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();
Comment thread
robobun marked this conversation as resolved.
// SAFETY: ssl is a live SSL*; available is a valid mutable slice.
let just_read = unsafe {
boring_sys::SSL_read(
Expand All @@ -1140,6 +1155,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
Expand Down Expand Up @@ -1186,8 +1209,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);
}

Expand All @@ -1201,13 +1223,28 @@ 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.
Comment thread
robobun marked this conversation as resolved.
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).
self.flush_pending_events();
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);
Expand Down Expand Up @@ -1500,6 +1537,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.
Comment thread
robobun marked this conversation as resolved.
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;
Expand Down
49 changes: 49 additions & 0 deletions test/js/node/http2/node-http2-upgrade.test.mts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
Loading
Loading