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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3742f74
lib: make `Error` instantiation less vulnerable to prototype pollution
aduh95 Jan 2, 2023
2e892fc
fixup! lib: make `Error` instantiation less vulnerable to prototype p…
aduh95 Jan 2, 2023
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const {
MathMax,
ObjectCreate,
ObjectDefineProperty,
ObjectDefineProperties,
ObjectGetPrototypeOf,
ObjectKeys,
String,
Expand All @@ -26,6 +27,7 @@ const {
validateObject,
} = require('internal/validators');
const { isErrorStackTraceLimitWritable } = require('internal/errors');
const { assignOwnProperties } = require('internal/util/safe-property-assignment');


const kReadableOperator = {
Expand Down Expand Up @@ -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.
Expand Down
63 changes: 31 additions & 32 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const {
MathMax,
Number,
NumberIsInteger,
ObjectAssign,
ObjectCreate,
ObjectDefineProperty,
ObjectDefineProperties,
ObjectIsExtensible,
Expand All @@ -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 •
edited
Loading

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.
Expand Down Expand Up @@ -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;
Expand All @@ -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;
});

Expand Down Expand Up @@ -249,8 +254,6 @@ class SystemError extends Error {

captureLargerStackTrace(this);

this.code = key;

ObjectDefineProperties(this, {
[kIsNodeError]: {
__proto__: null,
Expand All @@ -273,6 +276,13 @@ class SystemError extends Error {
writable: true,
configurable: true,
},
code: {
__proto__: null,
value: key,
enumerable: true,
writable: true,
configurable: true,
},
info: {
__proto__: null,
value: context,
Expand Down Expand Up @@ -397,7 +407,7 @@ function makeNodeErrorWithCode(Base, key) {
},
});
captureLargerStackTrace(error);
error.code = key;
setOwnProperty(error, 'code', key);
return error;
};
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
});
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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;
}

Expand Down Expand Up @@ -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' });
}
}

Expand All @@ -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;
});

Expand Down
19 changes: 12 additions & 7 deletions lib/internal/event_target.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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;
}
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/fs/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}

Expand Down
7 changes: 5 additions & 2 deletions lib/internal/http2/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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,
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/main/mksnapshot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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)) {
Expand Down
Loading