From 3fb520187f2c552e4f3d2c74adbf9622e56db1a7 Mon Sep 17 00:00:00 2001 From: tchapacan Date: Fri, 22 Dec 2023 14:53:21 +0100 Subject: [PATCH] refacto update isrequire/isunsafecallee, remove new probe --- src/probes/index.js | 4 +-- src/probes/isRequire.js | 40 ++++++++++++++++++----- src/probes/isUnsafeCallee.js | 2 -- src/probes/isUnsafeEvalRequire.js | 40 ----------------------- src/utils.js | 3 +- test/issues/179-UnsafeEvalRequire.spec.js | 3 ++ test/probes/isUnsafeEvalRequire.spec.js | 35 -------------------- 7 files changed, 38 insertions(+), 89 deletions(-) delete mode 100644 src/probes/isUnsafeEvalRequire.js delete mode 100644 test/probes/isUnsafeEvalRequire.spec.js diff --git a/src/probes/index.js b/src/probes/index.js index 4a778f5..e574b5d 100644 --- a/src/probes/index.js +++ b/src/probes/index.js @@ -15,17 +15,15 @@ import isUnaryExpression from "./isUnaryExpression.js"; import isWeakCrypto from "./isWeakCrypto.js"; import isClassDeclaration from "./isClassDeclaration.js"; import isMethodDefinition from "./isMethodDefinition.js"; -import isUnsafeEvalRequire from "./isUnsafeEvalRequire.js"; // CONSTANTS const kListOfProbes = [ - isUnsafeEvalRequire, + isRequire, isUnsafeCallee, isLiteral, isLiteralRegex, isRegexObject, isVariableDeclaration, - isRequire, isImportDeclaration, isMemberExpression, isAssignmentExpression, diff --git a/src/probes/isRequire.js b/src/probes/isRequire.js index 03c08d8..70d406a 100644 --- a/src/probes/isRequire.js +++ b/src/probes/isRequire.js @@ -11,28 +11,50 @@ import { getCallExpressionArguments } from "@nodesecure/estree-ast-utils"; -function validateNode(node, { tracer }) { +function validateNode(node, option) { const id = getCallExpressionIdentifier(node); + const argument = getCallExpressionArguments(node); + if (id === null) { return [false]; } - const data = tracer.getDataFromIdentifier(id); + const data = option.tracer.getDataFromIdentifier(id); + + if (data !== null && data.name === "require") { + return [ + true, + data?.identifierOrMemberExpr ?? void 0 + ]; + } + else if (id === "eval" && argument[0] === "require") { + return [ + true, + option?.identifiersName[0].name ?? void 0 + ]; + } return [ - data !== null && data.name === "require", - data?.identifierOrMemberExpr ?? void 0 + false, + null ]; } function main(node, options) { - const { analysis } = options; + const { analysis, data: calleeName } = options; const { tracer } = analysis; if (node.arguments.length === 0) { return; } const arg = node.arguments.at(0); + const id = getCallExpressionIdentifier(node); + + + if (arg.value === "require" && id === "eval") { + analysis.addWarning("unsafe-import", calleeName, node.loc); + analysis.addDependency(calleeName, node.loc, true); + } switch (arg.type) { // const foo = "http"; require(foo); @@ -48,12 +70,14 @@ function main(node, options) { } break; - // require("http") + // require("http") case "Literal": - analysis.addDependency(arg.value, node.loc); + if (arg.value !== "require") { + analysis.addDependency(arg.value, node.loc); + } break; - // require(["ht", "tp"]) + // require(["ht", "tp"]) case "ArrayExpression": { const value = [...arrayExpressionToString(arg, { tracer })] .join("") diff --git a/src/probes/isUnsafeCallee.js b/src/probes/isUnsafeCallee.js index c444e27..62512b3 100644 --- a/src/probes/isUnsafeCallee.js +++ b/src/probes/isUnsafeCallee.js @@ -15,8 +15,6 @@ function main(node, options) { const { analysis, data: calleeName } = options; analysis.addWarning("unsafe-stmt", calleeName, node.loc); - - return Symbol.for("skipWalk"); } export default { diff --git a/src/probes/isUnsafeEvalRequire.js b/src/probes/isUnsafeEvalRequire.js deleted file mode 100644 index b82894d..0000000 --- a/src/probes/isUnsafeEvalRequire.js +++ /dev/null @@ -1,40 +0,0 @@ -// Import Internal Dependencies -import { - getCallExpressionArguments, - getCallExpressionIdentifier -} from "@nodesecure/estree-ast-utils"; - -/** - * @description Detect unsafe import - * @example - * const stream = eval('require')('stream'); - */ -function validateNode(node) { - const identifier = getCallExpressionIdentifier(node); - const argument = getCallExpressionArguments(node); - - const isUnsafeEvalRequire = ( - identifier && - identifier === "eval" && - node.callee.arguments && - node.arguments.at(0).value && - node.callee.arguments.at(0).value === "require"); - - return [ - isUnsafeEvalRequire, - isUnsafeEvalRequire && argument[0] - ]; -} - -function main(node, options) { - const { analysis, data: calleeName } = options; - - analysis.addWarning("unsafe-import", calleeName, node.loc); -} - -export default { - name: "isUnsafeEvalRequire", - validateNode, - main, - breakOnMatch: false -}; diff --git a/src/utils.js b/src/utils.js index 312bb15..0f6919c 100644 --- a/src/utils.js +++ b/src/utils.js @@ -13,7 +13,8 @@ export function isUnsafeCallee(node) { // For Function we are looking for this: `Function("...")();` // A double CallExpression return [ - identifier === "eval" || (identifier === "Function" && node.callee.type === "CallExpression"), + (identifier === "eval" && node.callee.type === "Identifier") || + (identifier === "Function" && node.callee.type === "CallExpression"), identifier ]; } diff --git a/test/issues/179-UnsafeEvalRequire.spec.js b/test/issues/179-UnsafeEvalRequire.spec.js index 9b8357c..8e089c5 100644 --- a/test/issues/179-UnsafeEvalRequire.spec.js +++ b/test/issues/179-UnsafeEvalRequire.spec.js @@ -21,4 +21,7 @@ test("should detect unsafe-import and unsafe-statement", () => { assert.equal(sastAnalysis.warnings.at(1).value, "eval"); assert.equal(sastAnalysis.warnings.at(1).kind, kWarningUnsafeStatement); assert.equal(sastAnalysis.warnings.length, 2); + assert.equal(sastAnalysis.dependencies.has("stream"), true); + assert.equal(sastAnalysis.dependencies.get("stream").unsafe, true); + assert.equal(sastAnalysis.dependencies.size, 1); }); diff --git a/test/probes/isUnsafeEvalRequire.spec.js b/test/probes/isUnsafeEvalRequire.spec.js deleted file mode 100644 index f45c4ec..0000000 --- a/test/probes/isUnsafeEvalRequire.spec.js +++ /dev/null @@ -1,35 +0,0 @@ -// Import Node.js dependencies -import { test } from "node:test"; -import assert from "node:assert"; - -// Import Internal Dependencies -import { parseScript, getSastAnalysis } from "../utils/index.js"; -import isUnsafeEvalRequire from "../../src/probes/isUnsafeEvalRequire.js"; - -// CONSTANTS -const kWarningUnsafeImport = "unsafe-import"; - -test("should detect unsafe-import", () => { - const str = "const stream = eval('require')('stream');"; - - const ast = parseScript(str); - const sastAnalysis = getSastAnalysis(str, isUnsafeEvalRequire) - .execute(ast.body); - - const result = sastAnalysis.getWarning(kWarningUnsafeImport); - assert.equal(result.kind, kWarningUnsafeImport); - assert.equal(result.value, "stream"); - assert.equal(sastAnalysis.analysis.warnings.length, 1); -}); - -test("should not detect unsafe import", () => { - const str = "const stream = eval('fastify')('express');"; - - const ast = parseScript(str); - const sastAnalysis = getSastAnalysis(str, isUnsafeEvalRequire) - .execute(ast.body); - - const result = sastAnalysis.getWarning(kWarningUnsafeImport); - assert.equal(result, undefined); - assert.equal(sastAnalysis.analysis.warnings.length, 0); -});