| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
1 parent a67dd46 commit bc0b538
3 files changed
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -87,6 +87,16 @@ function freeSocketErrorListener(err) { | |||
| 87 | 87 | socket.emit('agentRemove'); | |
| 88 | 88 | } | |
| 89 | 89 | ||
| 90 | + // Guard against unsolicited data arriving while a socket is idle in the | ||
| 91 | + // freeSockets pool. When the HTTPParser is detached the data would sit | ||
| 92 | + // in the TCP buffer and be silently consumed as the response for the | ||
| 93 | + // *next* request that reuses the socket (response-queue poisoning). | ||
| 94 | + // See: https://hackerone.com/reports/3582376 | ||
| 95 | + function freeSocketDataGuard() { | ||
| 96 | + debug('DATA on FREE socket - destroying poisoned socket'); | ||
| 97 | + this.destroy(); | ||
| 98 | + } | ||
| 99 | + | ||
| 90 | 100 | function Agent(options) { | |
| 91 | 101 | if (!(this instanceof Agent)) | |
| 92 | 102 | return new Agent(options); | |
@@ -197,6 +207,8 @@ function Agent(options) { | |||
| 197 | 207 | this.removeSocket(socket, options); | |
| 198 | 208 | ||
| 199 | 209 | socket.once('error', freeSocketErrorListener); | |
| 210 | + socket.on('data', freeSocketDataGuard); | ||
| 211 | + socket.resume(); | ||
| 200 | 212 | freeSockets.push(socket); | |
| 201 | 213 | }); | |
| 202 | 214 | ||
@@ -588,6 +600,7 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) { | |||
| 588 | 600 | Agent.prototype.reuseSocket = function reuseSocket(socket, req) { | |
| 589 | 601 | debug('have free socket'); | |
| 590 | 602 | socket.removeListener('error', freeSocketErrorListener); | |
| 603 | + socket.removeListener('data', freeSocketDataGuard); | ||
| 591 | 604 | req.reusedSocket = true; | |
| 592 | 605 | socket.ref(); | |
| 593 | 606 | }; | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -0,0 +1,90 @@ | |||
| 1 | + 'use strict'; | ||
| 2 | + | ||
| 3 | + // Regression test for HackerOne report #3582376 | ||
| 4 | + // HTTP Response Queue Poisoning via TOCTOU Race Condition in http.Agent | ||
| 5 | + // | ||
| 6 | + // When keepAlive is true, there is a window between a socket entering the | ||
| 7 | + // freeSockets pool (parser detached) and being reassigned. If the server | ||
| 8 | + // writes a full HTTP response during this window, it is consumed as the | ||
| 9 | + // response for the *next* request — poisoning the response queue. | ||
| 10 | + // | ||
| 11 | + // The fix attaches a data guard listener + resume() on idle sockets so | ||
| 12 | + // that unsolicited data causes the socket to be destroyed. | ||
| 13 | + | ||
| 14 | + const common = require('../common'); | ||
| 15 | + const assert = require('assert'); | ||
| 16 | + const http = require('http'); | ||
| 17 | + | ||
| 18 | + let serverSocket; | ||
| 19 | + | ||
| 20 | + const server = http.createServer(common.mustCall((req, res) => { | ||
| 21 | + // Capture the raw socket on the first request | ||
| 22 | + serverSocket ||= req.socket; | ||
| 23 | + res.end(req.url); | ||
| 24 | + }, 2)); // Expect request1 and request2 | ||
| 25 | + | ||
| 26 | + server.listen(0, common.mustCall(() => { | ||
| 27 | + const agent = new http.Agent({ keepAlive: true }); | ||
| 28 | + const options = { host: '127.0.0.1', port: server.address().port, agent }; | ||
| 29 | + const name = agent.getName(options); | ||
| 30 | + | ||
| 31 | + // Step 1: Send request1 | ||
| 32 | + const request1 = http.request({ ...options, path: '/request1' }); | ||
| 33 | + request1.end(); | ||
| 34 | + | ||
| 35 | + request1.on('response', common.mustCall((response) => { | ||
| 36 | + let body = ''; | ||
| 37 | + response.setEncoding('utf8'); | ||
| 38 | + response.on('data', (data) => { body += data; }); | ||
| 39 | + response.on('end', common.mustCall(() => { | ||
| 40 | + assert.strictEqual(body, '/request1'); | ||
| 41 | + })); | ||
| 42 | + })); | ||
| 43 | + | ||
| 44 | + request1.on('close', common.mustCall(() => { | ||
| 45 | + // Use nextTick to ensure socket is in freeSockets | ||
| 46 | + process.nextTick(common.mustCall(() => { | ||
| 47 | + // Verify the socket is in the free pool with parser detached | ||
| 48 | + assert.strictEqual(agent.freeSockets[name]?.length, 1); | ||
| 49 | + const freeSocket = agent.freeSockets[name][0]; | ||
| 50 | + assert.strictEqual(freeSocket.parser, null); | ||
| 51 | + // With the fix, a data guard listener is attached | ||
| 52 | + assert.strictEqual(freeSocket.listenerCount('data'), 1); | ||
| 53 | + | ||
| 54 | + // Step 2: Server injects a poisoned response while socket is idle | ||
| 55 | + serverSocket.write( | ||
| 56 | + 'HTTP/1.1 200 OK\r\n' + | ||
| 57 | + 'X-Poisoned: true\r\n' + | ||
| 58 | + 'Connection: keep-alive\r\n' + | ||
| 59 | + 'Content-Length: 0\r\n' + | ||
| 60 | + '\r\n' | ||
| 61 | + ); | ||
| 62 | + | ||
| 63 | + // Step 3: Allow the event loop to poll I/O so the guard can fire. | ||
| 64 | + // In a real attack, there is always time between the poison arriving | ||
| 65 | + // and the next client request. setTimeout(0) runs after the I/O poll | ||
| 66 | + // phase, giving the guard a chance to receive the poisoned data. | ||
| 67 | + setTimeout(common.mustCall(() => { | ||
| 68 | + // The guard should have destroyed the poisoned socket | ||
| 69 | + assert.strictEqual(freeSocket.destroyed, true); | ||
| 70 | + assert.strictEqual(agent.freeSockets[name], undefined); | ||
| 71 | + | ||
| 72 | + // Step 4: Send request2 — should get a fresh connection | ||
| 73 | + const request2 = http.request({ ...options, path: '/request2' }); | ||
| 74 | + request2.end(); | ||
| 75 | + | ||
| 76 | + request2.on('response', common.mustCall((response) => { | ||
| 77 | + let body = ''; | ||
| 78 | + response.setEncoding('utf8'); | ||
| 79 | + response.on('data', (data) => { body += data; }); | ||
| 80 | + response.on('end', common.mustCall(() => { | ||
| 81 | + assert.strictEqual(response.headers['x-poisoned'], undefined); | ||
| 82 | + assert.strictEqual(body, '/request2'); | ||
| 83 | + agent.destroy(); | ||
| 84 | + server.close(); | ||
| 85 | + })); | ||
| 86 | + })); | ||
| 87 | + }), 50); | ||
| 88 | + })); | ||
| 89 | + })); | ||
| 90 | + })); | ||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -149,7 +149,8 @@ server.listen(0, common.mustCall(() => { | |||
| 149 | 149 | function checkListeners(socket) { | |
| 150 | 150 | const callback = common.mustCall(() => { | |
| 151 | 151 | if (!socket.destroyed) { | |
| 152 | - assert.strictEqual(socket.listenerCount('data'), 0); | ||
| 152 | + // Sockets have freeSocketDataGuard while in the free pool. | ||
| 153 | + assert.strictEqual(socket.listenerCount('data'), 1); | ||
| 153 | 154 | assert.strictEqual(socket.listenerCount('drain'), 0); | |
| 154 | 155 | // Sockets have freeSocketErrorListener. | |
| 155 | 156 | assert.strictEqual(socket.listenerCount('error'), 1); | |
| Back | FazBrowse Home | New Git URL |
0 commit comments