tls: deliver decrypted bytes before SSLWrapper answers close_notify (wss via CONNECT proxy reports 1006 on a server close)

src/uws/lib.rs

@@ -595,21 +595,9 @@ pub mod ssl_wrapper {
             // we already sent the ssl shutdown
             if self.flags.sent_ssl_shutdown() || self.flags.fatal_error() {
                 if fast_shutdown {
-                    // A fast shutdown is a full teardown — the owner calls it
-                    // right before detaching/freeing handlers.ctx (proxy tunnel
-                    // on response-complete; TLS-over-duplex on raw EOF).
-                    //
-                    // Run the close callback now, regardless of whether the
-                    // peer's close_notify has arrived: if it HAS (processed
-                    // mid handle_reading, which sets sent_ssl_shutdown before
-                    // flushing the final decrypted bytes), closed_notified may
-                    // still be unset and handle_reading's deferred
-                    // trigger_close_callback would otherwise fire on_close
-                    // into the freed ctx; if it has NOT (peer went away after
-                    // our shutdown), the TLS-over-duplex teardown chain
-                    // (UpgradedDuplex::on_close -> DuplexUpgradeContext::on_close
-                    // -> deinit) never runs and leaks the whole context graph.
-                    // trigger_close_callback is idempotent (closed_notified).
+                    // The owner frees handlers.ctx after a fast shutdown, so close now: a
+                    // handle_reading still on the stack would fire on_close into it (#31959),
+                    // and when no peer close_notify comes, nothing else runs the owner's teardown.
                     self.flags.set_received_ssl_shutdown(true);
                     self.trigger_close_callback();
                     // Do not read self after the close callback: the owner's
@@ -1034,8 +1022,6 @@ pub mod ssl_wrapper {
                             // Remotely-Initiated Shutdown
                             // See: https://www.openssl.org/docs/manmaster/man3/SSL_shutdown.html
                             self.flags.set_received_ssl_shutdown(true);
-                            // 2-step shutdown
-                            let _ = self.shutdown(false);
                             self.handle_end_of_renegotiation();
                         }
                         if err == boring_sys::SSL_ERROR_SSL || err == boring_sys::SSL_ERROR_SYSCALL
@@ -1060,6 +1046,10 @@ pub mod ssl_wrapper {
                         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);
+                        }
                         self.trigger_close_callback();
                         return false;
                     } else {

test/js/bun/http/proxy.test.ts

@@ -1124,10 +1124,10 @@ test("HTTPS origin close-delimited body via HTTP proxy does not ECONNRESET", asy
   }
 });
 
-// Use-after-free in the proxy tunnel close path: when the final response
-// bytes and the TLS close_notify arrive in one TCP batch, SSLWrapper's
-// handle_reading sets sent_ssl_shutdown before flushing the decrypted bytes.
-// The data callback completes the response, and the done path's
+// Use-after-free in the proxy tunnel close path: the final response bytes and
+// the TLS close_notify arrive in one TCP batch, so the data callback completes
+// the response while SSLWrapper's handle_reading is still on the stack. Back
+// then handle_reading had already set sent_ssl_shutdown, and the done path's
 // ProxyTunnel.shutdown() hit SSLWrapper.shutdown()'s already-shut-down early
 // return without marking the wrapper closed_notified, so after the client was
 // freed handle_reading still fired on_close into the stale handlers.ctx.

test/js/node/tls/node-tls-connect.test.ts

@@ -843,6 +843,84 @@ it("a client and a server TLSSocket connected through a synchronous in-memory du
   });
 });
 
