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

aduh95 · 2023-01-02T19:31:13Z

Having a more robust Error instantiation makes sure the user would get the actual error rather than an unrelated one if the prototype was altered somewhere else.

Before this change:

$ node -e 'Object.defineProperty(Object.prototype, "code", {set(){throw new Error}});fs.readFile()'              
[eval]:1
Object.defineProperty(Object.prototype, "code", {set(){throw new Error}});fs.readFile()
                                                       ^

Error
    at TypeError.set ([eval]:1:62)
    at new NodeError (node:internal/errors:401:16)
    at __node_internal_ (node:internal/validators:421:11)
    at maybeCallback (node:fs:169:3)
    at Object.readFile (node:fs:372:14)
    at [eval]:1:78
    at Script.runInThisContext (node:vm:129:12)
    at Object.runInThisContext (node:vm:307:38)
    at node:internal/process/execution:83:21
    at [eval]-wrapper:6:24

Node.js v19.3.0

After this change:

$ node -e 'Object.defineProperty(Object.prototype, "code", {set(){throw new Error}});fs.readFile()'
node:internal/validators:421
    throw new ERR_INVALID_ARG_TYPE(name, 'Function', value);
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "cb" argument must be of type function. Received undefined
    at maybeCallback (node:fs:169:3)
    at Object.readFile (node:fs:372:14)
    at [eval]:1:78
    at Script.runInThisContext (node:vm:128:12)
    at Object.runInThisContext (node:vm:306:38)
    at node:internal/process/execution:83:21
    at [eval]-wrapper:6:24
    at runScript (node:internal/process/execution:82:62)
    at evalScript (node:internal/process/execution:104:10)
    at node:internal/main/eval_string:50:3 {
  code: 'ERR_INVALID_ARG_TYPE'
}

Node.js v20.0.0-pre

nodejs-github-bot · 2023-01-02T19:31:17Z

ljharb · 2023-01-02T19:41:36Z

lib/internal/errors.js

 const kIsNodeError = Symbol('kIsNodeError');

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

 const messages = new SafeMap();
-const codes = {};
+const codes = ObjectCreate(null);


Suggested change

const codes = ObjectCreate(null);

const codes = { __proto__: null };

this is equally robust and avoids a function call.

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

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?

ljharb · 2023-01-02T19:41:36Z

lib/internal/util/safe-property-assignment.js

+ * @returns {T}
+ */
+function assignOwnProperties(obj, ...sources) {
+  const descriptors = ObjectCreate(null);


Suggested change

const descriptors = ObjectCreate(null);

const descriptors = { __proto__: null };

lib/internal/util/safe-property-assignment.js

…ollution

lib: make Error instantiation less vulnerable to prototype pollution

3742f74

aduh95 added errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-benchmark-ci PR that need a benchmark CI run. labels Jan 2, 2023

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 2, 2023

ljharb reviewed Jan 2, 2023

View changes

fixup! lib: make Error instantiation less vulnerable to prototype p…

2e892fc

…ollution

Dec	JAN	Feb
	03
2022	2023	2024

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

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

aduh95 commented Jan 2, 2023

nodejs-github-bot commented Jan 2, 2023

ljharb Jan 2, 2023

JakobJingleheimer Jan 3, 2023

ljharb Jan 3, 2023 •

edited

ljharb Jan 2, 2023

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

	const descriptors = ObjectCreate(null);
	const descriptors = { __proto__: null };

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

Are you sure you want to change the base?

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

Conversation

aduh95 commented Jan 2, 2023

nodejs-github-bot commented Jan 2, 2023

ljharb Jan 2, 2023

Choose a reason for hiding this comment

JakobJingleheimer Jan 3, 2023

Choose a reason for hiding this comment

ljharb Jan 3, 2023 • edited

Choose a reason for hiding this comment

ljharb Jan 2, 2023

Choose a reason for hiding this comment

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

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

ljharb Jan 3, 2023 •

edited