lib: add lint rule to protect against Object.prototype.then pollution

aduh95 · aduh95 · commit fe474c0fce47 · 2022-10-19T04:22:41.000+02:00
diff --git a/lib/internal/crypto/cfrg.js b/lib/internal/crypto/cfrg.js
@@ -188,7 +188,7 @@ async function cfrgGenerateKey(algorithm, extractable, keyUsages) {
       privateUsages,
       extractable);
 
-  return { privateKey, publicKey };
+  return { __proto__: null, privateKey, publicKey };
 }
 
 function cfrgExportKey(key, format) {
diff --git a/lib/internal/crypto/ec.js b/lib/internal/crypto/ec.js
@@ -146,7 +146,7 @@ async function ecGenerateKey(algorithm, extractable, keyUsages) {
       privateUsages,
       extractable);
 
-  return { publicKey, privateKey };
+  return { __proto__: null, publicKey, privateKey };
 }
 
 function ecExportKey(key, format) {
diff --git a/lib/internal/crypto/rsa.js b/lib/internal/crypto/rsa.js
@@ -219,7 +219,7 @@ async function rsaKeyGenerate(
       privateUsages,
       extractable);
 
-  return { publicKey, privateKey };
+  return { __proto__: null, publicKey, privateKey };
 }
 
 function rsaExportKey(key, format) {
diff --git a/lib/internal/fs/cp/cp.js b/lib/internal/fs/cp/cp.js
@@ -115,7 +115,7 @@ async function checkPaths(src, dest, opts) {
       code: 'EINVAL',
     });
   }
