Skip to content

fix(ext/node): fix TLS socket error ordering and app data leak before identity verification#33585

Merged
bartlomieju merged 3 commits intomainfrom
fix/tls-socket-error-ordering
Apr 27, 2026
Merged

fix(ext/node): fix TLS socket error ordering and app data leak before identity verification#33585
bartlomieju merged 3 commits intomainfrom
fix/tls-socket-error-ordering

Conversation

@bartlomieju
Copy link
Copy Markdown
Member

Summary

  • Error/close ordering: In Socket._destroy, call cb(exception) before
    handle.close() so the stream's error emission (via nextTick) is scheduled
    before the handle close callback. Also switch TLSWrap.close from
    queueMicrotask to nextTick -- inside native callbacks, queueMicrotask
    fires before process.nextTick in Deno, causing close to fire before
    error which made the HTTP client emit a spurious ECONNRESET.
  • TLS app data leak: Gate clear_in() on established && !closing so
    application data buffered before the TLS handshake is not fed to rustls
    until after the JS onhandshakedone callback confirms the connection.
    In Node.js, OpenSSL fires the info callback synchronously during
    SSL_read, giving checkServerIdentity a chance to reject before any
    app data is encrypted and sent. Adds a setClosing() op that JS
    TLSWrap.close() calls synchronously to signal the Rust side. After
    the handshake callback, pending cleartext is only drained if the
    connection was not closed.

Fixes 3 node_compat tests:

  • test-https-client-checkServerIdentity
  • test-https-simple
  • test-https-strict

Test plan

  • ./x test-compat test-https-client-checkServerIdentity.js passes
  • ./x test-compat test-https-simple.js passes
  • ./x test-compat test-https-strict.js passes
  • ./tools/lint.js clean
  • ./tools/format.js clean

In native callback context, queueMicrotask fires before
process.nextTick in Deno, causing the socket 'close' event to fire
before 'error'. This causes the HTTP client's socketCloseListener to
emit a spurious ECONNRESET before the real TLS error.

Fix: call cb(exception) before handle.close() in Socket._destroy so
the error emission is scheduled first, and use nextTick instead of
queueMicrotask in TLSWrap.close.
Gate clear_in() on established && !closing so application data
buffered before the TLS handshake is not fed to rustls until after
the JS onhandshakedone callback confirms the connection. This matches
Node.js where OpenSSL fires the info callback synchronously during
SSL_read, giving checkServerIdentity a chance to reject before any
app data is encrypted and sent.

Adds a setClosing() op that JS TLSWrap.close() calls synchronously
to signal the Rust side. After the handshake callback, pending
cleartext is only drained if the connection was not closed.
Copy link
Copy Markdown
Contributor

@fibibot fibibot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Three coordinated changes that solve real bugs:

1. Socket._destroy reorder (net.ts) — capturing handle locally, nulling _handle/_sockname, then calling cb(exception) before handle.close(closeCb) correctly schedules the stream's nextTick error emission ahead of the close callback. The reordering is observably equivalent to Node's pattern (Node calls _handle.close(cb) first then cb(err) last but still gets error-before-close because close's I/O is async). Local-handle capture keeps the close callback safe even after this._handle = null. Verified nothing in the synchronous path between cb() and handle.close() reads this._handle.

2. TLSWrap.close queueMicrotask → nextTick (_tls_wrap.js) — exactly right. In Deno when called from inside a native callback, microtasks drain before nextTick, which inverts Node's ordering and was making close fire before error (the ECONNRESET symptom). nextTick puts the close callback in the same queue as the stream's error tick, preserving the documented Node order.

3. TLS app data leak gate (tls_wrap.rs) — the security-relevant fix. Buffered cleartext written before the handshake completes was being drained into rustls and sent on the wire even if checkServerIdentity rejected the connection synchronously in onhandshakedone. Gating clear_in() on established && !closing, and gating the post-handshake drain in do_enc_in_action_callback on !(*ptr).closing after the JS callback returns, gives the same semantic as Node's OpenSSL info-callback ordering (verify-before-encrypt). The JS-side setClosing() op called synchronously from TLSWrap.close() ensures the flag is set before the post-handshake drain runs.

The unsafe &mut *self.inner.as_mut_ptr() in set_closing is consistent with the existing pattern in this file and safe in practice — JS is single-threaded, the only re-entrant access is during the JS callback (where the original ptr is on the stack), and the lifetimes of the materialized &muts don't overlap.

CI fully green across 110 checks.

Two non-blocking notes inline.

Comment thread ext/node/ops/tls_wrap.rs
Object.setPrototypeOf(TLSSocket, net.Socket);

tlsWrap.TLSWrap.prototype.close = function close(cb) {
// Signal to Rust that this TLSWrap is being closed, so it won't
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick sanity-check question — is TLSWrap.close() the only path where the socket can be destroyed after a failed handshake-callback identity check?

Reading the flow: JS onhandshakedoneonConnectSecurecheckServerIdentity throws → caught somewhere → socket.destroy(err)Socket._destroythis._handle.close(cb) where _handle is the TLSWrap → enters this close method → setClosing().

Is that the only path? If JS rejects via some other mechanism (e.g., a 'secureConnect' listener that calls socket.destroy() outside the onConnectSecure handler, or an explicit tlsSocket.destroy() from user code) does that still go through TLSWrap.close() synchronously, or could it bypass and leave closing: false while the post-handshake drain runs?

Not blocking — the checkServerIdentity test should exercise the documented path — but worth confirming there's no alternative destroy entry point that skips setClosing().

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes -- TLSWrap.close() is the only path. Every destroy route goes through Socket._destroy which calls this._handle.close(cb), and for a TLSSocket, _handle is always the TLSWrap instance, so it always dispatches to TLSWrap.prototype.close which calls setClosing() synchronously before anything else.

Even the user-code case (socket.destroy() from a 'secureConnect' listener or anywhere else) goes through the same Writable.destroy -> _destroy -> _handle.close() path -- there's no alternative entry point that could skip it.

_destroySsl() exists separately but it only calls destroySsl() (Rust teardown()), not close() -- and it's only called from TLSWrap.prototype.close itself (via the done callback) or from _tlsError cleanup, never as a standalone destroy path.

@bartlomieju bartlomieju merged commit 9f06363 into main Apr 27, 2026
112 checks passed
@bartlomieju bartlomieju deleted the fix/tls-socket-error-ordering branch April 27, 2026 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants