From 94056a5908fe31dfaf20e4ba776941186717bd83 Mon Sep 17 00:00:00 2001 From: jean Date: Thu, 1 Feb 2024 15:49:29 +0100 Subject: [PATCH 1/2] refactor: create and use RequireCallExpressionWalker class in isRequire probe --- src/ProbeRunner.js | 2 +- .../isRequire/RequireCallExpressionWalker.js | 93 +++++++++++++++++++ src/probes/{ => isRequire}/isRequire.js | 74 +-------------- test/probes/isRequire.spec.js | 10 +- 4 files changed, 101 insertions(+), 78 deletions(-) create mode 100644 src/probes/isRequire/RequireCallExpressionWalker.js rename src/probes/{ => isRequire}/isRequire.js (61%) diff --git a/src/ProbeRunner.js b/src/ProbeRunner.js index 08926db..4172f62 100644 --- a/src/ProbeRunner.js +++ b/src/ProbeRunner.js @@ -7,7 +7,7 @@ import isLiteral from "./probes/isLiteral.js"; import isLiteralRegex from "./probes/isLiteralRegex.js"; import isRegexObject from "./probes/isRegexObject.js"; import isVariableDeclaration from "./probes/isVariableDeclaration.js"; -import isRequire from "./probes/isRequire.js"; +import isRequire from "./probes/isRequire/isRequire.js"; import isImportDeclaration from "./probes/isImportDeclaration.js"; import isMemberExpression from "./probes/isMemberExpression.js"; import isArrayExpression from "./probes/isArrayExpression.js"; diff --git a/src/probes/isRequire/RequireCallExpressionWalker.js b/src/probes/isRequire/RequireCallExpressionWalker.js new file mode 100644 index 0000000..4a58d3e --- /dev/null +++ b/src/probes/isRequire/RequireCallExpressionWalker.js @@ -0,0 +1,93 @@ +// Import Node.js Dependencies +import path from "node:path"; + +// Import Third-party Dependencies +import { Hex } from "@nodesecure/sec-literal"; +import { walk as doWalk } from "estree-walker"; +import { + arrayExpressionToString, + getMemberExpressionIdentifier, + getCallExpressionArguments +} from "@nodesecure/estree-ast-utils"; + +export class RequireCallExpressionWalker { + constructor(tracer) { + this.tracer = tracer; + this.dependencies = new Set(); + this.triggerWarning = true; + } + + walk(nodeToWalk) { + this.dependencies = new Set(); + this.triggerWarning = true; + + // we need the `this` context of doWalk.enter + const self = this; + doWalk(nodeToWalk, { + enter(node) { + if (node.type !== "CallExpression" || node.arguments.length === 0) { + return; + } + + const rootArgument = node.arguments.at(0); + if (rootArgument.type === "Literal" && Hex.isHex(rootArgument.value)) { + self.dependencies.add(Buffer.from(rootArgument.value, "hex").toString()); + this.skip(); + + return; + } + + const fullName = node.callee.type === "MemberExpression" ? + [...getMemberExpressionIdentifier(node.callee)].join(".") : + node.callee.name; + const tracedFullName = self.tracer.getDataFromIdentifier(fullName)?.identifierOrMemberExpr ?? fullName; + switch (tracedFullName) { + case "atob": + self.#handleAtob(node); + break; + case "Buffer.from": + self.#handleBufferFrom(node); + break; + case "require.resolve": + self.#handleRequireResolve(rootArgument); + break; + case "path.join": + self.#handlePathJoin(node); + break; + } + } + }); + + return { dependencies: this.dependencies, triggerWarning: this.triggerWarning }; + } + + #handleAtob(node) { + const nodeArguments = getCallExpressionArguments(node, { tracer: this.tracer }); + if (nodeArguments !== null) { + this.dependencies.add(Buffer.from(nodeArguments.at(0), "base64").toString()); + } + } + + #handleBufferFrom(node) { + const [element] = node.arguments; + if (element.type === "ArrayExpression") { + const depName = [...arrayExpressionToString(element)].join("").trim(); + this.dependencies.add(depName); + } + } + + #handleRequireResolve(rootArgument) { + if (rootArgument.type === "Literal") { + this.dependencies.add(rootArgument.value); + } + } + + #handlePathJoin(node) { + if (!node.arguments.every((arg) => arg.type === "Literal" && typeof arg.value === "string")) { + return; + } + const constructedPath = path.posix.join(...node.arguments.map((arg) => arg.value)); + this.dependencies.add(constructedPath); + this.triggerWarning = false; + } +} diff --git a/src/probes/isRequire.js b/src/probes/isRequire/isRequire.js similarity index 61% rename from src/probes/isRequire.js rename to src/probes/isRequire/isRequire.js index d1966ab..cb568c4 100644 --- a/src/probes/isRequire.js +++ b/src/probes/isRequire/isRequire.js @@ -13,7 +13,8 @@ import { getCallExpressionIdentifier, getCallExpressionArguments } from "@nodesecure/estree-ast-utils"; -import { ProbeSignals } from "../ProbeRunner.js"; +import { ProbeSignals } from "../../ProbeRunner.js"; +import { RequireCallExpressionWalker } from "./RequireCallExpressionWalker.js"; function validateNodeRequire(node, { tracer }) { const id = getCallExpressionIdentifier(node, { @@ -53,7 +54,6 @@ function teardown({ analysis }) { analysis.dependencyAutoWarning = false; } - function main(node, options) { const { analysis, data: calleeName } = options; const { tracer } = analysis; @@ -123,7 +123,8 @@ function main(node, options) { // require(Buffer.from("...", "hex").toString()); case "CallExpression": { - const { dependencies, triggerWarning } = walkRequireCallExpression(arg, tracer); + const walker = new RequireCallExpressionWalker(tracer); + const { dependencies, triggerWarning } = walker.walk(arg); dependencies.forEach((depName) => analysis.addDependency(depName, node.loc, true)); if (triggerWarning) { @@ -139,73 +140,6 @@ function main(node, options) { } } -function walkRequireCallExpression(nodeToWalk, tracer) { - const dependencies = new Set(); - let triggerWarning = true; - - walk(nodeToWalk, { - enter(node) { - if (node.type !== "CallExpression" || node.arguments.length === 0) { - return; - } - - const rootArgument = node.arguments.at(0); - if (rootArgument.type === "Literal" && Hex.isHex(rootArgument.value)) { - dependencies.add(Buffer.from(rootArgument.value, "hex").toString()); - - return this.skip(); - } - - const fullName = node.callee.type === "MemberExpression" ? - [...getMemberExpressionIdentifier(node.callee)].join(".") : - node.callee.name; - const tracedFullName = tracer.getDataFromIdentifier(fullName)?.identifierOrMemberExpr ?? fullName; - - switch (tracedFullName) { - case "atob": { - const nodeArguments = getCallExpressionArguments(node, { tracer }); - if (nodeArguments !== null) { - dependencies.add( - Buffer.from(nodeArguments.at(0), "base64").toString() - ); - } - - break; - } - case "Buffer.from": { - const [element] = node.arguments; - - if (element.type === "ArrayExpression") { - const depName = [...arrayExpressionToString(element)].join("").trim(); - dependencies.add(depName); - } - break; - } - case "require.resolve": { - if (rootArgument.type === "Literal") { - dependencies.add(rootArgument.value); - } - break; - } - - case "path.join": { - if (!node.arguments.every((arg) => arg.type === "Literal" && typeof arg.value === "string")) { - break; - } - - const constructedPath = path.posix.join(...node.arguments.map((arg) => arg.value)); - dependencies.add(constructedPath); - - triggerWarning = false; - break; - } - } - } - }); - - return { dependencies, triggerWarning }; -} - export default { name: "isRequire", validateNode: [validateNodeRequire, validateNodeEvalRequire], diff --git a/test/probes/isRequire.spec.js b/test/probes/isRequire.spec.js index 9bb4fa0..6acac2c 100644 --- a/test/probes/isRequire.spec.js +++ b/test/probes/isRequire.spec.js @@ -4,7 +4,7 @@ import assert from "node:assert"; // Import Internal Dependencies import { getSastAnalysis, parseScript } from "../utils/index.js"; -import isRequire from "../../src/probes/isRequire.js"; +import isRequire from "../../src/probes/isRequire/isRequire.js"; test("it should ignore require CallExpression with no (zero) arguments", () => { const str = ` @@ -360,17 +360,13 @@ test("(require CallExpression): it should detect MemberExpression require.resolv test("(require CallExpression): it should detect obfuscated atob value", () => { const str = ` const myFunc = atob; - const ff = myFunc('b3' + 'M='); - const dep = require(ff); + const dep = require(myFunc('b3' + 'M=')); `; + const ast = parseScript(str); const sastAnalysis = getSastAnalysis(str, isRequire) .execute(ast.body); - assert.strictEqual(sastAnalysis.warnings().length, 0); - const dependencies = sastAnalysis.dependencies(); assert.strictEqual(dependencies.size, 1); - assert.ok(dependencies.has("os")); }); - From 2b1658aca1c193a41a8bcce0e1e3b99d446731da Mon Sep 17 00:00:00 2001 From: jean Date: Thu, 1 Feb 2024 15:58:33 +0100 Subject: [PATCH 2/2] test: should detect unsafe-import when detect obfuscated atob value --- test/probes/isRequire.spec.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/probes/isRequire.spec.js b/test/probes/isRequire.spec.js index 6acac2c..c68b88e 100644 --- a/test/probes/isRequire.spec.js +++ b/test/probes/isRequire.spec.js @@ -367,6 +367,11 @@ test("(require CallExpression): it should detect obfuscated atob value", () => { const sastAnalysis = getSastAnalysis(str, isRequire) .execute(ast.body); + assert.strictEqual(sastAnalysis.warnings().length, 1); + const warning = sastAnalysis.getWarning("unsafe-import"); + assert.strictEqual(warning.kind, "unsafe-import"); + const dependencies = sastAnalysis.dependencies(); assert.strictEqual(dependencies.size, 1); + assert.ok(dependencies.has("os")); });