refactor: consider Function("return this") as safe (#211)

NodeSecure · Jan 31, 2024 · 7422acb · 7422acb
1 parent be5c3d9
commit 7422acb
Show file tree

Hide file tree

Showing 10 changed files with 213 additions and 20 deletions.
diff --git a/src/probes/isUnsafeCallee.js b/src/probes/isUnsafeCallee.js
@@ -15,6 +15,13 @@ function validateNode(node) {
 function main(node, options) {
   const { analysis, data: calleeName } = options;
 
+  if (
+    calleeName === "Function" &&
+    node.callee.arguments.length > 0 &&
+    node.callee.arguments[0].value === "return this"
+  ) {
+    return ProbeSignals.Skip;
+  }
   analysis.addWarning("unsafe-stmt", calleeName, node.loc);
 
   return ProbeSignals.Skip;

diff --git a/test/issues/180-logicalexpr-return-this.spec.js b/test/issues/180-logicalexpr-return-this.spec.js
@@ -0,0 +1,21 @@
+// Import Node.js Dependencies
+import { test } from "node:test";
+import assert from "node:assert";
+
+// Import Internal Dependencies
+import { runASTAnalysis } from "../../index.js";
+
+/**
+ * @see https://github.com/NodeSecure/js-x-ray/issues/180
+ */
+test("should detect required core 'http' with a LogicalExpr containing Function('return this')()", () => {
+  const { warnings, dependencies } = runASTAnalysis(`
+    var root = freeGlobal || freeSelf || Function('return this')();
+    const foo = root.require;
+    foo("http");
+  `);
+
+  assert.strictEqual(warnings.length, 0);
+  assert.strictEqual(dependencies.size, 1);
+  assert.ok(dependencies.has("http"));
+});
diff --git a/test/probes/isUnsafeCallee.spec.js b/test/probes/isUnsafeCallee.spec.js
@@ -21,9 +21,19 @@ test("should detect eval", () => {
   assert.equal(result.value, "eval");
 });
 
-test("should detect Function", () => {
+test("should not detect warnings for Function with return this", () => {
   const str = "Function(\"return this\")()";
 
+  const ast = parseScript(str);
+  const sastAnalysis = getSastAnalysis(str, isUnsafeCallee)
+    .execute(ast.body);
+
+  assert.strictEqual(sastAnalysis.warnings.length, 0);
+});
+
+test("should detect for unsafe Function statement", () => {
+  const str = "Function(\"anything in here\")()";
+
   const ast = parseScript(str);
   const sastAnalysis = getSastAnalysis(str, isUnsafeCallee)
     .execute(ast.body);

diff --git a/workspaces/estree-ast-utils/README.md b/workspaces/estree-ast-utils/README.md
@@ -115,6 +115,27 @@ Return `true` if the given Node is a Literal Regex Node.
 
 </details>
 
+<details><summary>extractLogicalExpression(node)</summary>
+
+Extract all LogicalExpression recursively and return an IterableIterator of 
+
+```ts
+{ operator: "||" | "&&" | "??", node: any }
+```
+
+For the following code example
+
+```js
+freeGlobal || freeSelf || Function('return this')();
+```
+
+The extract will return three parts
+- freeGlobal
+- freeSelf
+- and finally `Function('return this')();`
+
+</details>
+
 ## License
 
 MIT
diff --git a/workspaces/estree-ast-utils/src/extractLogicalExpression.js b/workspaces/estree-ast-utils/src/extractLogicalExpression.js
@@ -0,0 +1,22 @@
+
+export function* extractLogicalExpression(
+  node
+) {
+  if (node.type !== "LogicalExpression") {
+    return;
+  }
+
+  if (node.left.type === "LogicalExpression") {
+    yield* extractLogicalExpression(node.left);
+  }
+  else {
+    yield { operator: node.operator, node: node.left };
+  }
+
+  if (node.right.type === "LogicalExpression") {
+    yield* extractLogicalExpression(node.right);
+  }
+  else {
+    yield { operator: node.operator, node: node.right };
+  }
+}
diff --git a/workspaces/estree-ast-utils/src/index.d.ts b/workspaces/estree-ast-utils/src/index.d.ts
@@ -3,6 +3,10 @@ import { VariableTracer } from "./utils/VariableTracer";
 
 export { VariableTracer };
 
+export function extractLogicalExpression(
+  node: any
+): IterableIterator<{ operator: "||" | "&&" | "??", node: any }>;
+
 export function arrayExpressionToString(
   node: any, options?: { tracer?: VariableTracer }
 ): IterableIterator<string>;

diff --git a/workspaces/estree-ast-utils/src/index.js b/workspaces/estree-ast-utils/src/index.js
@@ -5,5 +5,6 @@ export * from "./getCallExpressionArguments.js";
 export * from "./concatBinaryExpression.js";
 export * from "./arrayExpressionToString.js";
 export * from "./isLiteralRegex.js";
+export * from "./extractLogicalExpression.js";
 
 export * from "./utils/VariableTracer.js";
diff --git a/workspaces/estree-ast-utils/src/utils/VariableTracer.js b/workspaces/estree-ast-utils/src/utils/VariableTracer.js
@@ -9,6 +9,7 @@ import { getMemberExpressionIdentifier } from "../getMemberExpressionIdentifier.
 import { getCallExpressionIdentifier } from "../getCallExpressionIdentifier.js";
 import { getVariableDeclarationIdentifiers } from "../getVariableDeclarationIdentifiers.js";
 import { getCallExpressionArguments } from "../getCallExpressionArguments.js";
+import { extractLogicalExpression } from "../extractLogicalExpression.js";
 
 // CONSTANTS
 const kGlobalIdentifiersToTrace = new Set([
@@ -233,10 +234,8 @@ export class VariableTracer extends EventEmitter {
     }
   }
 
-  #walkRequireCallExpression(variableDeclaratorNode) {
-    const { init, id } = variableDeclaratorNode;
-
-    const moduleNameLiteral = init.arguments
+  #walkRequireCallExpression(node, id) {
+    const moduleNameLiteral = node.arguments
       .find((argumentNode) => argumentNode.type === "Literal" && this.#traced.has(argumentNode.value));
     if (!moduleNameLiteral) {
       return;
@@ -256,17 +255,48 @@ export class VariableTracer extends EventEmitter {
   }
 
   #walkVariableDeclarationWithIdentifier(variableDeclaratorNode) {
-    const { init, id } = variableDeclaratorNode;
+    const { init } = variableDeclaratorNode;
 
     switch (init.type) {
+      /**
+       * var root = freeGlobal || freeSelf || Function('return this')();
+       */
+      case "LogicalExpression": {
+        for (const { node } of extractLogicalExpression(init)) {
+          this.#walkVariableDeclarationInitialization(
+            variableDeclaratorNode,
+            node
+          );
+        }
+
+        return void 0;
+      }
+
+      default:
+        return this.#walkVariableDeclarationInitialization(
+          variableDeclaratorNode
+        );
+    }
+  }
+
+  #walkVariableDeclarationInitialization(
+    variableDeclaratorNode,
+    childNode = variableDeclaratorNode.init
+  ) {
+    const { id } = variableDeclaratorNode;
+
+    switch (childNode.type) {
       // let foo = "10"; <-- "foo" is the key and "10" the value
       case "Literal":
-        this.literalIdentifiers.set(id.name, init.value);
+        this.literalIdentifiers.set(id.name, childNode.value);
         break;
 
-      // const g = eval("this");
+      /**
+       * const g = eval("this");
+       * const g = Function("return this")();
+       */
       case "CallExpression": {
-        const fullIdentifierPath = getCallExpressionIdentifier(init);
+        const fullIdentifierPath = getCallExpressionIdentifier(childNode);
         if (fullIdentifierPath === null) {
           break;
         }
@@ -277,18 +307,18 @@ export class VariableTracer extends EventEmitter {
         // const id = Function.prototype.call.call(require, require, "http");
         if (this.#neutralCallable.has(identifierName) || isEvilIdentifierPath(fullIdentifierPath)) {
           // TODO: make sure we are walking on a require CallExpr here ?
-          this.#walkRequireCallExpression(variableDeclaratorNode);
+          this.#walkRequireCallExpression(childNode, id);
         }
         else if (kUnsafeGlobalCallExpression.has(identifierName)) {
           this.#variablesRefToGlobal.add(id.name);
         }
         // const foo = require("crypto");
         // const bar = require.call(null, "crypto");
         else if (kRequirePatterns.has(identifierName)) {
-          this.#walkRequireCallExpression(variableDeclaratorNode);
+          this.#walkRequireCallExpression(childNode, id);
         }
         else if (tracedFullIdentifierName === "atob") {
-          const callExprArguments = getCallExpressionArguments(init, { tracer: this });
+          const callExprArguments = getCallExpressionArguments(childNode, { tracer: this });
           if (callExprArguments === null) {
             break;
           }
@@ -307,9 +337,9 @@ export class VariableTracer extends EventEmitter {
 
       // const r = require
       case "Identifier": {
-        const identifierName = init.name;
+        const identifierName = childNode.name;
         if (this.#traced.has(identifierName)) {
-          this.#declareNewAssignment(identifierName, variableDeclaratorNode.id);
+          this.#declareNewAssignment(identifierName, id);
         }
         else if (this.#isGlobalVariableIdentifier(identifierName)) {
           this.#variablesRefToGlobal.add(id.name);
@@ -321,22 +351,22 @@ export class VariableTracer extends EventEmitter {
       // process.mainModule and require.resolve
       case "MemberExpression": {
         // Example: ["process", "mainModule"]
-        const memberExprParts = [...getMemberExpressionIdentifier(init, { tracer: this })];
+        const memberExprParts = [...getMemberExpressionIdentifier(childNode, { tracer: this })];
         const memberExprFullname = memberExprParts.join(".");
 
         // Function.prototype.call
         if (isNeutralCallable(memberExprFullname)) {
-          this.#neutralCallable.add(variableDeclaratorNode.id.name);
+          this.#neutralCallable.add(id.name);
         }
         else if (this.#traced.has(memberExprFullname)) {
-          this.#declareNewAssignment(memberExprFullname, variableDeclaratorNode.id);
+          this.#declareNewAssignment(memberExprFullname, id);
         }
         else {
           const alternativeMemberExprParts = this.#searchForMemberExprAlternative(memberExprParts);
           const alternativeMemberExprFullname = alternativeMemberExprParts.join(".");
 
           if (this.#traced.has(alternativeMemberExprFullname)) {
-            this.#declareNewAssignment(alternativeMemberExprFullname, variableDeclaratorNode.id);
+            this.#declareNewAssignment(alternativeMemberExprFullname, id);
           }
         }
 
@@ -359,14 +389,14 @@ export class VariableTracer extends EventEmitter {
 
         // const {} = Function.prototype.call.call(require, require, "http");
         if (isEvilIdentifierPath(fullIdentifierPath)) {
-          this.#walkRequireCallExpression(variableDeclaratorNode);
+          this.#walkRequireCallExpression(init, id);
         }
         else if (kUnsafeGlobalCallExpression.has(identifierName)) {
           this.#autoTraceId(id);
         }
         // const { createHash } = require("crypto");
         else if (kRequirePatterns.has(identifierName)) {
-          this.#walkRequireCallExpression(variableDeclaratorNode);
+          this.#walkRequireCallExpression(init, id);
         }
 
         break;

diff --git a/workspaces/estree-ast-utils/test/VariableTracer/assignments.spec.js b/workspaces/estree-ast-utils/test/VariableTracer/assignments.spec.js
@@ -131,3 +131,27 @@ test("it should be able to Trace a require Assignment with atob", (tape) => {
 
   tape.end();
 });
+
+test("it should be able to Trace a global assignment using a LogicalExpression", (tape) => {
+  const helpers = createTracer(true);
+  const assignments = helpers.getAssignmentArray();
+
+  helpers.walkOnCode(`
+    var root = freeGlobal || freeSelf || Function('return this')();
+    const foo = root.require;
+    foo("http");
+  `);
+  const foo = helpers.tracer.getDataFromIdentifier("foo");
+  tape.deepEqual(foo, {
+    name: "require",
+    identifierOrMemberExpr: "require",
+    assignmentMemory: ["foo"]
+  });
+  tape.strictEqual(assignments.length, 1);
+
+  const [eventOne] = assignments;
+  tape.strictEqual(eventOne.identifierOrMemberExpr, "require");
+  tape.strictEqual(eventOne.id, "foo");
+
+  tape.end();
+});
diff --git a/workspaces/estree-ast-utils/test/extractLogicalExpression.spec.js b/workspaces/estree-ast-utils/test/extractLogicalExpression.spec.js
@@ -0,0 +1,53 @@
+// Import Third-party Dependencies
+import test from "tape";
+
+// Import Internal Dependencies
+import { extractLogicalExpression } from "../src/index.js";
+import { codeToAst, getExpressionFromStatement } from "./utils.js";
+
+test("it should extract two Nodes from a LogicalExpression with two operands", (tape) => {
+  const [astNode] = codeToAst("5 || 10");
+  const iter = extractLogicalExpression(
+    getExpressionFromStatement(astNode)
+  );
+
+  const iterResult = [...iter];
+  tape.strictEqual(iterResult.length, 2);
+
+  tape.strictEqual(iterResult[0].operator, "||");
+  tape.strictEqual(iterResult[0].node.type, "Literal");
+  tape.strictEqual(iterResult[0].node.value, 5);
+
+  tape.strictEqual(iterResult[1].operator, "||");
+  tape.strictEqual(iterResult[1].node.type, "Literal");
+  tape.strictEqual(iterResult[1].node.value, 10);
+
+  tape.end();
+});
+
+test("it should extract all nodes and add up all Literal values", (tape) => {
+  const [astNode] = codeToAst("5 || 10 || 15 || 20");
+  const iter = extractLogicalExpression(
+    getExpressionFromStatement(astNode)
+  );
+
+  const total = [...iter]
+    .reduce((previous, { node }) => previous + node.value, 0);
+  tape.strictEqual(total, 50);
+
+  tape.end();
+});
+
+test("it should extract all Nodes but with different operators and a LogicalExpr on the right", (tape) => {
+  const [astNode] = codeToAst("5 || 10 && 55");
+  const iter = extractLogicalExpression(
+    getExpressionFromStatement(astNode)
+  );
+
+  const operators = new Set(
+    [...iter].map(({ operator }) => operator)
+  );
+  tape.deepEqual([...operators], ["||", "&&"]);
+
+  tape.end();
+});