Skip to content

Commit

Permalink
tools: add avoid-prototype-pollution lint rule
Browse files Browse the repository at this point in the history
PR-URL: nodejs/node#43308
Backport-PR-URL: nodejs/node#44081
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
aduh95 authored and guangwong committed Oct 10, 2022
1 parent a727b31 commit e404223
Show file tree
Hide file tree
Showing 9 changed files with 258 additions and 5 deletions.
1 change: 1 addition & 0 deletions lib/.eslintrc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ rules:
- name: SubtleCrypto
message: Use `const { SubtleCrypto } = require('internal/crypto/webcrypto');` instead of the global.
# Custom rules in tools/eslint-rules
node-core/avoid-prototype-pollution: error
node-core/lowercase-name-for-primitive: error
node-core/non-ascii-character: error
node-core/no-array-destructuring: error
Expand Down
3 changes: 3 additions & 0 deletions lib/crypto.js
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ ObjectDefineProperties(module.exports, {
// Aliases for randomBytes are deprecated.
// The ecosystem needs those to exist for backwards compatibility.
prng: {
__proto__: null,
enumerable: false,
configurable: true,
writable: true,
Expand All @@ -305,6 +306,7 @@ ObjectDefineProperties(module.exports, {
randomBytes
},
pseudoRandomBytes: {
__proto__: null,
enumerable: false,
configurable: true,
writable: true,
Expand All @@ -314,6 +316,7 @@ ObjectDefineProperties(module.exports, {
randomBytes
},
rng: {
__proto__: null,
enumerable: false,
configurable: true,
writable: true,
Expand Down
4 changes: 4 additions & 0 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,7 @@ function createGlobalConsole(consoleFromVM) {
// https://heycam.github.io/webidl/#es-namespaces
function exposeNamespace(target, name, namespaceObject) {
ObjectDefineProperty(target, name, {
__proto__: null,
writable: true,
enumerable: false,
configurable: true,
Expand All @@ -506,6 +507,7 @@ function exposeNamespace(target, name, namespaceObject) {
// https://heycam.github.io/webidl/#es-interfaces
function exposeInterface(target, name, interfaceObject) {
ObjectDefineProperty(target, name, {
__proto__: null,
writable: true,
enumerable: false,
configurable: true,
Expand All @@ -516,6 +518,7 @@ function exposeInterface(target, name, interfaceObject) {
// https://heycam.github.io/webidl/#define-the-operations
function defineOperation(target, name, method) {
ObjectDefineProperty(target, name, {
__proto__: null,
writable: true,
enumerable: true,
configurable: true,
Expand All @@ -526,6 +529,7 @@ function defineOperation(target, name, method) {
// https://heycam.github.io/webidl/#Replaceable
function defineReplacableAttribute(target, name, value) {
ObjectDefineProperty(target, name, {
__proto__: null,
writable: true,
enumerable: true,
configurable: true,
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/bootstrap/pre_execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -209,18 +209,21 @@ function setupWebCrypto() {
if (internalBinding('config').hasOpenSSL) {
webcrypto ??= require('internal/crypto/webcrypto');
ObjectDefineProperty(globalThis, 'Crypto', {
__proto__: null,
writable: true,
enumerable: false,
configurable: true,
value: webcrypto.Crypto
});
ObjectDefineProperty(globalThis, 'CryptoKey', {
__proto__: null,
writable: true,
enumerable: false,
configurable: true,
value: webcrypto.CryptoKey
});
ObjectDefineProperty(globalThis, 'SubtleCrypto', {
__proto__: null,
writable: true,
enumerable: false,
configurable: true,
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/per_context/domexception.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,14 @@ function throwInvalidThisError(Base, type) {
const key = 'ERR_INVALID_THIS';
ObjectDefineProperties(err, {
message: {
__proto__: null,
value: `Value of "this" must be of ${type}`,
enumerable: false,
writable: true,
configurable: true,
},
toString: {
__proto__: null,
value() {
return `${this.name} [${key}]: ${this.message}`;
},
Expand Down
10 changes: 5 additions & 5 deletions lib/internal/per_context/primordials.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ function copyPropsRenamed(src, dest, prefix) {
copyAccessor(dest, prefix, newKey, desc);
} else {
const name = `${prefix}${newKey}`;
ReflectDefineProperty(dest, name, desc);
ReflectDefineProperty(dest, name, { __proto__: null, ...desc });
if (varargsMethods.includes(name)) {
ReflectDefineProperty(dest, `${name}Apply`, {
__proto__: null,
Expand All @@ -105,7 +105,7 @@ function copyPropsRenamedBound(src, dest, prefix) {
}

const name = `${prefix}${newKey}`;
ReflectDefineProperty(dest, name, desc);
ReflectDefineProperty(dest, name, { __proto__: null, ...desc });
if (varargsMethods.includes(name)) {
ReflectDefineProperty(dest, `${name}Apply`, {
__proto__: null,
Expand All @@ -129,7 +129,7 @@ function copyPrototype(src, dest, prefix) {
}

const name = `${prefix}${newKey}`;
ReflectDefineProperty(dest, name, desc);
ReflectDefineProperty(dest, name, { __proto__: null, ...desc });
if (varargsMethods.includes(name)) {
ReflectDefineProperty(dest, `${name}Apply`, {
__proto__: null,
Expand Down Expand Up @@ -313,7 +313,7 @@ const copyProps = (src, dest) => {
ReflectDefineProperty(
dest,
key,
ReflectGetOwnPropertyDescriptor(src, key));
{ __proto__: null, ...ReflectGetOwnPropertyDescriptor(src, key) });
}
});
};
Expand Down Expand Up @@ -341,7 +341,7 @@ const makeSafe = (unsafe, safe) => {
return new SafeIterator(this);
};
}
ReflectDefineProperty(safe.prototype, key, desc);
ReflectDefineProperty(safe.prototype, key, { __proto__: null, ...desc });
}
});
} else {
Expand Down
1 change: 1 addition & 0 deletions lib/readline.js
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,7 @@ ObjectSetPrototypeOf(Interface.prototype, EventEmitter.prototype);
ObjectSetPrototypeOf(Interface, EventEmitter);

ObjectDefineProperty(Interface.prototype, 'columns', {
__proto__: null,
configurable: true,
enumerable: true,
get: function() {
Expand Down
147 changes: 147 additions & 0 deletions test/parallel/test-eslint-avoid-prototype-pollution.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
'use strict';

const common = require('../common');
if ((!common.hasCrypto) || (!common.hasIntl)) {
common.skip('ESLint tests require crypto and Intl');
}

common.skipIfEslintMissing();

const RuleTester = require('../../tools/node_modules/eslint').RuleTester;
const rule = require('../../tools/eslint-rules/avoid-prototype-pollution');

new RuleTester({
parserOptions: { ecmaVersion: 2022 },
})
.run('property-descriptor-no-prototype-pollution', rule, {
valid: [
'ObjectDefineProperties({}, {})',
'ObjectCreate(null, {})',
'ObjectDefineProperties({}, { key })',
'ObjectCreate(null, { key })',
'ObjectDefineProperties({}, { ...spread })',
'ObjectCreate(null, { ...spread })',
'ObjectDefineProperties({}, { key: valueDescriptor })',
'ObjectCreate(null, { key: valueDescriptor })',
'ObjectDefineProperties({}, { key: { ...{}, __proto__: null } })',
'ObjectCreate(null, { key: { ...{}, __proto__: null } })',
'ObjectDefineProperties({}, { key: { __proto__: null } })',
'ObjectCreate(null, { key: { __proto__: null } })',
'ObjectDefineProperties({}, { key: { __proto__: null, enumerable: true } })',
'ObjectCreate(null, { key: { __proto__: null, enumerable: true } })',
'ObjectDefineProperties({}, { key: { "__proto__": null } })',
'ObjectCreate(null, { key: { "__proto__": null } })',
'ObjectDefineProperties({}, { key: { \'__proto__\': null } })',
'ObjectCreate(null, { key: { \'__proto__\': null } })',
'ObjectDefineProperty({}, "key", ObjectCreate(null))',
'ReflectDefineProperty({}, "key", ObjectCreate(null))',
'ObjectDefineProperty({}, "key", valueDescriptor)',
'ReflectDefineProperty({}, "key", valueDescriptor)',
'ObjectDefineProperty({}, "key", { __proto__: null })',
'ReflectDefineProperty({}, "key", { __proto__: null })',
'ObjectDefineProperty({}, "key", { __proto__: null, enumerable: true })',
'ReflectDefineProperty({}, "key", { __proto__: null, enumerable: true })',
'ObjectDefineProperty({}, "key", { "__proto__": null })',
'ReflectDefineProperty({}, "key", { "__proto__": null })',
'ObjectDefineProperty({}, "key", { \'__proto__\': null })',
'ReflectDefineProperty({}, "key", { \'__proto__\': null })',
],
invalid: [
{
code: 'ObjectDefineProperties({}, ObjectGetOwnPropertyDescriptors({}))',
errors: [{ message: /prototype pollution/ }],
},
{
code: 'ObjectCreate(null, ObjectGetOwnPropertyDescriptors({}))',
errors: [{ message: /prototype pollution/ }],
},
{
code: 'ObjectDefineProperties({}, { key: {} })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ObjectCreate(null, { key: {} })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ObjectDefineProperties({}, { key: { [void 0]: { ...{ __proto__: null } } } })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ObjectCreate(null, { key: { [void 0]: { ...{ __proto__: null } } } })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ObjectDefineProperties({}, { key: { __proto__: Object.prototype } })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ObjectCreate(null, { key: { __proto__: Object.prototype } })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ObjectDefineProperties({}, { key: { [`__proto__`]: null } })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ObjectCreate(null, { key: { [`__proto__`]: null } })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ObjectDefineProperties({}, { key: { enumerable: true } })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ObjectCreate(null, { key: { enumerable: true } })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ObjectDefineProperty({}, "key", {})',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ReflectDefineProperty({}, "key", {})',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ObjectDefineProperty({}, "key", ObjectGetOwnPropertyDescriptor({}, "key"))',
errors: [{ message: /prototype pollution/ }],
},
{
code: 'ReflectDefineProperty({}, "key", ObjectGetOwnPropertyDescriptor({}, "key"))',
errors: [{ message: /prototype pollution/ }],
},
{
code: 'ObjectDefineProperty({}, "key", ReflectGetOwnPropertyDescriptor({}, "key"))',
errors: [{ message: /prototype pollution/ }],
},
{
code: 'ReflectDefineProperty({}, "key", ReflectGetOwnPropertyDescriptor({}, "key"))',
errors: [{ message: /prototype pollution/ }],
},
{
code: 'ObjectDefineProperty({}, "key", { __proto__: Object.prototype })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ReflectDefineProperty({}, "key", { __proto__: Object.prototype })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ObjectDefineProperty({}, "key", { [`__proto__`]: null })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ReflectDefineProperty({}, "key", { [`__proto__`]: null })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ObjectDefineProperty({}, "key", { enumerable: true })',
errors: [{ message: /null-prototype/ }],
},
{
code: 'ReflectDefineProperty({}, "key", { enumerable: true })',
errors: [{ message: /null-prototype/ }],
},
]
});
92 changes: 92 additions & 0 deletions tools/eslint-rules/avoid-prototype-pollution.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
'use strict';

function checkProperties(context, node) {
if (
node.type === 'CallExpression' &&
node.callee.name === 'ObjectGetOwnPropertyDescriptors'
) {
context.report({
node,
message:
'Property descriptors inherits from the Object prototype, therefore are subject to prototype pollution',
});
}
if (node.type !== 'ObjectExpression') return;
for (const { key, value } of node.properties) {
if (
key != null && value != null &&
!(key.type === 'Identifier' && key.name === '__proto__') &&
!(key.type === 'Literal' && key.value === '__proto__')
) {
checkPropertyDescriptor(context, value);
}
}
}

function checkPropertyDescriptor(context, node) {
if (
node.type === 'CallExpression' &&
(node.callee.name === 'ObjectGetOwnPropertyDescriptor' ||
node.callee.name === 'ReflectGetOwnPropertyDescriptor')
) {
context.report({
node,
message:
'Property descriptors inherits from the Object prototype, therefore are subject to prototype pollution',
suggest: [{
desc: 'Wrap the property descriptor in a null-prototype object',
fix(fixer) {
return [
fixer.insertTextBefore(node, '{ __proto__: null,...'),
fixer.insertTextAfter(node, ' }'),
];
},
}],
});
}
if (node.type !== 'ObjectExpression') return;

for (const { key, value } of node.properties) {
if (
key != null && value != null &&
((key.type === 'Identifier' && key.name === '__proto__') ||
(key.type === 'Literal' && key.value === '__proto__')) &&
value.type === 'Literal' && value.value === null
) {
return true;
}
}

context.report({
node,
message: 'Must use null-prototype object for property descriptors',
});
}

const CallExpression = 'ExpressionStatement[expression.type="CallExpression"]';
module.exports = {
meta: { hasSuggestions: true },
create(context) {
return {
[`${CallExpression}[expression.callee.name=${/^(Object|Reflect)DefinePropert(ies|y)$/}]`](
node
) {
switch (node.expression.callee.name) {
case 'ObjectDefineProperties':
checkProperties(context, node.expression.arguments[1]);
break;
case 'ReflectDefineProperty':
case 'ObjectDefineProperty':
checkPropertyDescriptor(context, node.expression.arguments[2]);
break;
default:
throw new Error('Unreachable');
}
},

[`${CallExpression}[expression.callee.name="ObjectCreate"][expression.arguments.length=2]`](node) {
checkProperties(context, node.expression.arguments[1]);
},
};
},
};

0 comments on commit e404223

Please sign in to comment.