mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(plugins): stop three ways a hostile archive or child could stall the host
All three sit in code whose whole job is to survive a plugin that misbehaves. readTarGz trusted the tar size field, which parseInt reads with a leading minus. At -512 the cursor advance cancels out and below it goes backwards, so the same header is read forever while every pass appends a member — a 70-byte upload pins the main thread and exhausts the heap, synchronously, taking HTTP, the socket and the database with it. The entry budgets never get a say: they run on the list readTarGz has to finish building first. Sideload is the reachable path. The supervisor handed onMessage's promise to an EventEmitter listener, which discards it. A rejection there is an unhandledRejection, the host has no net for it, and Node ends the process — the supervisor killed by the plugin it contains. dispatch() read `'_inv' in req.params` before handle()'s try/catch, so params that are not an object threw a TypeError into exactly that discarded promise.
This commit is contained in:
@@ -82,7 +82,11 @@ export class PluginRpcHost {
|
||||
}
|
||||
|
||||
async dispatch(req: RpcRequest, actingUserId?: number): Promise<RpcResponse | RpcError> {
|
||||
const raw = (req.params ?? {}) as Record<string, unknown>;
|
||||
// Anything but an object is treated as no params at all. The envelope comes
|
||||
// off an IPC channel a plugin can write to, and `'_inv' in raw` below throws a
|
||||
// TypeError on a primitive — outside handle()'s try/catch, so it escapes as a
|
||||
// rejection rather than an error envelope.
|
||||
const raw = (typeof req.params === 'object' && req.params !== null ? req.params : {}) as Record<string, unknown>;
|
||||
// The supervisor resolves the acting user from `_inv` BEFORE dispatch and passes
|
||||
// the request on untouched, so `_inv` is visible to every handler today and is in
|
||||
// effect a reserved param name. Strip it here: no handler and no audit reads
|
||||
|
||||
@@ -83,6 +83,15 @@ function readTarGz(buf: Buffer, lim: Required<ExtractLimits>): Member[] {
|
||||
const prefix = block.subarray(345, 500).toString('utf8').replace(/\0.*$/, '');
|
||||
if (prefix) name = `${prefix}/${name}`;
|
||||
const size = parseInt(block.subarray(124, 136).toString('utf8').replace(/\0.*$/, '').trim() || '0', 8);
|
||||
// A negative size walks the cursor backwards: `off += 512` and then
|
||||
// `off += Math.ceil(size / 512) * 512` cancel out at -512 and go negative
|
||||
// below it, so the same header is read forever while every pass appends a
|
||||
// member — a 70-byte archive is enough to hold the main thread and exhaust
|
||||
// memory. The entry budgets in extractArchive never get a say; they run on
|
||||
// the list this function has to finish building first.
|
||||
if (!Number.isSafeInteger(size) || size < 0) {
|
||||
throw new ExtractError(`invalid entry size in tar header: ${block.subarray(124, 136).toString('utf8').replace(/\0.*$/, '').trim()}`);
|
||||
}
|
||||
const typeflag = String.fromCharCode(block[156]);
|
||||
off += 512;
|
||||
const data = tar.subarray(off, off + size);
|
||||
|
||||
@@ -474,7 +474,7 @@ export class PluginSupervisor {
|
||||
sup.child = child;
|
||||
sup.lastBeat = Date.now();
|
||||
|
||||
child.on('message', (raw: unknown) => this.onMessage(sup, raw as Envelope));
|
||||
child.on('message', (raw: unknown) => this.handleChildMessage(sup, raw));
|
||||
child.on('exit', (code, signal) => this.onExit(sup, code, signal));
|
||||
child.on('error', (e) => this.hooks.onLog?.(sup.id, 'error', `child error: ${e.message}`));
|
||||
child.stdout?.on('data', (b) => this.recordLog(sup, 'info', String(b).trimEnd()));
|
||||
@@ -502,6 +502,19 @@ export class PluginSupervisor {
|
||||
this.hooks.onLog?.(sup.id, level, msg, meta);
|
||||
}
|
||||
|
||||
/**
|
||||
* An EventEmitter listener throws away what its callback returns, and onMessage is
|
||||
* async — so a rejection in there surfaces as an unhandledRejection, which Node 22
|
||||
* turns into process exit. The host installs no net for it (the plugin child does,
|
||||
* for itself), so one malformed envelope would take down the very supervisor whose
|
||||
* job is to contain a misbehaving plugin. Log it against the plugin and carry on.
|
||||
*/
|
||||
private handleChildMessage(sup: Supervised, raw: unknown): void {
|
||||
this.onMessage(sup, raw as Envelope).catch((e: unknown) => {
|
||||
this.hooks.onLog?.(sup.id, 'error', `message handling failed: ${e instanceof Error ? e.message : String(e)}`);
|
||||
});
|
||||
}
|
||||
|
||||
private async onMessage(sup: Supervised, msg: Envelope): Promise<void> {
|
||||
if (!msg || typeof msg !== 'object') return;
|
||||
|
||||
|
||||
@@ -110,4 +110,29 @@ describe('dispatch strips the supervisor _inv marker', () => {
|
||||
await host.dispatch(req('packing.setBagMembers', { tripId: 1, bagId: 80, userIds: [3], _inv: 'req-7' }), 42);
|
||||
expect(setBagMembers).toHaveBeenCalledWith('1', '80', [3]);
|
||||
});
|
||||
|
||||
/**
|
||||
* The `_inv` check runs before handle()'s try/catch, so a params value that is not
|
||||
* an object used to throw a TypeError out of dispatch instead of coming back as an
|
||||
* error envelope. The supervisor discards the promise dispatch lives on, which made
|
||||
* that an unhandledRejection — process exit, from one malformed envelope.
|
||||
*/
|
||||
it('RPCINV-007 params that are not an object come back as an envelope, not a rejection', async () => {
|
||||
const host = new PluginRpcHost('p', new Set(['db:own']), makeDeps(), dbRegistry());
|
||||
|
||||
for (const params of [5, 'text', true] as unknown as Record<string, unknown>[]) {
|
||||
const res = await host.dispatch({ k: 'req', id: 'x', method: 'db.query', params });
|
||||
expect(res).toMatchObject({ k: 'res', id: 'x', ok: false });
|
||||
}
|
||||
});
|
||||
|
||||
it('RPCINV-008 null and undefined params still behave as an empty object', async () => {
|
||||
const host = new PluginRpcHost('p', new Set(['db:own']), makeDeps(), dbRegistry());
|
||||
|
||||
const nulled = await host.dispatch({ k: 'req', id: 'x', method: 'db.query', params: null as unknown as Record<string, unknown> });
|
||||
// The type says params is required; the IPC channel does not, so a child can
|
||||
// leave it off entirely.
|
||||
const missing = await host.dispatch({ k: 'req', id: 'x', method: 'db.query', params: undefined as unknown as Record<string, unknown> });
|
||||
expect(nulled).toEqual(missing);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -11,13 +11,15 @@ import zlib from 'node:zlib';
|
||||
import { safeJoin, extractArchive, ExtractError } from '../../../src/nest/plugins/install/safe-extract';
|
||||
|
||||
// ── tiny archive builders ────────────────────────────────────────────────────
|
||||
function tarHeader(name: string, size: number, typeflag = '0', linkname = ''): Buffer {
|
||||
function tarHeader(name: string, size: number, typeflag = '0', linkname = '', rawSize?: string): Buffer {
|
||||
const h = Buffer.alloc(512, 0);
|
||||
h.write(name, 0);
|
||||
h.write('0000644', 100);
|
||||
h.write('0000000', 108);
|
||||
h.write('0000000', 116);
|
||||
h.write(size.toString(8).padStart(11, '0'), 124);
|
||||
// rawSize writes the 12-byte field verbatim, for headers a real tar would never
|
||||
// produce (negative, non-octal) — the reader has to survive those too.
|
||||
h.write(rawSize !== undefined ? rawSize.padEnd(11, ' ') : size.toString(8).padStart(11, '0'), 124);
|
||||
h.write('00000000000', 136);
|
||||
h.write(' ', 148); // checksum placeholder = spaces
|
||||
h.write(typeflag, 156);
|
||||
@@ -29,12 +31,12 @@ function tarHeader(name: string, size: number, typeflag = '0', linkname = ''): B
|
||||
h.write(sum.toString(8).padStart(6, '0') + '\0 ', 148);
|
||||
return h;
|
||||
}
|
||||
function makeTarGz(entries: Array<{ name: string; data?: string; type?: string; link?: string }>): Buffer {
|
||||
function makeTarGz(entries: Array<{ name: string; data?: string; type?: string; link?: string; rawSize?: string }>): Buffer {
|
||||
const parts: Buffer[] = [];
|
||||
for (const e of entries) {
|
||||
const body = Buffer.from(e.data ?? '');
|
||||
parts.push(tarHeader(e.name, e.type === '5' || e.type === '2' ? 0 : body.length, e.type ?? '0', e.link ?? ''));
|
||||
if (e.type !== '5' && e.type !== '2') {
|
||||
parts.push(tarHeader(e.name, e.type === '5' || e.type === '2' ? 0 : body.length, e.type ?? '0', e.link ?? '', e.rawSize));
|
||||
if (e.rawSize === undefined && e.type !== '5' && e.type !== '2') {
|
||||
parts.push(body);
|
||||
const pad = (512 - (body.length % 512)) % 512;
|
||||
if (pad) parts.push(Buffer.alloc(pad, 0));
|
||||
@@ -130,4 +132,20 @@ describe('extractArchive', () => {
|
||||
it('rejects an unsupported format', () => {
|
||||
expect(() => extractArchive(Buffer.from('not an archive'), dest)).toThrow(/unsupported/);
|
||||
});
|
||||
|
||||
// A size field is 12 ASCII bytes of octal, and parseInt honours a leading minus.
|
||||
// The reader advanced by `512 + Math.ceil(size / 512) * 512`, which is zero at
|
||||
// -512 and negative below it: the cursor never leaves the first header, and every
|
||||
// pass appends another member until the heap is gone. Synchronous, on the main
|
||||
// thread, from a 70-byte upload.
|
||||
it('refuses a negative entry size instead of looping on it', () => {
|
||||
const gz = makeTarGz([{ name: 'evil.txt', rawSize: '-1000' }]);
|
||||
expect(() => extractArchive(gz, dest)).toThrow(ExtractError);
|
||||
expect(() => extractArchive(gz, dest)).toThrow(/invalid entry size/);
|
||||
});
|
||||
|
||||
it('refuses a size field that is not a number at all', () => {
|
||||
const gz = makeTarGz([{ name: 'evil.txt', rawSize: 'not-octal' }]);
|
||||
expect(() => extractArchive(gz, dest)).toThrow(/invalid entry size/);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -311,3 +311,39 @@ describe('supervisor throttles plugin log/stderr volume (host-thread DoS guard)'
|
||||
expect(sup.droppedLogs).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* The 'message' listener is where a rejection would escape: an EventEmitter discards
|
||||
* what its callback returns, onMessage is async, and the host has no
|
||||
* unhandledRejection handler — so Node 22 would end the process. A plugin getting to
|
||||
* kill the supervisor that contains it defeats the point of the supervisor.
|
||||
*/
|
||||
describe('supervisor message handling never escapes as a rejection', () => {
|
||||
it('logs a failed message against the plugin and keeps running', async () => {
|
||||
const onLog = vi.fn();
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
const s = new PluginSupervisor((() => ({ dispose: vi.fn() })) as any, { onLog }, {});
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
(s as any).onMessage = () => Promise.reject(new Error('malformed envelope'));
|
||||
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
expect(() => (s as any).handleChildMessage({ id: 'p' }, { k: 'req', id: 'x' })).not.toThrow();
|
||||
|
||||
await new Promise((r) => setTimeout(r, 0));
|
||||
expect(onLog).toHaveBeenCalledWith('p', 'error', expect.stringContaining('malformed envelope'));
|
||||
});
|
||||
|
||||
it('says nothing when the message is handled normally', async () => {
|
||||
const onLog = vi.fn();
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
const s = new PluginSupervisor((() => ({ dispose: vi.fn() })) as any, { onLog }, {});
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
(s as any).onMessage = () => Promise.resolve();
|
||||
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
(s as any).handleChildMessage({ id: 'p' }, { k: 'evt', topic: 'log' });
|
||||
|
||||
await new Promise((r) => setTimeout(r, 0));
|
||||
expect(onLog).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user