-  return { srcStat, destStat, skipped: false };
+  return { __proto__: null, srcStat, destStat, skipped: false };
 }
 
 function areIdentical(srcStat, destStat) {
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
@@ -580,7 +580,7 @@ async function read(handle, bufferOrParams, offset, length, position) {
   length |= 0;
 
   if (length === 0)
-    return { bytesRead: length, buffer };
+    return { __proto__: null, bytesRead: length, buffer };
 
   if (buffer.byteLength === 0) {
     throw new ERR_INVALID_ARG_VALUE('buffer', buffer,
@@ -595,7 +595,7 @@ async function read(handle, bufferOrParams, offset, length, position) {
   const bytesRead = (await binding.read(handle.fd, buffer, offset, length,
                                         position, kUsePromises)) || 0;
 
-  return { bytesRead, buffer };
+  return { __proto__: null, bytesRead, buffer };
 }
 
 async function readv(handle, buffers, position) {
@@ -606,12 +606,12 @@ async function readv(handle, buffers, position) {
 
   const bytesRead = (await binding.readBuffers(handle.fd, buffers, position,
                                                kUsePromises)) || 0;
-  return { bytesRead, buffers };
+  return { __proto__: null, bytesRead, buffers };
 }
 
 async function write(handle, buffer, offsetOrOptions, length, position) {
   if (buffer?.byteLength === 0)
-    return { bytesWritten: 0, buffer };
+    return { __proto__: null, bytesWritten: 0, buffer };
 
   let offset = offsetOrOptions;
   if (isArrayBufferView(buffer)) {
@@ -636,14 +636,14 @@ async function write(handle, buffer, offsetOrOptions, length, position) {
     const bytesWritten =
       (await binding.writeBuffer(handle.fd, buffer, offset,
                                  length, position, kUsePromises)) || 0;
-    return { bytesWritten, buffer };
+    return { __proto__: null, bytesWritten, buffer };
   }
 
   validateStringAfterArrayBufferView(buffer, 'buffer');
   validateEncoding(buffer, length);
   const bytesWritten = (await binding.writeString(handle.fd, buffer, offset,
                                                   length, kUsePromises)) || 0;
-  return { bytesWritten, buffer };
+  return { __proto__: null, bytesWritten, buffer };
 }
 
 async function writev(handle, buffers, position) {
@@ -653,12 +653,12 @@ async function writev(handle, buffers, position) {
     position = null;
 
   if (buffers.length === 0) {
-    return { bytesWritten: 0, buffers };
+    return { __proto__: null, bytesWritten: 0, buffers };
   }
 
   const bytesWritten = (await binding.writeBuffers(handle.fd, buffers, position,
                                                    kUsePromises)) || 0;
-  return { bytesWritten, buffers };
+  return { __proto__: null, bytesWritten, buffers };
 }
 
 async function rename(oldPath, newPath) {
diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js
@@ -59,7 +59,7 @@ async function getSource(url, context) {
   if (policy?.manifest) {
     policy.manifest.assertIntegrity(parsed, source);
   }
-  return { responseURL, source };
+  return { __proto__: null, responseURL, source };
 }
 
 
@@ -93,6 +93,7 @@ async function defaultLoad(url, context) {
   }
 
   return {
+    __proto__: null,
     format,
     responseURL,
     source,
diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
@@ -378,6 +378,7 @@ class ESMLoader {
     const { module } = await job.run();
 
     return {
+      __proto__: null,
       namespace: module.getNamespace(),
     };
   }
@@ -664,6 +665,7 @@ class ESMLoader {
     }
 
     return {
+      __proto__: null,
       format,
       responseURL,
       source,
@@ -880,6 +882,7 @@ class ESMLoader {
     }
 
     return {
+      __proto__: null,
       format,
       url,
     };
diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js
@@ -215,7 +215,7 @@ class ModuleJob {
       }
       throw e;
     }
-    return { module: this.module };
+    return { __proto__: null, module: this.module };
   }
 }
 ObjectSetPrototypeOf(ModuleJob.prototype, null);
diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js
@@ -977,7 +977,7 @@ async function defaultResolve(specifier, context = {}) {
         missing = false;
       } else if (destination) {
         const href = destination.href;
-        return { url: href };
+        return { __proto__: null, url: href };
       }
       if (missing) {
         // Prevent network requests from firing if resolution would be banned.
@@ -1017,7 +1017,7 @@ async function defaultResolve(specifier, context = {}) {
         )
       )
     ) {
-      return { url: parsed.href };
+      return { __proto__: null, url: parsed.href };
     }
   } catch {
     // Ignore exception
@@ -1035,7 +1035,7 @@ async function defaultResolve(specifier, context = {}) {
   if (maybeReturn) return maybeReturn;
 
   // This must come after checkIfDisallowedImport
-  if (parsed && parsed.protocol === 'node:') return { url: specifier };
+  if (parsed && parsed.protocol === 'node:') return { __proto__: null, url: specifier };
 
   throwIfUnsupportedURLScheme(parsed, experimentalNetworkImports);
 
@@ -1087,6 +1087,7 @@ async function defaultResolve(specifier, context = {}) {
   throwIfUnsupportedURLProtocol(url);
 
   return {
+    __proto__: null,
     // Do NOT cast `url` to a string: that will work even when there are real
     // problems, silencing them
     url: url.href,
diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js
@@ -476,7 +476,7 @@ class ReadableStream {
 
     async function returnSteps(value) {
       if (done)
-        return { done: true, value };
+        return { done: true, value }; // eslint-disable-line node-core/avoid-prototype-pollution
       done = true;
 
       if (reader[kState].stream === undefined) {
@@ -488,11 +488,11 @@ class ReadableStream {
         const result = readableStreamReaderGenericCancel(reader, value);
         readableStreamReaderGenericRelease(reader);
         await result;
-        return { done: true, value };
+        return { done: true, value }; // eslint-disable-line node-core/avoid-prototype-pollution
       }
 
       readableStreamReaderGenericRelease(reader);
-      return { done: true, value };
+      return { done: true, value }; // eslint-disable-line node-core/avoid-prototype-pollution
     }
 
     // TODO(@jasnell): Explore whether an async generator
diff --git a/test/parallel/test-eslint-avoid-prototype-pollution.js b/test/parallel/test-eslint-avoid-prototype-pollution.js
@@ -45,6 +45,17 @@ new RuleTester({
       'ReflectDefineProperty({}, "key", { "__proto__": null })',
       'ObjectDefineProperty({}, "key", { \'__proto__\': null })',
       'ReflectDefineProperty({}, "key", { \'__proto__\': null })',
+      'async function myFn() { return { __proto__: null } }',
+      'async function myFn() { function myFn() { return {} } return { __proto__: null } }',
+      'const myFn = async function myFn() { return { __proto__: null } }',
+      'const myFn = async function () { return { __proto__: null } }',
+      'const myFn = async () => { return { __proto__: null } }',
+      'const myFn = async () => ({ __proto__: null })',
+      'function myFn() { return {} }',
+      'const myFn = function myFn() { return {} }',
+      'const myFn = function () { return {} }',
+      'const myFn = () => { return {} }',
+      'const myFn = () => ({})',
       'StringPrototypeReplace("some string", "some string", "some replacement")',
       'StringPrototypeReplaceAll("some string", "some string", "some replacement")',
       'StringPrototypeSplit("some string", "some string")',
@@ -150,6 +161,34 @@ new RuleTester({
         code: 'ReflectDefineProperty({}, "key", { enumerable: true })',
         errors: [{ message: /null-prototype/ }],
       },
+      {
+        code: 'async function myFn(){ return {} }',
+        errors: [{ message: /null-prototype/ }],
+      },
+      {
+        code: 'async function myFn(){ async function someOtherFn() { return { __proto__: null } } return {} }',
+        errors: [{ message: /null-prototype/ }],
+      },
+      {
+        code: 'async function myFn(){ if (true) { return {} } return { __proto__: null } }',
+        errors: [{ message: /null-prototype/ }],
+      },
+      {
+        code: 'const myFn = async function myFn(){ return {} }',
+        errors: [{ message: /null-prototype/ }],
+      },
+      {
+        code: 'const myFn = async function (){ return {} }',
+        errors: [{ message: /null-prototype/ }],
+      },
+      {
+        code: 'const myFn = async () => { return {} }',
+        errors: [{ message: /null-prototype/ }],
+      },
+      {
+        code: 'const myFn = async () => ({})',
+        errors: [{ message: /null-prototype/ }],
+      },
       {
         code: 'RegExpPrototypeTest(/some regex/, "some string")',
         errors: [{ message: /looks up the "exec" property/ }],
diff --git a/tools/eslint-rules/avoid-prototype-pollution.js b/tools/eslint-rules/avoid-prototype-pollution.js
@@ -1,6 +1,7 @@
 'use strict';
 
 const CallExpression = (fnName) => `CallExpression[callee.name=${fnName}]`;
+const AnyFunction = 'FunctionDeclaration, FunctionExpression, ArrowFunctionExpression';
 
 function checkProperties(context, node) {
   if (
@@ -25,6 +26,22 @@ function checkProperties(context, node) {
   }
 }
 
+function isNullPrototypeObjectExpression(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;
+    }
+  }
+  return false;
+}
+
 function checkPropertyDescriptor(context, node) {
   if (
     node.type === 'CallExpression' &&
@@ -46,23 +63,12 @@ function checkPropertyDescriptor(context, 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;
-    }
+  if (isNullPrototypeObjectExpression(node) === false) {
+    context.report({
+      node,
+      message: 'Must use null-prototype object for property descriptors',
+    });
   }
-
-  context.report({
-    node,
-    message: 'Must use null-prototype object for property descriptors',
-  });
 }
 
 function createUnsafeStringMethodReport(context, name, lookedUpProperty) {
@@ -117,6 +123,24 @@ module.exports = {
       [`${CallExpression('ObjectCreate')}[arguments.length=2]`](node) {
         checkProperties(context, node.arguments[1]);
       },
+
+      [`:matches(${AnyFunction})[async=true]>BlockStatement ReturnStatement>ObjectExpression, :matches(${AnyFunction})[async=true]>ObjectExpression`](node) {
+        if (node.parent.type === 'ReturnStatement') {
+          let { parent } = node;
+          do {
+            ({ parent } = parent);
+          } while (!parent.type.includes('Function'));
+
+          if (!parent.async) return;
+        }
+        if (isNullPrototypeObjectExpression(node) === false) {
+          context.report({
+            node,
+            message: 'Use null-prototype when returning from async function',
+          });
+        }
+      },
+
       [CallExpression('RegExpPrototypeTest')](node) {
         context.report({
           node,

Original file line number	Diff line number	Diff line change
`@@ -188,7 +188,7 @@ async function cfrgGenerateKey(algorithm, extractable, keyUsages) {`
`188`	`188`	`privateUsages,`
`189`	`189`	`extractable);`
`190`	`190`
`191`		`- return { privateKey, publicKey };`
	`191`	`+ return { __proto__: null, privateKey, publicKey };`
`192`	`192`	`}`
`193`	`193`
`194`	`194`	`function cfrgExportKey(key, format) {`
Original file line number	Diff line number	Diff line change
`@@ -146,7 +146,7 @@ async function ecGenerateKey(algorithm, extractable, keyUsages) {`
`146`	`146`	`privateUsages,`
`147`	`147`	`extractable);`
`148`	`148`
`149`		`- return { publicKey, privateKey };`
	`149`	`+ return { __proto__: null, publicKey, privateKey };`
`150`	`150`	`}`
`151`	`151`
`152`	`152`	`function ecExportKey(key, format) {`
Original file line number	Diff line number	Diff line change
`@@ -219,7 +219,7 @@ async function rsaKeyGenerate(`
`219`	`219`	`privateUsages,`
`220`	`220`	`extractable);`
`221`	`221`
`222`		`- return { publicKey, privateKey };`
	`222`	`+ return { __proto__: null, publicKey, privateKey };`
`223`	`223`	`}`
`224`	`224`
`225`	`225`	`function rsaExportKey(key, format) {`
Original file line number	Diff line number	Diff line change
`@@ -115,7 +115,7 @@ async function checkPaths(src, dest, opts) {`
`115`	`115`	`code: 'EINVAL',`
`116`	`116`	`});`
`117`	`117`	`}`
`118`		`- return { srcStat, destStat, skipped: false };`
	`118`	`+ return { __proto__: null, srcStat, destStat, skipped: false };`
`119`	`119`	`}`
`120`	`120`
`121`	`121`	`function areIdentical(srcStat, destStat) {`
Original file line number	Diff line number	Diff line change
`@@ -59,7 +59,7 @@ async function getSource(url, context) {`
`59`	`59`	`if (policy?.manifest) {`
`60`	`60`	`policy.manifest.assertIntegrity(parsed, source);`
`61`	`61`	`}`
`62`		`- return { responseURL, source };`
	`62`	`+ return { __proto__: null, responseURL, source };`
`63`	`63`	`}`
`64`	`64`
`65`	`65`
`@@ -93,6 +93,7 @@ async function defaultLoad(url, context) {`
`93`	`93`	`}`
`94`	`94`
`95`	`95`	`return {`
	`96`	`+ __proto__: null,`
`96`	`97`	`format,`
`97`	`98`	`responseURL,`
`98`	`99`	`source,`
Original file line number	Diff line number	Diff line change
`@@ -378,6 +378,7 @@ class ESMLoader {`
`378`	`378`	`const { module } = await job.run();`
`379`	`379`
`380`	`380`	`return {`
	`381`	`+ __proto__: null,`
`381`	`382`	`namespace: module.getNamespace(),`
`382`	`383`	`};`
`383`	`384`	`}`
`@@ -664,6 +665,7 @@ class ESMLoader {`
`664`	`665`	`}`
`665`	`666`
`666`	`667`	`return {`
	`668`	`+ __proto__: null,`
`667`	`669`	`format,`
`668`	`670`	`responseURL,`
`669`	`671`	`source,`
`@@ -880,6 +882,7 @@ class ESMLoader {`
`880`	`882`	`}`
`881`	`883`
`882`	`884`	`return {`
	`885`	`+ __proto__: null,`
`883`	`886`	`format,`
`884`	`887`	`url,`
`885`	`888`	`};`
Original file line number	Diff line number	Diff line change
`@@ -215,7 +215,7 @@ class ModuleJob {`
`215`	`215`	`}`
`216`	`216`	`throw e;`
`217`	`217`	`}`
`218`		`- return { module: this.module };`
	`218`	`+ return { __proto__: null, module: this.module };`
`219`	`219`	`}`
`220`	`220`	`}`
`221`	`221`	`ObjectSetPrototypeOf(ModuleJob.prototype, null);`