Skip to content

Commit ffac8fd

Browse files
committed
net: reject immutable onread buffers
reject immutable buffers supplied directly or returned by an onread buffer factory before using them for socket reads. emit ERR_INVALID_ARG_VALUE without modifying the backing store. Refs: #66346 Signed-off-by: Fatih Güzel <fatihguzeldev@gmail.com>
1 parent 2aef805 commit ffac8fd

6 files changed

Lines changed: 283 additions & 4 deletions

File tree

‎lib/internal/stream_base_commons.js‎

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,12 @@ const {
1515
streamBaseState,
1616
} = internalBinding('stream_wrap');
1717
const { UV_EOF } = internalBinding('uv');
18+
const { isArrayBufferViewImmutable } = internalBinding('util');
1819
const {
1920
ErrnoException,
21+
codes: {
22+
ERR_INVALID_ARG_VALUE,
23+
},
2024
} = require('internal/errors');
2125
const { owner_symbol } = require('internal/async_hooks').symbols;
2226
const {
@@ -168,6 +172,25 @@ function afterWriteDispatched(handle, ret, buffer, cb) {
168172
return req;
169173
}
170174

175+
function onUserBufferError(stream, buffer) {
176+
stream.destroy(new ERR_INVALID_ARG_VALUE(
177+
'onread.buffer', buffer, 'must not be backed by an immutable ArrayBuffer'));
178+
}
179+
180+
function setUserBuffer(stream, buffer, deferError = false) {
181+
if (isArrayBufferViewImmutable(buffer)) {
182+
if (deferError) {
183+
// Error formatting can invoke user code, so initialization must finish.
184+
process.nextTick(onUserBufferError, stream, buffer);
185+
} else {
186+
onUserBufferError(stream, buffer);
187+
}
188+
return false;
189+
}
190+
stream[kBuffer] = buffer;
191+
return true;
192+
}
193+
171194
function onStreamRead(arrayBuffer) {
172195
const nread = streamBaseState[kReadBytesOrError];
173196

@@ -185,8 +208,10 @@ function onStreamRead(arrayBuffer) {
185208
const bufGen = stream[kBufferGen];
186209
if (bufGen !== null) {
187210
const nextBuf = bufGen();
188-
if (isUint8Array(nextBuf))
189-
stream[kBuffer] = ret = nextBuf;
211+
if (isUint8Array(nextBuf)) {
212+
if (!setUserBuffer(stream, nextBuf)) return;
213+
ret = nextBuf;
214+
}
190215
}
191216
} else {
192217
const offset = streamBaseState[kArrayBufferOffset];
@@ -280,6 +305,7 @@ module.exports = {
280305
writevGeneric,
281306
writeGeneric,
282307
onStreamRead,
308+
setUserBuffer,
283309
kAfterAsyncWrite,
284310
kMaybeDestroy,
285311
kUpdateTimer,

‎lib/net.js‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ const {
9393
writevGeneric,
9494
writeGeneric,
9595
onStreamRead,
96+
setUserBuffer,
9697
kAfterAsyncWrite,
9798
kHandle,
9899
kUpdateTimer,
@@ -357,9 +358,9 @@ function initSocketHandle(self) {
357358
userBuf = bufGen();
358359
if (!isUint8Array(userBuf))
359360
return;
360-
self[kBuffer] = userBuf;
361361
}
362-
self._handle.useUserBuffer(userBuf);
362+
if (setUserBuffer(self, userBuf, true))
363+
self._handle.useUserBuffer(userBuf);
363364
}
364365
}
365366
}

‎src/node_util.cc‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,14 @@ void ArrayBufferViewHasBuffer(const FunctionCallbackInfo<Value>& args) {
188188
args.GetReturnValue().Set(args[0].As<ArrayBufferView>()->HasBuffer());
189189
}
190190

191+
static void IsArrayBufferViewImmutable(
192+
const FunctionCallbackInfo<Value>& args) {
193+
CHECK(args[0]->IsArrayBufferView());
194+
Local<ArrayBufferView> view = args[0].As<ArrayBufferView>();
195+
// On-heap views cannot have immutable backing storage.
196+
args.GetReturnValue().Set(view->HasBuffer() && view->Buffer()->IsImmutable());
197+
}
198+
191199
static uint32_t GetUVHandleTypeCode(const uv_handle_type type) {
192200
// TODO(anonrig): We can use an enum here and then create the array in the
193201
// binding, which will remove the hard-coding in C++ and JS land.
@@ -508,6 +516,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
508516
registry->Register(GetExternalValue);
509517
registry->Register(Sleep);
510518
registry->Register(ArrayBufferViewHasBuffer);
519+
registry->Register(IsArrayBufferViewImmutable);
511520
registry->Register(GuessHandleType);
512521
registry->Register(fast_guess_handle_type_);
513522
registry->Register(ParseEnv);
@@ -637,6 +646,10 @@ void Initialize(Local<Object> target,
637646
SetMethod(context, target, "parseEnv", ParseEnv);
638647
SetMethod(
639648
context, target, "arrayBufferViewHasBuffer", ArrayBufferViewHasBuffer);
649+
SetMethodNoSideEffect(context,
650+
target,
651+
"isArrayBufferViewImmutable",
652+
IsArrayBufferViewImmutable);
640653
SetMethod(context,
641654
target,
642655
"constructSharedArrayBuffer",
Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
// Flags: --js-immutable-arraybuffer
2+
'use strict';
3+
4+
const common = require('../common');
5+
const assert = require('assert');
6+
const net = require('net');
7+
const { once } = require('events');
8+
const { test } = require('node:test');
9+
10+
if (typeof ArrayBuffer.prototype.transferToImmutable !== 'function')
11+
common.skip('ArrayBuffer.prototype.transferToImmutable is not available');
12+
13+
const message = Buffer.from('test');
14+
15+
async function createServer(t) {
16+
const connections = [];
17+
const server = net.createServer((socket) => {
18+
connections.push(socket);
19+
// Rejecting a replacement buffer may close the client with unread data.
20+
socket.on('error', (err) => {
21+
if (err.code !== 'ECONNRESET') throw err;
22+
});
23+
socket.end(message);
24+
});
25+
t.after(async () => {
26+
for (const socket of connections) socket.destroy();
27+
await new Promise((resolve) => server.close(resolve));
28+
});
29+
server.listen(0, '127.0.0.1');
30+
await once(server, 'listening');
31+
return server.address().port;
32+
}
33+
34+
for (const View of [Buffer, Uint8Array]) {
35+
for (const offset of [0, 1]) {
36+
for (const mode of ['direct', 'first', 'replacement']) {
37+
test(`${View.name}, offset ${offset}, ${mode} immutable buffer`, {
38+
timeout: common.platformTimeout(5000),
39+
}, async (t) => {
40+
const original = [0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff];
41+
const backing = Uint8Array.from(original).buffer.transferToImmutable();
42+
const target = View === Buffer ?
43+
Buffer.from(backing, offset, 4) : new Uint8Array(backing, offset, 4);
44+
const mutable = new Uint8Array(1);
45+
let generated = 0;
46+
const factory = () => {
47+
generated++;
48+
return mode === 'replacement' && generated === 1 ? mutable : target;
49+
};
50+
const reads = [];
51+
const errors = [];
52+
const socket = new net.Socket({
53+
onread: {
54+
buffer: mode === 'direct' ? target : factory,
55+
callback(nread, buffer) {
56+
reads.push({ buffer, bytes: [...buffer.subarray(0, nread)] });
57+
},
58+
},
59+
});
60+
t.after(() => socket.destroy());
61+
socket.on('error', (err) => errors.push(err));
62+
socket.on('close', common.mustCall());
63+
socket.on('data', common.mustNotCall());
64+
const closed = new Promise((resolve) => socket.once('close', resolve));
65+
const port = await createServer(t);
66+
socket.connect(port, '127.0.0.1');
67+
await closed;
68+
69+
assert.strictEqual(errors.length, 1);
70+
assert.strictEqual(errors[0].name, 'TypeError');
71+
assert.strictEqual(errors[0].code, 'ERR_INVALID_ARG_VALUE');
72+
assert.match(errors[0].message, /onread\.buffer.*immutable ArrayBuffer/);
73+
assert.strictEqual(socket.connecting, false);
74+
assert.strictEqual(backing.immutable, true);
75+
assert.deepStrictEqual([...new Uint8Array(backing)], original);
76+
if (mode === 'replacement') {
77+
assert.strictEqual(generated, 2);
78+
assert.strictEqual(reads.length, 1);
79+
assert.strictEqual(reads[0].buffer, mutable);
80+
assert.deepStrictEqual(reads[0].bytes, [...message.subarray(0, 1)]);
81+
} else {
82+
assert.strictEqual(generated, mode === 'first' ? 1 : 0);
83+
assert.deepStrictEqual(reads, []);
84+
}
85+
});
86+
}
87+
}
88+
}
89+
90+
for (const kind of ['bound', 'retry']) {
91+
test(`reject an immutable buffer during ${kind} initialization`, {
92+
timeout: common.platformTimeout(5000),
93+
}, async (t) => {
94+
const port = await createServer(t);
95+
const backing = new ArrayBuffer(4).transferToImmutable();
96+
const target = new Uint8Array(backing);
97+
let generated = 0;
98+
const options = {
99+
host: '127.0.0.1',
100+
port,
101+
onread: {
102+
buffer: target,
103+
callback: common.mustNotCall(),
104+
},
105+
};
106+
let socket;
107+
if (kind === 'bound') {
108+
options.handle = new net.BoundSocket({ host: '127.0.0.1' });
109+
socket = net.connect(options);
110+
} else {
111+
options.host = 'localhost';
112+
options.autoSelectFamily = true;
113+
options.autoSelectFamilyAttemptTimeout = common.platformTimeout(10);
114+
options.lookup = (host, opts, callback) => callback(null, [
115+
{ address: '127.0.0.2', family: 4 },
116+
{ address: '127.0.0.1', family: 4 },
117+
]);
118+
options.onread.buffer = () => (++generated === 1 ? new Uint8Array(4) : target);
119+
socket = net.connect(options).unref();
120+
}
121+
t.after(() => socket.destroy());
122+
socket.on('close', common.mustCall());
123+
socket.on('error', common.expectsError({
124+
name: 'TypeError',
125+
code: 'ERR_INVALID_ARG_VALUE',
126+
}));
127+
await new Promise((resolve) => socket.once('close', resolve));
128+
assert.deepStrictEqual([...target], [0, 0, 0, 0]);
129+
if (kind === 'retry') assert.strictEqual(generated, 2);
130+
});
131+
}
132+
133+
for (const shared of [false, true]) {
134+
test(`mutable ${shared ? 'shared' : 'on-heap'} replacement buffers`, {
135+
timeout: common.platformTimeout(5000),
136+
}, async (t) => {
137+
const chunks = [];
138+
const socket = new net.Socket({
139+
onread: {
140+
buffer: () => (shared ?
141+
new Uint8Array(new SharedArrayBuffer(1)) : new Uint8Array(1)),
142+
callback(nread, buffer) {
143+
chunks.push(Buffer.from(buffer.subarray(0, nread)));
144+
},
145+
},
146+
});
147+
t.after(() => socket.destroy());
148+
socket.on('error', common.mustNotCall());
149+
const closed = new Promise((resolve) => socket.once('close', resolve));
150+
const port = await createServer(t);
151+
socket.connect(port, '127.0.0.1');
152+
await closed;
153+
assert.deepStrictEqual(Buffer.concat(chunks), message);
154+
});
155+
}
Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
// Flags: --js-immutable-arraybuffer
2+
'use strict';
3+
4+
const common = require('../common');
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
8+
const assert = require('assert');
9+
const tls = require('tls');
10+
const { once } = require('events');
11+
const { test } = require('node:test');
12+
const fixtures = require('../common/fixtures');
13+
14+
if (typeof ArrayBuffer.prototype.transferToImmutable !== 'function')
15+
common.skip('ArrayBuffer.prototype.transferToImmutable is not available');
16+
17+
for (const factory of [false, true]) {
18+
test(`immutable ${factory ? 'generated' : 'static'} TLS onread buffer`, {
19+
timeout: common.platformTimeout(5000),
20+
}, async (t) => {
21+
const backing = new ArrayBuffer(4).transferToImmutable();
22+
const target = new Uint8Array(backing);
23+
const socket = new tls.TLSSocket(undefined, {
24+
onread: {
25+
buffer: factory ? () => target : target,
26+
callback: common.mustNotCall(),
27+
},
28+
});
29+
t.after(() => socket.destroy());
30+
socket.on('error', common.expectsError({
31+
name: 'TypeError',
32+
code: 'ERR_INVALID_ARG_VALUE',
33+
}));
34+
socket.on('close', common.mustCall());
35+
await new Promise((resolve) => socket.once('close', resolve));
36+
assert.deepStrictEqual([...target], [0, 0, 0, 0]);
37+
});
38+
}
39+
40+
test('reject an immutable TLS replacement buffer', {
41+
timeout: common.platformTimeout(5000),
42+
}, async (t) => {
43+
const backing = new ArrayBuffer(4).transferToImmutable();
44+
const target = new Uint8Array(backing);
45+
const mutable = new Uint8Array(1);
46+
const connections = [];
47+
let socket;
48+
const server = tls.createServer({
49+
key: fixtures.readKey('agent2-key.pem'),
50+
cert: fixtures.readKey('agent2-cert.pem'),
51+
}, (socket) => {
52+
connections.push(socket);
53+
socket.on('error', (err) => {
54+
if (err.code !== 'ECONNRESET') throw err;
55+
});
56+
socket.end('test');
57+
});
58+
t.after(async () => {
59+
socket?.destroy();
60+
for (const socket of connections) socket.destroy();
61+
await new Promise((resolve) => server.close(resolve));
62+
});
63+
server.listen(0, '127.0.0.1');
64+
await once(server, 'listening');
65+
let generated = 0;
66+
socket = tls.connect({
67+
host: '127.0.0.1',
68+
port: server.address().port,
69+
rejectUnauthorized: false,
70+
onread: {
71+
buffer: common.mustCall(() => (++generated === 1 ? mutable : target), 2),
72+
callback: common.mustCall((nread, buffer) => {
73+
assert.strictEqual(buffer, mutable);
74+
assert.strictEqual(nread, 1);
75+
assert.strictEqual(buffer[0], 't'.charCodeAt(0));
76+
}),
77+
},
78+
});
79+
socket.on('error', common.expectsError({ code: 'ERR_INVALID_ARG_VALUE' }));
80+
socket.on('close', common.mustCall());
81+
await new Promise((resolve) => socket.once('close', resolve));
82+
assert.deepStrictEqual([...target], [0, 0, 0, 0]);
83+
});

‎typings/internalBinding/util.d.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ export interface UtilBinding {
3232
sleep(msec: number): void;
3333
isConstructor(fn: Function): boolean;
3434
arrayBufferViewHasBuffer(view: ArrayBufferView): boolean;
35+
isArrayBufferViewImmutable(view: ArrayBufferView): boolean;
3536
propertyFilter: {
3637
ALL_PROPERTIES: 0;
3738
ONLY_WRITABLE: 1;

0 commit comments

Comments
 (0)