Skip to content

Commit

Permalink
lib: make primordials Promise methods safe
Browse files Browse the repository at this point in the history
`catch` and `finally` methods on %Promise.prototype% looks up the `then`
property of the instance, making it at risk of prototype pollution.

PR-URL: #38650
Backport-PR-URL: #38878
Refs: https://tc39.es/ecma262/#sec-promise.prototype.catch
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
aduh95 authored and targos committed Jun 14, 2021
1 parent 864fe99 commit ded8335
Show file tree
Hide file tree
Showing 7 changed files with 87 additions and 13 deletions.
14 changes: 7 additions & 7 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ const {
MathMin,
NumberIsSafeInteger,
Promise,
PromisePrototypeFinally,
PromisePrototypeThen,
PromiseResolve,
SafeArrayIterator,
SafePromisePrototypeFinally,
Symbol,
Uint8Array,
} = primordials;
Expand Down Expand Up @@ -186,12 +186,12 @@ class FileHandle extends EventEmitterMixin(JSTransferable) {
this[kRefs]--;
if (this[kRefs] === 0) {
this[kFd] = -1;
this[kClosePromise] = PromisePrototypeFinally(
this[kClosePromise] = SafePromisePrototypeFinally(
this[kHandle].close(),
() => { this[kClosePromise] = undefined; }
);
} else {
this[kClosePromise] = PromisePrototypeFinally(
this[kClosePromise] = SafePromisePrototypeFinally(
new Promise((resolve, reject) => {
this[kCloseResolve] = resolve;
this[kCloseReject] = reject;
Expand Down Expand Up @@ -507,7 +507,7 @@ async function rename(oldPath, newPath) {

async function truncate(path, len = 0) {
const fd = await open(path, 'r+');
return PromisePrototypeFinally(ftruncate(fd, len), fd.close);
return SafePromisePrototypeFinally(ftruncate(fd, len), fd.close);
}

async function ftruncate(handle, len = 0) {
Expand Down Expand Up @@ -638,7 +638,7 @@ async function lchmod(path, mode) {
throw new ERR_METHOD_NOT_IMPLEMENTED('lchmod()');

const fd = await open(path, O_WRONLY | O_SYMLINK);
return PromisePrototypeFinally(fchmod(fd, mode), fd.close);
return SafePromisePrototypeFinally(fchmod(fd, mode), fd.close);
}

async function lchown(path, uid, gid) {
Expand Down Expand Up @@ -717,7 +717,7 @@ async function writeFile(path, data, options) {
checkAborted(options.signal);

const fd = await open(path, flag, options.mode);
return PromisePrototypeFinally(
return SafePromisePrototypeFinally(
writeFileHandle(fd, data, options.signal, options.encoding), fd.close);
}

Expand All @@ -742,7 +742,7 @@ async function readFile(path, options) {
checkAborted(options.signal);

const fd = await open(path, flag, 0o666);
return PromisePrototypeFinally(readFileHandle(fd, options), fd.close);
return SafePromisePrototypeFinally(readFileHandle(fd, options), fd.close);
}

module.exports = {
Expand Down
9 changes: 6 additions & 3 deletions lib/internal/modules/run_main.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
'use strict';

const {
PromisePrototypeFinally,
StringPrototypeEndsWith,
} = primordials;
const CJSLoader = require('internal/modules/cjs/loader');
Expand Down Expand Up @@ -51,7 +50,7 @@ function runMainESM(mainPath) {
}));
}

function handleMainPromise(promise) {
async function handleMainPromise(promise) {
// Handle a Promise from running code that potentially does Top-Level Await.
// In that case, it makes sense to set the exit code to a specific non-zero
// value if the main code never finishes running.
Expand All @@ -60,7 +59,11 @@ function handleMainPromise(promise) {
process.exitCode = 13;
}
process.on('exit', handler);
return PromisePrototypeFinally(promise, () => process.off('exit', handler));
try {
return await promise;
} finally {
process.off('exit', handler);
}
}

// For backwards compatibility, we have to run a bunch of
Expand Down
31 changes: 31 additions & 0 deletions lib/internal/per_context/primordials.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,8 @@ const {
Map,
ObjectFreeze,
ObjectSetPrototypeOf,
Promise,
PromisePrototypeThen,
Set,
SymbolIterator,
WeakMap,
Expand Down Expand Up @@ -384,5 +386,34 @@ primordials.SafeWeakRef = makeSafe(
}
);

const SafePromise = makeSafe(
Promise,
class SafePromise extends Promise {
// eslint-disable-next-line no-useless-constructor
constructor(executor) { super(executor); }
}
);

primordials.PromisePrototypeCatch = (thisPromise, onRejected) =>
PromisePrototypeThen(thisPromise, undefined, onRejected);

/**
* Attaches a callback that is invoked when the Promise is settled (fulfilled or
* rejected). The resolved value cannot be modified from the callback.
* Prefer using async functions when possible.
* @param {Promise<any>} thisPromise
* @param {() => void) | undefined | null} onFinally The callback to execute
* when the Promise is settled (fulfilled or rejected).
* @returns A Promise for the completion of the callback.
*/
primordials.SafePromisePrototypeFinally = (thisPromise, onFinally) =>
// Wrapping on a new Promise is necessary to not expose the SafePromise
// prototype to user-land.
new Promise((a, b) =>
new SafePromise((a, b) => PromisePrototypeThen(thisPromise, a, b))
.finally(onFinally)
.then(a, b)
);

ObjectSetPrototypeOf(primordials, null);
ObjectFreeze(primordials);
6 changes: 3 additions & 3 deletions lib/timers/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
const {
FunctionPrototypeBind,
Promise,
PromisePrototypeFinally,
PromiseReject,
SafePromisePrototypeFinally,
} = primordials;

const {
Expand Down Expand Up @@ -71,7 +71,7 @@ function setTimeout(after, value, options = {}) {
}
});
return oncancel !== undefined ?
PromisePrototypeFinally(
SafePromisePrototypeFinally(
ret,
() => signal.removeEventListener('abort', oncancel)) : ret;
}
Expand Down Expand Up @@ -115,7 +115,7 @@ function setImmediate(value, options = {}) {
}
});
return oncancel !== undefined ?
PromisePrototypeFinally(
SafePromisePrototypeFinally(
ret,
() => signal.removeEventListener('abort', oncancel)) : ret;
}
Expand Down
1 change: 1 addition & 0 deletions test/message/esm_display_syntax_error_import.out
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ SyntaxError: The requested module '../fixtures/es-module-loaders/module-named-ex
at async ModuleJob.run (node:internal/modules/esm/module_job:*:*)
at async Loader.import (node:internal/modules/esm/loader:*:*)
at async Object.loadESM (node:internal/process/esm_loader:*:*)
at async handleMainPromise (node:internal/modules/run_main:*:*)
1 change: 1 addition & 0 deletions test/message/esm_display_syntax_error_import_module.out
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ SyntaxError: The requested module './module-named-exports.mjs' does not provide
at async ModuleJob.run (node:internal/modules/esm/module_job:*:*)
at async Loader.import (node:internal/modules/esm/loader:*:*)
at async Object.loadESM (node:internal/process/esm_loader:*:*)
at async handleMainPromise (node:internal/modules/run_main:*:*)
38 changes: 38 additions & 0 deletions test/parallel/test-primordials-promise.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Flags: --expose-internals
'use strict';

const common = require('../common');
const assert = require('assert');

const {
PromisePrototypeCatch,
PromisePrototypeThen,
SafePromisePrototypeFinally,
} = require('internal/test/binding').primordials;

Promise.prototype.catch = common.mustNotCall();
Promise.prototype.finally = common.mustNotCall();
Promise.prototype.then = common.mustNotCall();

assertIsPromise(PromisePrototypeCatch(Promise.reject(), common.mustCall()));
assertIsPromise(PromisePrototypeThen(test(), common.mustCall()));
assertIsPromise(SafePromisePrototypeFinally(test(), common.mustCall()));

async function test() {
const catchFn = common.mustCall();
const finallyFn = common.mustCall();

try {
await Promise.reject();
} catch {
catchFn();
} finally {
finallyFn();
}
}

function assertIsPromise(promise) {
// Make sure the returned promise is a genuine %Promise% object and not a
// subclass instance.
assert.strictEqual(Object.getPrototypeOf(promise), Promise.prototype);
}

0 comments on commit ded8335

Please sign in to comment.