ReviewOS

home-lang/home

node:http directory test run aborts (SIGSEGV in BoringSSL SSL_new during TLS connect) — heap corruption, culprit≠victim

#78
Closed glennmichael123 opened this 10 days ago ·
10 days ago

Summary

Running the full test/js/node/http directory in one process aborts with SIGSEGV → abort (exit 134) partway through. Reproduces on both home-debug and home-release-safe. It is a heap-corruption crash: the faulting frame is a BoringSSL cert allocation during a TLS client connect, but that allocation is the victim — an earlier (non-TLS) test corrupts the heap, and the next TLS SSL_new trips over it.

Repro

cd /tmp/bun-pinned   # bun 1.3.14 pin worktree
home-release-safe test test/js/node/http
# → exits 134, no test summary printed (aborts mid-run)

Faulting backtrace (from the macOS crash report)

libsystem_c.dylib            abort
home-debug                   debug.handleSegfault           (SIGSEGV handler)
bssl::New<bssl::CERT, bssl::SSL_X509_METHOD const*&>(...)
bssl::ssl_cert_dup(bssl::CERT*)
SSL_new
us_internal_ssl_attach
us_socket_group_connect_resolved_dns
us_socket_group_connect
uws_sys.SocketGroup.SocketGroup.connect
runtime.socket.socket.NewSocket(true).doConnect     (is_ssl = true)
runtime.socket.Listener.connectInner
runtime.node.node_net_binding.doConnect

Analysis

  • bssl::New<CERT> crashing is the classic signature of prior heap corruption (or a UAF of the SSL_CTX's cert), not a bug in SSL_new itself.
  • Accumulation-dependent + culprit ≠ victim:
    • No single node/http file crashes when run alone.
    • Neither contiguous half crashes alone; neither quarter of the first half crashes.
    • The 4 TLS-connecting files (node-http-agent-tls-options, node-https-checkServerIdentity, node-http-with-ws, node-http.test.ts) run together are clean (exit 1).
    • ⇒ the corrupting file is a non-TLS test; the victim is a later TLS connect. They must run in the same process for the crash to surface.
  • The SSLContextCache refcounting (getOrCreate ups the ref on cache hits; ex-data free callback tombstones entries) and the C us_socket_group_connect async path (us_internal_ssl_ctx_up_ref at context.c) both look correct on inspection, so the corruption likely originates outside the TLS path.

Confirmed NOT a regression

Reproduces with the recent allocator commit (55e731ca6) reverted, i.e. it predates the current session's work.

Suggested next step

Root-causing a distant-victim heap corruption needs an ASan build (-fsanitize=address) to catch the corrupting write at its source, or MIMALLOC_GUARDED_SAMPLE_RATE/secure-mode mimalloc to guard-page allocations. Plain release/debug mimalloc here has no double-free/overflow detection (debug level 0, secure level 0).

10 days ago

test/js/node/tls reproduces the same crash — a more contained (all-TLS) repro

home-release-safe test test/js/node/tls    # → exit 134, identical backtrace

Backtrace is byte-for-byte the same (bssl::New<CERT> ← ssl_cert_dup ← SSL_new ← us_internal_ssl_attach ← us_socket_group_connect_resolved_dns), so it's the same root cause — not http-specific. Since node/tls is entirely TLS tests, both culprit and victim live here.

Bisection so far (release-safe):

  • files[0:11] clean; files[11:22] crashes.
  • files[11:16] clean alone; files[16:22] clean alone → culprit and victim straddle index 16.
  • files[11:22] minus node-tls-root-certs-concurrent-init still crashes → that race is not required.
  • node-tls-root-certs-concurrent-init + files[16:22] is clean → it is not sufficient either.

So the culprit is one of node-tls-rootcertificates-immutable / node-tls-server / node-tls-socket-allow-half-open-option / node-tls-upgrade (files 12–15), and the victim is a TLS connect in files[16:22] (renegotiation / ssl-ctx-cache / test-*-ca* / tls-connect-socket-churn). Simple contiguous bisection doesn't converge because culprit ≠ victim and it's multi-file — an ASan build is the right tool to catch the corrupting write directly.

10 days ago

It's a UAF, not an overflow. Ran the crashing set under MIMALLOC_GUARDED_SAMPLE_RATE=1 (guard page per allocation) — the crash stays at the same victim site (bssl::New<CERT> inside SSL_new), never faulting earlier at a corrupting write. So this is a use-after-free of the SSL_CTX's cert (ssl_cert_dup reads cert->x509_method from a freed CERT), most likely an SSL_CTX double-free in one of the culprit-candidate files (node-tls-rootcertificates-immutable, node-tls-server, node-tls-socket-allow-half-open-option, node-tls-upgrade) that drops a ref the SSLContextCache/socket still relies on. Next step is an ASan build or a targeted audit of SSL_CTX_free balance in the TLS server/upgrade/SecureContext teardown paths.

10 days ago

Root-caused and fixed in 610ee9ee0

It was a stale-pointer reuse in the per-VM SSLContextCache, not a double-free in the test files. My earlier bisection kept failing to converge because the culprit and victim aren't a fixed pair — any two TLS connects that share a config digest collide through the cache.

Mechanism

SSLContextCache deliberately holds zero refs on cached SSL_CTX*. Correctness depends entirely on BoringSSL's CRYPTO_EX_free callback (bun_ssl_ctx_cache_on_free) firing when a CTX's last ref drops — that's what nulls entry.ctx (tombstones the slot). The callback only fires if the entry pointer is stored at the ex_data index the callback is registered under: us_ssl_ctx_cache_ex_idx, exported by the linked openssl.c.o.

Home stubbed that getter to return 0 and never wired the real symbol:

pub var us_ssl_ctx_cache_ex_idx: *const fn () callconv(.c) c_int = stub_ex_idx; // → 0

So SSL_CTX_set_ex_data(ctx, 0, entry) wrote to index 0, which has no free callback. The tombstone never fired:

  1. Connection A opens with a cacheable TLS config → cache stores SSL_CTX* X (map holds 0 refs).
  2. A's connection deinits → SSL_CTX_free(X) → refcount 0 → X is freed, but entry.ctx still points at freed X (no tombstone).
  3. Connection B opens with the same config digest → cache hit → SSL_CTX_up_ref(freed X) → hands back freed memory → SSL_new → ssl_cert_dup reads cert->x509_method off the freed CERT → SIGABRT.

This matches every observation: same victim site under MIMALLOC_GUARDED_SAMPLE_RATE=1 (UAF, not overflow — faults at the read, not a corrupting write), and culprit ≠ victim (they're just two connects sharing a digest).

Fix

Bind the getter to the real extern (the C symbol is in the link graph — nm shows _us_ssl_ctx_cache_ex_idx as T in openssl.c.o), gated on enable_jsc_link so the standalone test build keeps the 0 fallback (it never exercises the cache-index paths). The C getter self-initializes via us_ex_idx_ensure(), so the registered index — with the tombstone callback attached — is always returned.

Verification (release-safe)

  • test/js/node/tls — was exit 134, now exit 0 (144 tests / 22 files, 120 pass).
  • test/js/node/http — was exit 134, now exit 0 (434 tests / 28 files, 411 pass).
  • No crash reports emitted. Remaining (fail)s are peer-certificate content assertions on bun-HEAD test files (634 commits past our pin), unrelated to the abort.

Closing.

Sign in to comment on this issue.