From 045f7047b99f0b56a319b73c8c9eff416323845f Mon Sep 17 00:00:00 2001 From: Jean <110341611+jean-michelet@users.noreply.github.com> Date: Fri, 2 Feb 2024 18:49:12 +0100 Subject: [PATCH] refactor(probe/isRequire): use new class RequireCallExpressionWalker (#231) * refactor: create and use RequireCallExpressionWalker class in isRequire probe * test: should detect unsafe-import when detect obfuscated atob value --- src/ProbeRunner.js | 2 +- .../isRequire/RequireCallExpressionWalker.js | 93 +++++++++++++++++++ src/probes/{ => isRequire}/isRequire.js | 81 +--------------- test/probes/isRequire.spec.js | 11 ++- 4 files changed, 104 insertions(+), 83 deletions(-) create mode 100644 src/probes/isRequire/RequireCallExpressionWalker.js rename src/probes/{ => isRequire}/isRequire.js (57%) diff --git a/src/ProbeRunner.js b/src/ProbeRunner.js index df29e61..56c4cb9 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 57% rename from src/probes/isRequire.js rename to src/probes/isRequire/isRequire.js index 325b141..83b44ee 100644 --- a/src/probes/isRequire.js +++ b/src/probes/isRequire/isRequire.js @@ -1,19 +1,13 @@ /* eslint-disable consistent-return */ -// Import Node.js Dependencies -import path from "node:path"; - -// Import Third-party Dependencies -import { Hex } from "@nodesecure/sec-literal"; -import { walk } from "estree-walker"; import { concatBinaryExpression, arrayExpressionToString, - getMemberExpressionIdentifier, 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 +47,6 @@ function teardown({ sourceFile }) { sourceFile.dependencyAutoWarning = false; } - function main(node, options) { const { sourceFile, data: calleeName } = options; const { tracer } = sourceFile; @@ -123,7 +116,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) => sourceFile.addDependency(depName, node.loc, true)); if (triggerWarning) { @@ -139,73 +133,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..c68b88e 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,18 @@ 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); + 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")); }); -