Skip to content

Commit 2f69f67

Browse files
Fcmam5metcoder95
authored andcommitted
Merge commit from fork
* security: harden Piscina options against prototype pollution Harden Piscina against prototype pollution by ensuring all user-controlled option reads come from own properties and by storing pool options on a null-prototype object. This prevents attackers who can pollute Object.prototype from influencing worker configuration (filename, name, transferList, signal, force) or pool defaults. NB: pool.options is now a null-prototype object. This is a potential breaking change for consumers that call Object.prototype methods directly on pool.options (e.g. pool.options.hasOwnProperty(...)). * fix: use sanitized options for resourceLimits validation * fix: sanitize run/close options with withNullPrototype Use the existing withNullPrototype helper instead of getOwn; address feedback from @metcoder95 * fix: avoid re-linking Object.prototype in Piscina constructor (cherry picked from commit bebbda2)
1 parent 6a23286 commit 2f69f67

3 files changed

Lines changed: 80 additions & 10 deletions

File tree

‎src/common.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,12 @@ export function getAvailableParallelism () : number {
6060
return availableParallelism();
6161
}
6262

63+
// Copy own properties onto a prototype-less object, so that options are never
64+
// resolved through a polluted prototype chain.
65+
export function withNullPrototype<T extends object>(source: T, overrides?: Partial<T>): T {
66+
return Object.assign(Object.create(null), source, overrides)
67+
}
68+
6369
export function promiseResolvers <T = any> () :
6470
{ promise: Promise<T>,
6571
resolve: (res: T) => void,

‎src/index.ts‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ import {
5353
markMovable,
5454
getAvailableParallelism,
5555
maybeFileURLToPath,
56+
withNullPrototype,
5657
promiseResolvers
5758
} from './common';
5859
const cpuParallelism : number = getAvailableParallelism();
@@ -191,7 +192,12 @@ class ThreadPool {
191192

192193
const filename =
193194
options.filename ? maybeFileURLToPath(options.filename) : null;
194-
this.options = { ...kDefaultOptions, ...options, filename, maxQueue: 0 };
195+
this.options = withNullPrototype({
196+
...kDefaultOptions,
197+
...options,
198+
filename,
199+
maxQueue: 0,
200+
});
195201

196202
if (this.options.recordTiming) {
197203
this.histogram = new PiscinaHistogramHandler();
@@ -749,8 +755,8 @@ export default class Piscina<T = any, R = any> extends EventEmitterAsyncResource
749755
#histogram: PiscinaHistogram | null = null;
750756

751757
constructor (options : Options = {}) {
752-
const opts = { ...options, '__proto__': null };
753-
super({ ...opts, name: 'Piscina' });
758+
const opts = withNullPrototype(options);
759+
super(withNullPrototype(opts, { name: 'Piscina' }));
754760

755761
if (typeof opts.filename !== 'string' && opts.filename != null) {
756762
throw new TypeError('options.filename must be a string or null');
@@ -827,12 +833,12 @@ export default class Piscina<T = any, R = any> extends EventEmitterAsyncResource
827833
new TypeError('options must be an object'));
828834
}
829835

830-
const {
831-
transferList,
832-
signal
833-
} = options;
834-
const filename = Object.prototype.hasOwnProperty.call(options, 'filename') ? options.filename : null;
835-
const name = Object.prototype.hasOwnProperty.call(options, 'name') ? options.name : null;
836+
options = withNullPrototype(options);
837+
838+
const transferList = options.transferList;
839+
const signal = options.signal ?? null;
840+
const filename = options.filename ?? null;
841+
const name = options.name ?? null;
836842

837843
if (transferList !== undefined && !Array.isArray(transferList)) {
838844
return Promise.reject(
@@ -858,6 +864,8 @@ export default class Piscina<T = any, R = any> extends EventEmitterAsyncResource
858864
throw TypeError('options must be an object');
859865
}
860866

867+
options = withNullPrototype(options);
868+
861869
let { force } = options;
862870

863871
if (force !== undefined && typeof force !== 'boolean') {

‎test/option-validation.test.ts‎

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,4 +152,60 @@ test('trackUnmanagedFds must be a boolean', () => {
152152
assert.throws(() => new Piscina(({
153153
trackUnmanagedFds: 'string'
154154
}) as any), /options.trackUnmanagedFds must be a boolean/);
155-
});
155+
});
156+
157+
test('execArgv is not tampered', async () => {
158+
(Object.prototype as any).execArgv = ['--not-a-real-flag']
159+
160+
const pool = new Piscina({
161+
filename: resolve(__dirname, 'fixtures/eval.js'),
162+
minThreads: 1,
163+
maxThreads: 1
164+
})
165+
166+
try {
167+
assert.strictEqual(pool.options.execArgv, undefined)
168+
assert.strictEqual(await pool.run('42'), 42)
169+
} finally {
170+
delete (Object.prototype as any).execArgv
171+
await pool.close()
172+
}
173+
})
174+
175+
test('loadBalancer is not tampered', async () => {
176+
let called = false
177+
;(Object.prototype as any).loadBalancer = () => { called = true; return null }
178+
179+
const pool = new Piscina({
180+
filename: resolve(__dirname, 'fixtures/eval.js'),
181+
minThreads: 1,
182+
maxThreads: 1
183+
})
184+
185+
try {
186+
assert.strictEqual(pool.options.loadBalancer, undefined)
187+
assert.strictEqual(await pool.run('42'), 42)
188+
assert.strictEqual(called, false)
189+
} finally {
190+
delete (Object.prototype as any).loadBalancer
191+
await pool.close()
192+
}
193+
})
194+
195+
test('env is not tampered', async () => {
196+
(Object.prototype as any).env = { NODE_OPTIONS: '--title=polluted' }
197+
198+
const pool = new Piscina({
199+
filename: resolve(__dirname, 'fixtures/eval.js'),
200+
minThreads: 1,
201+
maxThreads: 1
202+
})
203+
204+
try {
205+
assert.strictEqual(pool.options.env, undefined)
206+
assert.strictEqual(await pool.run('42'), 42)
207+
} finally {
208+
delete (Object.prototype as any).env
209+
await pool.close()
210+
}
211+
})

0 commit comments

Comments
 (0)