Skip to content

Commit 22ba585

Browse files
mcollinaaduh95
mcollina
authored and
aduh95
committed
tls,http: prevent nested parser execution from empty writes
An empty write on a TLS socket calls `ClearOut()` directly, even when it comes from JS that runs inside `Cycle()`, for example the HTTP parser's `on_body`. `ClearOut()` then decrypts the next record and passes it to the same listener while it is still processing the current one. For an HTTPS server this runs `llhttp_execute()` inside itself. The nested call can stop in the middle of a body chunk and leave the open span pointing into its own buffer, which is freed when it returns. The outer call then reports that span against its own buffer, so the request body contains server heap memory, or the process aborts. Skip the read on an empty write while a cycle is running. The cycle reads the next record once the listener has returned. In the HTTP parser, re-entrant `Execute()` calls remain supported, but each call now restores the caller's buffer when it returns, and data callbacks fail with `HPE_REENTRANT_EXECUTE` when they point outside the buffer that is being executed. Refs: https://hackerone.com/reports/4150886 Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: nodejs-private/node-private#1021 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Tim Perry <pimterry@gmail.com> CVE-ID: CVE-2026-123864
1 parent 0eebafb commit 22ba585

3 files changed

Lines changed: 163 additions & 5 deletions

File tree

‎src/crypto/crypto_tls.cc‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1175,7 +1175,11 @@ int TLSWrap::DoWrite(WriteWrap* w,
11751175
// WriteWrap was accepted by the stream, or that we call Done() on it.
11761176
if (length == 0) {
11771177
Debug(this, "Empty write");
1178-
ClearOut();
1178+
// When the write comes from JS running inside Cycle(), for example from a
1179+
// stream listener in ClearOut(), the cycle reads and encrypts any pending
1180+
// data once that JS returns. Reading here would hand the next record to
1181+
// the listener while it is still processing the current one.
1182+
if (cycle_depth_ == 0) ClearOut();
11791183
if (BIO_pending(enc_out_) == 0) {
11801184
Debug(this, "No pending encrypted output, writing to underlying stream");
11811185
CHECK(!current_empty_write_);

‎src/node_http_parser.cc‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -888,19 +888,26 @@ class Parser : public AsyncWrap, public StreamListener {
888888
return;
889889

890890
// Hooks for GetCurrentBuffer
891+
const char* const outer_data = current_buffer_data_;
892+
const size_t outer_len = current_buffer_len_;
891893
current_buffer_len_ = nread;
892894
current_buffer_data_ = buf.base;
893895

894896
MakeCallback(cb.As<Function>(), 1, &ret);
895897

896-
current_buffer_len_ = 0;
897-
current_buffer_data_ = nullptr;
898+
current_buffer_len_ = outer_len;
899+
current_buffer_data_ = outer_data;
898900
}
899901

900902

901903
Local<Value> Execute(const char* data, size_t len) {
902904
EscapableHandleScope scope(env()->isolate());
903905

906+
// A callback can run JS that executes the parser again, for example by
907+
// pushing data into the socket synchronously. Restore the caller's buffer
908+
// afterwards so that its own callbacks are checked against it.
909+
const char* const outer_data = current_buffer_data_;
910+
const size_t outer_len = current_buffer_len_;
904911
current_buffer_len_ = len;
905912
current_buffer_data_ = data;
906913
got_exception_ = false;
@@ -935,8 +942,8 @@ class Parser : public AsyncWrap, public StreamListener {
935942
llhttp_pause(&parser_);
936943
}
937944

938-
current_buffer_len_ = 0;
939-
current_buffer_data_ = nullptr;
945+
current_buffer_len_ = outer_len;
946+
current_buffer_data_ = outer_data;
940947

941948
// If there was an exception in one of the callbacks
942949
if (got_exception_)
@@ -1180,6 +1187,11 @@ class Parser : public AsyncWrap, public StreamListener {
11801187
if (parser->is_being_freed_) {
11811188
return 0;
11821189
}
1190+
if (!parser->IsInCurrentBuffer(args...)) {
1191+
llhttp_set_error_reason(
1192+
p, "HPE_REENTRANT_EXECUTE:Parser re-entered in the middle of data");
1193+
return HPE_USER;
1194+
}
11831195
int rv = (parser->*Member)(std::forward<Args>(args)...);
11841196
if (rv == 0) {
11851197
rv = parser->MaybePause();
@@ -1188,6 +1200,20 @@ class Parser : public AsyncWrap, public StreamListener {
11881200
}
11891201
};
11901202

1203+
// A nested Execute() that stops in the middle of a span (e.g. a chunk of the
1204+
// body) leaves llhttp pointing into the nested call's buffer, which can be
1205+
// freed by the time the outer call reports the span. Data callbacks must
1206+
// only ever see the buffer that is being executed.
1207+
bool IsInCurrentBuffer() const {
1208+
return true;
1209+
}
1210+
bool IsInCurrentBuffer(const char* at, size_t length) const {
1211+
const uintptr_t begin = reinterpret_cast<uintptr_t>(current_buffer_data_);
1212+
const uintptr_t pos = reinterpret_cast<uintptr_t>(at);
1213+
return pos >= begin && pos - begin <= current_buffer_len_ &&
1214+
length <= current_buffer_len_ - (pos - begin);
1215+
}
1216+
11911217
typedef int (Parser::*Call)();
11921218
typedef int (Parser::*DataCall)(const char* at, size_t length);
11931219

‎test/parallel/test-https-empty-write-nested-parser-execute.js‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
// A zero-length write on a TLS socket used to read the next TLS record
8+
// synchronously. When that write came from JavaScript running inside the
9+
// HTTP parser's `on_body` callback, the parser was executed re-entrantly and
10+
// the request body included memory that the client never sent.
11+
//
12+
// Request 1 is a HEAD request whose response headers are flushed early, so
13+
// `res.end('done')` writes '' to the socket. Its 'end' handler runs from the
14+
// tick queue while the body of request 2 is being parsed, with the next TLS
15+
// record already received.
16+
17+
const assert = require('assert');
18+
const https = require('https');
19+
const net = require('net');
20+
const tls = require('tls');
21+
const { Duplex } = require('stream');
22+
const fixtures = require('../common/fixtures');
23+
24+
// Holds back writes so that several TLS records reach the server in a single
25+
// TCP write.
26+
class Batch extends Duplex {
27+
constructor(socket) {
28+
super();
29+
this.socket = socket;
30+
this.hold = false;
31+
this.queue = [];
32+
socket.on('data', (chunk) => this.push(chunk));
33+
socket.on('end', () => this.push(null));
34+
socket.on('close', () => this.destroy());
35+
}
36+
_read() {}
37+
_write(chunk, encoding, callback) {
38+
if (this.hold)
39+
this.queue.push(chunk);
40+
else
41+
this.socket.write(chunk);
42+
callback();
43+
}
44+
_final(callback) {
45+
this.socket.end(callback);
46+
}
47+
_destroy(err, callback) {
48+
this.socket.destroy();
49+
callback(err);
50+
}
51+
flush() {
52+
this.hold = false;
53+
this.socket.write(Buffer.concat(this.queue));
54+
this.queue = [];
55+
}
56+
}
57+
58+
const options = {
59+
key: fixtures.readKey('agent1-key.pem'),
60+
cert: fixtures.readKey('agent1-cert.pem'),
61+
};
62+
63+
// Sends request 1, then the rest of it plus the start of request 2 and
64+
// `record` as two TLS records in one TCP write, then `tail`.
65+
function test(record, tail, check) {
66+
return new Promise((resolve) => {
67+
const bodies = [];
68+
const server = https.createServer(options, (req, res) => {
69+
res.flushHeaders();
70+
const chunks = [];
71+
bodies.push(chunks);
72+
req.on('data', (chunk) => chunks.push(chunk));
73+
req.on('error', () => {});
74+
req.on('end', () => res.end('done'));
75+
});
76+
77+
server.listen(0, common.mustCall(() => {
78+
const batch = new Batch(net.connect(server.address().port));
79+
const socket = tls.connect({ socket: batch, rejectUnauthorized: false },
80+
common.mustCall(() => {
81+
socket.write('HEAD / HTTP/1.1\r\nHost: x\r\n' +
82+
'Transfer-Encoding: chunked\r\n\r\n');
83+
}));
84+
let response = '';
85+
socket.setEncoding('utf8');
86+
socket.on('data', (chunk) => {
87+
response += chunk;
88+
if (response.endsWith('\r\n4\r\ndone\r\n0\r\n\r\n'))
89+
socket.end();
90+
});
91+
socket.on('error', () => {});
92+
socket.once('data', common.mustCall(() => {
93+
batch.hold = true;
94+
socket.write('0\r\n\r\nPOST / HTTP/1.1\r\nHost: x\r\n' +
95+
'Transfer-Encoding: chunked\r\n\r\n1\r\nZ', common.mustCall(() => {
96+
socket.write(record, common.mustCall(() => {
97+
batch.flush();
98+
if (tail)
99+
socket.write(tail);
100+
}));
101+
}));
102+
}));
103+
socket.on('close', common.mustCall(() => {
104+
server.close();
105+
check(bodies.map((chunks) => Buffer.concat(chunks).toString('latin1')),
106+
response);
107+
resolve();
108+
}));
109+
}));
110+
});
111+
}
112+
113+
(async () => {
114+
// A valid request 2: its body arrives intact and both requests get a
115+
// response.
116+
await test('\r\n5\r\nHE', 'LLO\r\n0\r\n\r\n', (bodies, response) => {
117+
assert.deepStrictEqual(bodies, ['', 'ZHELLO']);
118+
assert.strictEqual(response.split('HTTP/1.1 200 OK\r\n').length, 3);
119+
assert.strictEqual(response.split('done').length, 2);
120+
});
121+
122+
// The chunk 'Z' is not followed by CRLF, so request 2 is a parse error. The
123+
// body must not contain anything but the byte that was sent.
124+
await test('5\r\nHE', null, (bodies, response) => {
125+
assert.deepStrictEqual(bodies, ['', 'Z']);
126+
assert.match(response, /^HTTP\/1\.1 200 OK\r\n/);
127+
});
128+
})().then(common.mustCall());

0 commit comments

Comments
 (0)