+it("the last 'data' event fires before the close_notify reply is written to a duplex transport (tls.connect({ socket }))", async () => {
+  // The peer's last application data and its close_notify reach the engine in
+  // one chunk. The engine used to answer the close_notify before it emitted
+  // the data decrypted ahead of it. Node emits the data first.
+  //
+  // TLS 1.2 keeps the record type readable on the wire: 21 is an alert.
+  const version = { minVersion: "TLSv1.2", maxVersion: "TLSv1.2" } as const;
+  // The record types in `bytes`, or null while the last record is incomplete.
+  const recordTypes = (bytes: Buffer) => {
+    const types: number[] = [];
+    let offset = 0;
+    while (offset + 5 <= bytes.length) {
+      const next = offset + 5 + bytes.readUInt16BE(offset + 3);
+      if (next > bytes.length) return null;
+      types.push(bytes[offset]);
+      offset = next;
+    }
+    return offset === bytes.length ? types : null;
+  };
+
+  const log: string[] = [];
+  // Once armed, the server's bytes are held until they end with its complete
+  // close_notify, then pushed to the client as one chunk.
+  let held: Buffer | null = null;
+  const clientSide: Duplex = new Duplex({
+    read() {},
+    write(chunk: Buffer, _encoding, callback) {
+      if (recordTypes(chunk)?.includes(21)) log.push("write close_notify");
+      serverSide.push(chunk);
+      callback();
+    },
+    final(callback) {
+      log.push("transport end");
+      serverSide.push(null);
+      callback();
+    },
+  });
+  const serverSide: Duplex = new Duplex({
+    read() {},
+    write(chunk: Buffer, _encoding, callback) {
+      if (held === null) {
+        clientSide.push(chunk);
+      } else {
+        held = Buffer.concat([held, chunk]);
+        if (recordTypes(held)?.at(-1) === 21) {
+          const burst = held;
+          held = null;
+          log.push(`push ${recordTypes(burst)!.join(",")}`);
+          clientSide.push(burst);
+        }
+      }
+      callback();
+    },
+    final(callback) {
+      clientSide.push(null);
+      callback();
+    },
+  });
+
+  const server = new TLSSocket(serverSide, {
+    isServer: true,
+    secureContext: tls.createSecureContext({ ...COMMON_CERT_, ...version }),
+  });
+  server.on("error", () => {});
+  server.on("data", () => server.end("last"));
+
+  const client = tls.connect({ socket: clientSide, rejectUnauthorized: false, ...version });
+  await once(client, "secureConnect");
+  client.on("data", (chunk: Buffer) => log.push(`data ${chunk}`));
+  client.on("end", () => log.push("end"));
+  held = Buffer.alloc(0);
+  client.write("go");
+  await once(client, "close");
+
+  // 23 is application data. One push carried it and the alert.
+  expect(log).toEqual(["push 23,21", "data last", "write close_notify", "transport end", "end"]);
+});
+
 describe("application data written over a Duplex transport before the handshake completes", () => {
   // Node parks such a write (TLSWrap's pending cleartext) and sends it right
   // after the handshake: the write is still pending when 'secureConnect' /

test/js/web/websocket/websocket-proxy.test.ts

@@ -2,6 +2,10 @@ import { afterAll, beforeAll, describe, expect, test } from "bun:test";
 import * as harness from "harness";
 import { tls as tlsCerts } from "harness";
 import type { HttpsProxyAgent as HttpsProxyAgentType } from "https-proxy-agent";
+import { createHash } from "node:crypto";
+import { once } from "node:events";
+import type { AddressInfo } from "node:net";
+import tls from "node:tls";
 import {
   type ClientEvent,
   clientEvents,
@@ -550,6 +554,139 @@ describe("WebSocket wss:// through HTTP proxy (TLS tunnel)", () => {
       spinProxy.close();
     }
   });
+
+  // A server that closes does not wait for the echo of its Close frame. It sends
+  // the frame and ends the TLS session behind it (ws.close() in Bun.serve does),
+  // so the frame and the close_notify alert usually reach the client in one
+  // read. The tunnel's TLS engine used to answer the close_notify with its own
+  // before it handed over the bytes decrypted ahead of it. The echo of the
+  // Close frame then had no TLS session left to go out on, and the client
+  // reported 1006 "Failed to write" instead of the server's close code.
+  describe("server close with the TLS close_notify behind the Close frame", () => {
+    const serverClose: ClientEvent[] = ["m1", "m2", { code: 4001, reason: "bye", wasClean: true }];
+
+    // "direct" is the reference: the same server without a proxy.
+    test.each(["direct", "http", "https"])("Bun.serve origin that calls ws.close() (%s)", async route => {
+      using origin = Bun.serve({
+        port: 0,
+        tls: { key: tlsCerts.key, cert: tlsCerts.cert },
+        fetch(req, server) {
+          if (server.upgrade(req)) return;
+          return new Response("Expected WebSocket", { status: 400 });
+        },
+        websocket: {
+          message(ws) {
+            ws.send("m1");
+            ws.send("m2");
+            ws.close(4001, "bye");
+          },
+        },
+      });
+      using recorded = await startRecordingProxy({ tls: route === "https" });
+      const ws = new WebSocket(`wss://127.0.0.1:${origin.port}`, {
+        ...(route === "direct" ? {} : { proxy: `${route}://127.0.0.1:${recorded.port}` }),
+        tls: { rejectUnauthorized: false },
+      });
+      ws.addEventListener("open", () => ws.send("go"));
+      expect({ events: await clientEvents(ws), requests: recorded.requests }).toEqual({
+        events: serverClose,
+        requests: route === "direct" ? [] : [connectRequest(origin.port)],
+      });
+    });
+
+    // The case above leaves it to the network whether both TLS records reach
+    // the client in one read. This one pins it.
+    test.each(["http", "https"])("Close frame and close_notify in one read (%s proxy)", async scheme => {
+      const frame = (opcode: number, payload: string | Uint8Array) => {
+        const body = Buffer.from(payload);
+        return Buffer.concat([Buffer.from([0x80 | opcode, body.length]), body]);
+      };
+      const closePayload = Buffer.concat([Buffer.from([4001 >> 8, 4001 & 0xff]), Buffer.from("bye")]);
+
+      // Completes the upgrade by hand. Whatever the client sends next is its
+      // "go": answer with two messages and a Close frame, and end the TLS
+      // session in the same call.
+      await using origin = tls.createServer({ key: tlsCerts.key, cert: tlsCerts.cert }, socket => {
+        let request = "";
+        let upgraded = false;
+        socket.on("error", () => {});
+        socket.on("data", chunk => {
+          if (upgraded) {
+            if (!socket.writableEnded) {
+              socket.end(Buffer.concat([frame(0x1, "m1"), frame(0x1, "m2"), frame(0x8, closePayload)]));
+            }
+            return;
+          }
+          request += chunk.toString("latin1");
+          if (!request.includes("\r\n\r\n")) return;
+          upgraded = true;
+          const key = /^sec-websocket-key: (.*)\r$/im.exec(request)![1];
+          const accept = createHash("sha1")
+            .update(key + "258EAFA5-E914-47DA-95CA-C5AB0DC85B11")
+            .digest("base64");
+          socket.write(
+            "HTTP/1.1 101 Switching Protocols\r\n" +
+              "Upgrade: websocket\r\n" +
+              "Connection: Upgrade\r\n" +
+              `Sec-WebSocket-Accept: ${accept}\r\n\r\n`,
+          );
+        });
+      });
+      origin.listen(0, "127.0.0.1");
+      await once(origin, "listening");
+      const originPort = (origin.address() as AddressInfo).port;
+
+      // The complete TLS records in `bytes`, and the bytes left over behind them.
+      const tlsRecords = (bytes: Buffer) => {
+        let complete = 0;
+        let offset = 0;
+        while (offset + 5 <= bytes.length) {
+          const next = offset + 5 + bytes.readUInt16BE(offset + 3);
+          if (next > bytes.length) break;
+          offset = next;
+          complete++;
+        }
+        return { complete, trailing: bytes.length - offset };
+      };
+      // Armed once the client is open. From then on the server sends exactly
+      // two TLS records: the frames, then the close_notify alert. Hold them
+      // until both are complete and forward them in one write, so the client
+      // decrypts them in one read.
+      let armed = false;
+      let held: Buffer | null = null;
+      const forwardedOnceArmed: ReturnType<typeof tlsRecords>[] = [];
+      using recorded = await startRecordingProxy({
+        tls: scheme === "https",
+        onTargetData(chunk, forward) {
+          let bytes = chunk;
+          if (held !== null) {
+            held = Buffer.concat([held, chunk]);
+            if (tlsRecords(held).complete < 2) return;
+            bytes = held;
+            held = null;
+          }
+          if (armed) forwardedOnceArmed.push(tlsRecords(bytes));
+          forward(bytes);
+        },
+      });
+
+      const ws = new WebSocket(`wss://127.0.0.1:${originPort}`, {
+        proxy: `${scheme}://127.0.0.1:${recorded.port}`,
+        tls: { rejectUnauthorized: false },
+      });
+      ws.addEventListener("open", () => {
+        armed = true;
+        held = Buffer.alloc(0);
+        ws.send("go");
+      });
+      expect({ events: await clientEvents(ws), requests: recorded.requests, forwardedOnceArmed }).toEqual({
+        events: serverClose,
+        requests: [connectRequest(originPort)],
+        // One write to the client carried the record with the frames and the alert.
+        forwardedOnceArmed: [{ complete: 2, trailing: 0 }],
+      });
+    });
+  });
 });
 
 describe("WebSocket through HTTPS proxy (TLS proxy)", () => {