http: avoid stream listeners on idle agent sockets

Signed-off-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #64004
Fixes: #63989
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>

Author: mcollina

lib/_http_agent.js

@@ -43,12 +43,22 @@ const {
   filterEnvForProxies,
 } = require('internal/http');
 const { AsyncResource } = require('async_hooks');
-const { async_id_symbol } = require('internal/async_hooks').symbols;
+const {
+  async_id_symbol,
+  owner_symbol,
+} = require('internal/async_hooks').symbols;
 const {
   getLazy,
   kEmptyObject,
   once,
 } = require('internal/util');
+const {
+  onStreamRead,
+} = require('internal/stream_base_commons');
+const {
+  kReadBytesOrError,
+  streamBaseState,
+} = internalBinding('stream_wrap');
 const {
   validateNumber,
   validateOneOf,
@@ -60,6 +70,7 @@ const { getOptionValue } = require('internal/options');
 const kOnKeylog = Symbol('onkeylog');
 const kRequestOptions = Symbol('requestOptions');
 const kRequestAsyncResource = Symbol('requestAsyncResource');
+const kFreeSocketDataGuard = Symbol('freeSocketDataGuard');
 
 // New Agent code.
 
@@ -92,9 +103,51 @@ function freeSocketErrorListener(err) {
 // in the TCP buffer and be silently consumed as the response for the
 // *next* request that reuses the socket (response-queue poisoning).
 // See: https://hackerone.com/reports/3582376
-function freeSocketDataGuard() {
-  debug('DATA on FREE socket - destroying poisoned socket');
-  this.destroy();
+function freeSocketOnReadGuard() {
+  const nread = streamBaseState[kReadBytesOrError];
+  if (nread === 0) return;
+
+  debug('READ on FREE socket - destroying poisoned socket');
+  this[owner_symbol].destroy();
+}
+
+function installFreeSocketDataGuard(socket) {
+  if (socket.readableLength > 0) {
+    debug('BUFFERED DATA on FREE socket - destroying poisoned socket');
+    socket.destroy();
+    return;
+  }
+
+  if (socket.connecting) {
+    socket[kFreeSocketDataGuard] = function onConnect() {
+      socket[kFreeSocketDataGuard] = null;
+      installFreeSocketDataGuard(socket);
+    };
+    socket.once('connect', socket[kFreeSocketDataGuard]);
+    return;
+  }
+
+  const handle = socket._handle;
+  if (handle) {
+    handle.onread = freeSocketOnReadGuard;
+    if (!handle.reading) {
+      handle.reading = true;
+      const err = handle.readStart();
+      if (err) socket.destroy();
+    }
+  }
+}
+
+function removeFreeSocketDataGuard(socket) {
+  if (socket[kFreeSocketDataGuard]) {
+    socket.removeListener('connect', socket[kFreeSocketDataGuard]);
+    socket[kFreeSocketDataGuard] = null;
+  }
+
+  const handle = socket._handle;
+  if (handle?.onread === freeSocketOnReadGuard) {
+    handle.onread = onStreamRead;
+  }
 }
 
 function Agent(options) {
@@ -206,8 +259,7 @@ function Agent(options) {
     this.removeSocket(socket, options);
 
     socket.once('error', freeSocketErrorListener);
-    socket.on('data', freeSocketDataGuard);
-    socket.resume();
+    installFreeSocketDataGuard(socket);
     freeSockets.push(socket);
   });
 
@@ -599,7 +651,7 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) {
 Agent.prototype.reuseSocket = function reuseSocket(socket, req) {
   debug('have free socket');
   socket.removeListener('error', freeSocketErrorListener);
-  socket.removeListener('data', freeSocketDataGuard);
+  removeFreeSocketDataGuard(socket);
   req.reusedSocket = true;
   socket.ref();
 };

test/parallel/test-http-agent-free-socket-data-guard.js

@@ -8,8 +8,8 @@
 // writes a full HTTP response during this window, it is consumed as the
 // response for the *next* request — poisoning the response queue.
 //
-// The fix attaches a data guard listener + resume() on idle sockets so
-// that unsolicited data causes the socket to be destroyed.
+// The fix installs a read guard on idle sockets so that unsolicited data
+// causes the socket to be destroyed without adding public stream listeners.
 
 const common = require('../common');
 const assert = require('assert');
@@ -48,8 +48,9 @@ server.listen(0, common.mustCall(() => {
       assert.strictEqual(agent.freeSockets[name]?.length, 1);
       const freeSocket = agent.freeSockets[name][0];
       assert.strictEqual(freeSocket.parser, null);
-      // With the fix, a data guard listener is attached
-      assert.strictEqual(freeSocket.listenerCount('data'), 1);
+      // With the fix, no public stream listeners are added.
+      assert.strictEqual(freeSocket.listenerCount('data'), 0);
+      assert.strictEqual(freeSocket.listenerCount('readable'), 0);
 
       // Step 2: Server injects a poisoned response while socket is idle
       serverSocket.write(

test/parallel/test-http-agent-keepalive.js

@@ -149,8 +149,9 @@ server.listen(0, common.mustCall(() => {
 function checkListeners(socket) {
   const callback = common.mustCall(() => {
     if (!socket.destroyed) {
-      // Sockets have freeSocketDataGuard while in the free pool.
-      assert.strictEqual(socket.listenerCount('data'), 1);
+      // Sockets have no public stream guard listeners while in the free pool.
+      assert.strictEqual(socket.listenerCount('readable'), 0);
+      assert.strictEqual(socket.listenerCount('data'), 0);
       assert.strictEqual(socket.listenerCount('drain'), 0);
       // Sockets have freeSocketErrorListener.
       assert.strictEqual(socket.listenerCount('error'), 1);