tools,doc: add guards against prototype pollution when creating proxies

aduh95 · danielleadams · commit 812140c65a2f · 2022-06-15T21:28:48.000-04:00
PR-URL: #43391 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
diff --git a/doc/contributing/primordials.md b/doc/contributing/primordials.md
@@ -705,3 +705,27 @@ class SomeClass {
 ObjectDefineProperty(SomeClass.prototype, 'readOnlyProperty', kEnumerableProperty);
 console.log(new SomeClass().readOnlyProperty); // genuine data
 ```
+
+### Defining a `Proxy` handler
+
+When defining a `Proxy`, the handler object could be at risk of prototype
+pollution when using a plain object literal:
+
+```js
+// User-land
+Object.prototype.get = () => 'Unrelated user-provided data';
+
+// Core
+const objectToProxy = { someProperty: 'genuine value' };
+
+const proxyWithPlainObjectLiteral = new Proxy(objectToProxy, {
+  has() { return false; },
+});
+console.log(proxyWithPlainObjectLiteral.someProperty); // Unrelated user-provided data
+
+const proxyWithNullPrototypeObject = new Proxy(objectToProxy, {
+  __proto__: null,
+  has() { return false; },
+});
+console.log(proxyWithNullPrototypeObject.someProperty); // genuine value
+```
diff --git a/lib/internal/debugger/inspect.js b/lib/internal/debugger/inspect.js
@@ -117,6 +117,7 @@ function createAgentProxy(domain, client) {
   };
 
   return new Proxy(agent, {
+    __proto__: null,
     get(target, name) {
       if (name in target) return target[name];
       return function callVirtualMethod(params) {
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -987,6 +987,7 @@ function trackAssignmentsTypedArray(typedArray) {
   }
 
   return new Proxy(typedArray, {
+    __proto__: null,
     get(obj, prop, receiver) {
       if (prop === 'copyAssigned') {
         return copyAssigned;
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -210,6 +210,8 @@ const wrapper = [
 ];
 
 let wrapperProxy = new Proxy(wrapper, {
+  __proto__: null,
+
   set(target, property, value, receiver) {
     patched = true;
     return ReflectSet(target, property, value, receiver);
@@ -718,6 +720,8 @@ function emitCircularRequireWarning(prop) {
 // A Proxy that can be used as the prototype of a module.exports object and
 // warns when non-existent properties are accessed.
 const CircularRequirePrototypeWarningProxy = new Proxy({}, {
+  __proto__: null,
+
   get(target, prop) {
     // Allow __esModule access in any case because it is used in the output
     // of transpiled code to determine whether something comes from an
diff --git a/test/parallel/test-eslint-avoid-prototype-pollution.js b/test/parallel/test-eslint-avoid-prototype-pollution.js
@@ -45,6 +45,10 @@ new RuleTester({
       'ReflectDefineProperty({}, "key", { "__proto__": null })',
       'ObjectDefineProperty({}, "key", { \'__proto__\': null })',
       'ReflectDefineProperty({}, "key", { \'__proto__\': null })',
+      'new Proxy({}, otherObject)',
+      'new Proxy({}, someFactory())',
+      'new Proxy({}, { __proto__: null })',
+      'new Proxy({}, { __proto__: null, ...{} })',
     ],
     invalid: [
       {
@@ -183,5 +187,21 @@ new RuleTester({
         code: 'StringPrototypeSplit("some string", /some regex/)',
         errors: [{ message: /looks up the Symbol\.split property/ }],
       },
+      {
+        code: 'new Proxy({}, {})',
+        errors: [{ message: /null-prototype/ }]
+      },
+      {
+        code: 'new Proxy({}, { [`__proto__`]: null })',
+        errors: [{ message: /null-prototype/ }]
+      },
+      {
+        code: 'new Proxy({}, { __proto__: Object.prototype })',
+        errors: [{ message: /null-prototype/ }]
+      },
+      {
+        code: 'new Proxy({}, { ...{ __proto__: null } })',
+        errors: [{ message: /null-prototype/ }]
+      },
     ]
   });
diff --git a/tools/eslint-rules/avoid-prototype-pollution.js b/tools/eslint-rules/avoid-prototype-pollution.js
@@ -128,6 +128,23 @@ module.exports = {
       ...createUnsafeStringMethodReport(context, 'StringPrototypeReplaceAll', 'Symbol.replace'),
       ...createUnsafeStringMethodReport(context, 'StringPrototypeSearch', 'Symbol.search'),
       ...createUnsafeStringMethodReport(context, 'StringPrototypeSplit', 'Symbol.split'),
+
+      'NewExpression[callee.name="Proxy"][arguments.1.type="ObjectExpression"]'(node) {
+        for (const { key, value } of node.arguments[1].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;
+          }
+        }
+        context.report({
+          node,
+          message: 'Proxy handler must be a null-prototype object'
+        });
+      }
     };
   },
 };

Original file line number	Diff line number	Diff line change
`@@ -987,6 +987,7 @@ function trackAssignmentsTypedArray(typedArray) {`
`987`	`987`	`}`
`988`	`988`
`989`	`989`	`return new Proxy(typedArray, {`
	`990`	`+ __proto__: null,`
`990`	`991`	`get(obj, prop, receiver) {`
`991`	`992`	`if (prop === 'copyAssigned') {`
`992`	`993`	`return copyAssigned;`