Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: make Error objects instantiation less vulnerable to prototype pollution #46065

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 33 additions & 19 deletions lib/internal/assert/assertion_error.js
Original file line number Diff line number Diff line change
@@ -8,6 +8,7 @@ const {
MathMax,
ObjectCreate,
ObjectDefineProperty,
ObjectDefineProperties,
ObjectGetPrototypeOf,
ObjectKeys,
String,
@@ -26,6 +27,7 @@ const {
validateObject,
} = require('internal/validators');
const { isErrorStackTraceLimitWritable } = require('internal/errors');
const { assignOwnProperties } = require('internal/util/safe-property-assignment');


const kReadableOperator = {
@@ -424,30 +426,42 @@ class AssertionError extends Error {

if (isErrorStackTraceLimitWritable()) Error.stackTraceLimit = limit;

this.generatedMessage = !message;
ObjectDefineProperty(this, 'name', {
__proto__: null,
value: 'AssertionError [ERR_ASSERTION]',
enumerable: false,
writable: true,
configurable: true
ObjectDefineProperties(this, {
generatedMessage: {
__proto__: null,
value: !message,
enumerable: true,
writable: true,
configurable: true,
},
name: {
__proto__: null,
value: 'AssertionError [ERR_ASSERTION]',
enumerable: false,
writable: true,
configurable: true
},
code: {
__proto__: null,
value: 'ERR_ASSERTION',
enumerable: true,
writable: true,
configurable: true,
},
});
this.code = 'ERR_ASSERTION';
if (details) {
this.actual = undefined;
this.expected = undefined;
this.operator = undefined;
assignOwnProperties(this, { actual: undefined, expected: undefined, operator: undefined });
for (let i = 0; i < details.length; i++) {
this['message ' + i] = details[i].message;
this['actual ' + i] = details[i].actual;
this['expected ' + i] = details[i].expected;
this['operator ' + i] = details[i].operator;
this['stack trace ' + i] = details[i].stack;
assignOwnProperties(this, {
['message ' + i]: details[i].message,
['actual ' + i]: details[i].actual,
['expected ' + i]: details[i].expected,
['operator ' + i]: details[i].operator,
['stack trace ' + i]: details[i].stack,
});
}
} else {
this.actual = actual;
this.expected = expected;
this.operator = operator;
assignOwnProperties(this, { actual, expected, operator });
}
ErrorCaptureStackTrace(this, stackStartFn || stackStartFunction);
// Create error message including the error code in the name.
63 changes: 31 additions & 32 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
@@ -32,7 +32,7 @@ const {
MathMax,
Number,
NumberIsInteger,
ObjectAssign,
ObjectCreate,
ObjectDefineProperty,
ObjectDefineProperties,
ObjectIsExtensible,
@@ -59,12 +59,17 @@ const {
URIError,
} = primordials;

const {
assignOwnProperties,
setOwnProperty,
} = require('internal/util/safe-property-assignment');

const kIsNodeError = Symbol('kIsNodeError');

const isWindows = process.platform === 'win32';

const messages = new SafeMap();
const codes = {};
const codes = ObjectCreate(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const codes = ObjectCreate(null);
const codes = { __proto__: null };

this is equally robust and avoids a function call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do prefer this, but I know Antoine prefers the other, and the diff is pretty "6 of one".

Copy link
Member

@ljharb ljharb Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the preference?

Even setting aside that Object.create is a regretted addition to the JS language, it's still got the potential overhead of a function call, as compared to the very straightforward syntax that node already uses with objects initialized with more than one property. Why is "no properties" a special case that requires a function call?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's still got the potential overhead of a function call

I don't think that should be a concern here because lib/internal/errors.js is already a part of the snapshot, so this code won't get executed at startup, rather the context would get deserialized from the snapshot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, fair - it’s still indirection, and it’s using an API call for something that’s available with syntax.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a style question that’s out of scope for this PR. If someone feels strongly about this, they should open a new PR that updates our eslint to enforce a particular style.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes thanks, i'm sure that won't get bogged down in blocks, i'll get right on that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opened here: #46083

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(that was landed, so this can be resolved)


const classRegExp = /^([A-Z][a-z0-9]*)+$/;
// Sorted by a rough estimate on most frequently used entries.
@@ -161,7 +166,7 @@ const aggregateTwoErrors = hideStackFrames((innerError, outerError) => {
outerError,
innerError,
]), outerError.message);
err.code = outerError.code;
setOwnProperty(err, 'code', outerError.code);
return err;
}
return innerError || outerError;
@@ -170,7 +175,7 @@ const aggregateTwoErrors = hideStackFrames((innerError, outerError) => {
const aggregateErrors = hideStackFrames((errors, message, code) => {
// eslint-disable-next-line no-restricted-syntax
const err = new AggregateError(new SafeArrayIterator(errors), message);
err.code = errors[0]?.code;
setOwnProperty(err, 'code', errors[0]?.code);
return err;
});

@@ -249,9 +254,14 @@ class SystemError extends Error {

captureLargerStackTrace(this);

this.code = key;

ObjectDefineProperties(this, {
code: {
__proto__: null,
value: key,
enumerable: true,
writable: true,
configurable: true,
},
[kIsNodeError]: {
__proto__: null,
value: true,
@@ -397,7 +407,7 @@ function makeNodeErrorWithCode(Base, key) {
},
});
captureLargerStackTrace(error);
error.code = key;
setOwnProperty(error, 'code', key);
return error;
};
}
@@ -533,15 +543,15 @@ const uvException = hideStackFrames(function uvException(ctx) {
if (prop === 'message' || prop === 'path' || prop === 'dest') {
continue;
}
err[prop] = ctx[prop];
setOwnProperty(err, prop, ctx[prop]);
}

err.code = code;
setOwnProperty(err, 'code', code);
if (path) {
err.path = path;
setOwnProperty(err, 'path', path);
}
if (dest) {
err.dest = dest;
setOwnProperty(err, 'dest', dest);
}

return captureLargerStackTrace(err);
@@ -578,12 +588,9 @@ const uvExceptionWithHostPort = hideStackFrames(
// eslint-disable-next-line no-restricted-syntax
const ex = new Error(`${message}${details}`);
if (isErrorStackTraceLimitWritable()) Error.stackTraceLimit = tmpLimit;
ex.code = code;
ex.errno = err;
ex.syscall = syscall;
ex.address = address;
assignOwnProperties(ex, { code, errno: err, syscall, address });
if (port) {
ex.port = port;
setOwnProperty(ex, 'port', port);
}

return captureLargerStackTrace(ex);
@@ -613,9 +620,7 @@ const errnoException = hideStackFrames(
// eslint-disable-next-line no-restricted-syntax
const ex = new Error(message);
if (isErrorStackTraceLimitWritable()) Error.stackTraceLimit = tmpLimit;
ex.errno = err;
ex.code = code;
ex.syscall = syscall;
assignOwnProperties(ex, { errno: err, code, syscall });

return captureLargerStackTrace(ex);
});
@@ -657,12 +662,9 @@ const exceptionWithHostPort = hideStackFrames(
// eslint-disable-next-line no-restricted-syntax
const ex = new Error(`${syscall} ${code}${details}`);
if (isErrorStackTraceLimitWritable()) Error.stackTraceLimit = tmpLimit;
ex.errno = err;
ex.code = code;
ex.syscall = syscall;
ex.address = address;
assignOwnProperties(ex, { errno: err, code, syscall, address });
if (port) {
ex.port = port;
setOwnProperty(ex, 'port', port);
}

return captureLargerStackTrace(ex);
@@ -702,11 +704,9 @@ const dnsException = hideStackFrames(function(code, syscall, hostname) {
// eslint-disable-next-line no-restricted-syntax
const ex = new Error(message);
if (isErrorStackTraceLimitWritable()) Error.stackTraceLimit = tmpLimit;
ex.errno = errno;
ex.code = code;
ex.syscall = syscall;
assignOwnProperties(ex, { errno, code, syscall });
if (hostname) {
ex.hostname = hostname;
setOwnProperty(ex, 'hostname', hostname);
}

return captureLargerStackTrace(ex);
@@ -715,7 +715,7 @@ const dnsException = hideStackFrames(function(code, syscall, hostname) {
function connResetException(msg) {
// eslint-disable-next-line no-restricted-syntax
const ex = new Error(msg);
ex.code = 'ECONNRESET';
setOwnProperty(ex, 'code', 'ECONNRESET');
return ex;
}

@@ -850,8 +850,7 @@ class AbortError extends Error {
throw new codes.ERR_INVALID_ARG_TYPE('options', 'Object', options);
}
super(message, options);
this.code = 'ABORT_ERR';
this.name = 'AbortError';
assignOwnProperties(this, { code: 'ABORT_ERR', name: 'AbortError' });
}
}

@@ -865,7 +864,7 @@ class AbortError extends Error {
const genericNodeError = hideStackFrames(function genericNodeError(message, errorProperties) {
// eslint-disable-next-line no-restricted-syntax
const err = new Error(message);
ObjectAssign(err, errorProperties);
assignOwnProperties(err, errorProperties);
return err;
});

19 changes: 12 additions & 7 deletions lib/internal/event_target.js
Original file line number Diff line number Diff line change
@@ -67,6 +67,7 @@ const kIsNodeStyleListener = Symbol('kIsNodeStyleListener');
const kTrustEvent = Symbol('kTrustEvent');

const { now } = require('internal/perf/utils');
const { assignOwnProperties } = require('internal/util/safe-property-assignment');

const kType = Symbol('type');
const kDetail = Symbol('detail');
@@ -516,10 +517,12 @@ class EventTarget {
`${size} ${type} listeners ` +
`added to ${inspect(this, { depth: -1 })}. Use ` +
'events.setMaxListeners() to increase limit');
w.name = 'MaxListenersExceededWarning';
w.target = this;
w.type = type;
w.count = size;
assignOwnProperties(w, {
name: 'MaxListenersExceededWarning',
target: this,
type,
count: size,
});
process.emitWarning(w);
}
}
@@ -567,9 +570,11 @@ class EventTarget {
// eslint-disable-next-line no-restricted-syntax
const w = new Error(`addEventListener called with ${listener}` +
' which has no effect.');
w.name = 'AddEventListenerArgumentTypeWarning';
w.target = this;
w.type = type;
assignOwnProperties(w, {
name: 'AddEventListenerArgumentTypeWarning',
target: this,
type: type,
});
process.emitWarning(w);
return;
}
4 changes: 2 additions & 2 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
@@ -108,6 +108,7 @@ const { Interface } = require('internal/readline/interface');
const {
JSTransferable, kDeserialize, kTransfer, kTransferList
} = require('internal/worker/js_transferable');
const { assignOwnProperties } = require('internal/util/safe-property-assignment');

const getDirectoryEntriesPromise = promisify(getDirents);
const validateRmOptionsPromise = promisify(validateRmOptions);
@@ -372,8 +373,7 @@ async function fsCall(fn, handle, ...args) {
if (handle.fd === -1) {
// eslint-disable-next-line no-restricted-syntax
const err = new Error('file closed');
err.code = 'EBADF';
err.syscall = fn.name;
assignOwnProperties(err, { code: 'EBADF', syscall: fn.name });
throw err;
}

7 changes: 5 additions & 2 deletions lib/internal/http2/util.js
Original file line number Diff line number Diff line change
@@ -34,6 +34,7 @@ const {
hideStackFrames,
kIsNodeError,
} = require('internal/errors');
const { assignOwnProperties } = require('internal/util/safe-property-assignment');

const kSensitiveHeaders = Symbol('nodejs.http2.sensitiveHeaders');
const kSocket = Symbol('socket');
@@ -551,8 +552,10 @@ class NghttpError extends Error {
super(customErrorCode ?
getMessage(customErrorCode, [], null) :
binding.nghttp2ErrorString(integerCode));
this.code = customErrorCode || 'ERR_HTTP2_ERROR';
this.errno = integerCode;
assignOwnProperties(this, {
code: customErrorCode || 'ERR_HTTP2_ERROR',
errno: integerCode,
});
captureLargerStackTrace(this);
ObjectDefineProperty(this, kIsNodeError, {
__proto__: null,
3 changes: 2 additions & 1 deletion lib/internal/main/mksnapshot.js
Original file line number Diff line number Diff line change
@@ -22,6 +22,7 @@ const {
const {
readFileSync
} = require('fs');
const { setOwnProperty } = require('internal/util/safe-property-assignment');

const supportedModules = new SafeSet(new SafeArrayIterator([
// '_http_agent',
@@ -100,7 +101,7 @@ function requireForUserSnapshot(id) {
const err = new Error(
`Cannot find module '${id}'. `
);
err.code = 'MODULE_NOT_FOUND';
setOwnProperty(err, 'code', 'MODULE_NOT_FOUND');
throw err;
}
if (!supportedInUserSnapshot(id)) {
21 changes: 12 additions & 9 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
@@ -85,9 +85,12 @@ const {
emitExperimentalWarning,
kEmptyObject,
filterOwnProperties,
setOwnProperty,
getLazy,
} = require('internal/util');
const {
assignOwnProperties,
setOwnProperty,
} = require('internal/util/safe-property-assignment');
const { internalCompileFunction } = require('internal/vm');
const assert = require('internal/assert');
const fs = require('fs');
@@ -420,9 +423,11 @@ function tryPackage(requestPath, exts, isMain, originalPath) {
`Cannot find module '${filename}'. ` +
'Please verify that the package.json has a valid "main" entry'
);
err.code = 'MODULE_NOT_FOUND';
err.path = path.resolve(requestPath, 'package.json');
err.requestPath = originalPath;
assignOwnProperties(err, {
code: 'MODULE_NOT_FOUND',
path: path.resolve(requestPath, 'package.json'),
requestPath: originalPath,
});
// TODO(BridgeAR): Add the requireStack as well.
throw err;
} else {
@@ -1059,8 +1064,7 @@ Module._resolveFilename = function(request, parent, isMain, options) {
}
// eslint-disable-next-line no-restricted-syntax
const err = new Error(message);
err.code = 'MODULE_NOT_FOUND';
err.requireStack = requireStack;
assignOwnProperties(err, { code: 'MODULE_NOT_FOUND', requireStack });
throw err;
};

@@ -1081,9 +1085,8 @@ function finalizeEsmResolution(resolved, parentPath, pkgPath) {
function createEsmNotFoundErr(request, path) {
// eslint-disable-next-line no-restricted-syntax
const err = new Error(`Cannot find module '${request}'`);
err.code = 'MODULE_NOT_FOUND';
if (path)
err.path = path;
setOwnProperty(err, 'code', 'MODULE_NOT_FOUND');
if (path) setOwnProperty(err, 'path', path);
// TODO(BridgeAR): Add the requireStack as well.
return err;
}
Loading