From c88199a7cbedaa540722e89fb3f14729d112133a Mon Sep 17 00:00:00 2001 From: tchapacan Date: Mon, 18 Dec 2023 17:27:52 +0100 Subject: [PATCH 1/9] add unsafe-import probe for eval/require --- .gitignore | 1 + src/probes/isUnsafeImport.js | 26 ++++++++++++++++++++++++ src/utils.js | 15 ++++++++++++++ test/probes/isUnsafeImport.spec.js | 32 ++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+) create mode 100644 src/probes/isUnsafeImport.js create mode 100644 test/probes/isUnsafeImport.spec.js diff --git a/.gitignore b/.gitignore index 7f815f4..b627d17 100644 --- a/.gitignore +++ b/.gitignore @@ -106,3 +106,4 @@ dist temp.js temp/ .vscode/ +.idea/ diff --git a/src/probes/isUnsafeImport.js b/src/probes/isUnsafeImport.js new file mode 100644 index 0000000..735fcef --- /dev/null +++ b/src/probes/isUnsafeImport.js @@ -0,0 +1,26 @@ +// Import Internal Dependencies +import { isUnsafeConstEvalRequireImport } from "../utils.js"; + +/** + * @description Detect unsafe import + * @example + * const stream = eval('require')('stream'); + */ +function validateNode(node) { + return isUnsafeConstEvalRequireImport(node); +} + +function main(node, options) { + const { analysis, data: calleeName } = options; + + analysis.addWarning("unsafe-import", calleeName, node.loc); + + return Symbol.for("skipWalk"); +} + +export default { + name: "isUnsafeImport", + validateNode, + main, + breakOnMatch: false +}; diff --git a/src/utils.js b/src/utils.js index 312bb15..abda9c5 100644 --- a/src/utils.js +++ b/src/utils.js @@ -18,6 +18,21 @@ export function isUnsafeCallee(node) { ]; } +export function isUnsafeConstEvalRequireImport(node) { + const isUnsafe = (node.type === "VariableDeclaration" && + node.kind === "const" && + node.declarations[0].init && + node.declarations[0].init.callee && + node.declarations[0].init.callee.callee && + node.declarations[0].init.callee.callee.name === "eval" && + node.declarations[0].init.callee.arguments[0].value === "require"); + + return [ + isUnsafe, + isUnsafe && node.declarations[0].init.arguments[0].value + ]; +} + export function rootLocation() { return { start: { line: 0, column: 0 }, end: { line: 0, column: 0 } }; } diff --git a/test/probes/isUnsafeImport.spec.js b/test/probes/isUnsafeImport.spec.js new file mode 100644 index 0000000..0339b99 --- /dev/null +++ b/test/probes/isUnsafeImport.spec.js @@ -0,0 +1,32 @@ +// 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 isUnsafeImport from "../../src/probes/isUnsafeImport.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, isUnsafeImport) + .execute(ast.body); + + const result = sastAnalysis.getWarning(kWarningUnsafeImport); + assert.equal(result.kind, kWarningUnsafeImport); + assert.equal(result.value, "stream"); +}); + +test("should not detect unsafe import", () => { + const str = "const stream = eval('fastify')('express');"; + + const ast = parseScript(str); + const sastAnalysis = getSastAnalysis(str, isUnsafeImport) + .execute(ast.body); + + const result = sastAnalysis.getWarning(kWarningUnsafeImport); + assert.equal(result, undefined); +}); From 3b230e12c3e382b09f44f1213c82fb731d0fcbd2 Mon Sep 17 00:00:00 2001 From: tchapacan Date: Mon, 18 Dec 2023 18:27:21 +0100 Subject: [PATCH 2/9] remove unecessary declaration & add eval/require both warning to unsafeimport probe --- src/probes/isUnsafeImport.js | 2 +- src/utils.js | 5 +++-- test/probes/isUnsafeImport.spec.js | 14 ++++++++++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/probes/isUnsafeImport.js b/src/probes/isUnsafeImport.js index 735fcef..9233e63 100644 --- a/src/probes/isUnsafeImport.js +++ b/src/probes/isUnsafeImport.js @@ -15,7 +15,7 @@ function main(node, options) { analysis.addWarning("unsafe-import", calleeName, node.loc); - return Symbol.for("skipWalk"); + return Symbol.for("breakWalk"); } export default { diff --git a/src/utils.js b/src/utils.js index abda9c5..699eb6a 100644 --- a/src/utils.js +++ b/src/utils.js @@ -19,12 +19,13 @@ export function isUnsafeCallee(node) { } export function isUnsafeConstEvalRequireImport(node) { - const isUnsafe = (node.type === "VariableDeclaration" && - node.kind === "const" && + const isUnsafe = (node.declarations && node.declarations[0].init && node.declarations[0].init.callee && node.declarations[0].init.callee.callee && + node.declarations[0].init.callee.callee.type === "Identifier" && node.declarations[0].init.callee.callee.name === "eval" && + node.declarations[0].init.callee.arguments[0].type === "Literal" && node.declarations[0].init.callee.arguments[0].value === "require"); return [ diff --git a/test/probes/isUnsafeImport.spec.js b/test/probes/isUnsafeImport.spec.js index 0339b99..95556b9 100644 --- a/test/probes/isUnsafeImport.spec.js +++ b/test/probes/isUnsafeImport.spec.js @@ -5,9 +5,11 @@ import assert from "node:assert"; // Import Internal Dependencies import { parseScript, getSastAnalysis } from "../utils/index.js"; import isUnsafeImport from "../../src/probes/isUnsafeImport.js"; +import isUnsafeCallee from "../../src/probes/isUnsafeCallee.js"; // CONSTANTS const kWarningUnsafeImport = "unsafe-import"; +const kWarningUnsafeStmt = "unsafe-stmt"; test("should detect unsafe import", () => { const str = "const stream = eval('require')('stream');"; @@ -20,6 +22,18 @@ test("should detect unsafe import", () => { assert.equal(result.value, "stream"); }); +test("should detect unsafe statement", () => { + const str = "const stream = eval('require')('stream');"; + + const ast = parseScript(str); + const sastAnalysis = getSastAnalysis(str, isUnsafeCallee) + .execute(ast.body); + + const result = sastAnalysis.getWarning(kWarningUnsafeStmt); + assert.equal(result.kind, kWarningUnsafeStmt); + assert.equal(result.value, "eval"); +}); + test("should not detect unsafe import", () => { const str = "const stream = eval('fastify')('express');"; From c206613ec220eebb0862dad1564fc8021ba6f7b4 Mon Sep 17 00:00:00 2001 From: tchapacan Date: Tue, 19 Dec 2023 08:29:02 +0100 Subject: [PATCH 3/9] update test & rename/simmplify new probe --- ...{isUnsafeImport.js => isUnsafeEvalRequire.js} | 13 +++++++++++-- src/utils.js | 16 ---------------- test/probes/isUnsafeImport.spec.js | 6 ++++-- 3 files changed, 15 insertions(+), 20 deletions(-) rename src/probes/{isUnsafeImport.js => isUnsafeEvalRequire.js} (58%) diff --git a/src/probes/isUnsafeImport.js b/src/probes/isUnsafeEvalRequire.js similarity index 58% rename from src/probes/isUnsafeImport.js rename to src/probes/isUnsafeEvalRequire.js index 9233e63..89a76ad 100644 --- a/src/probes/isUnsafeImport.js +++ b/src/probes/isUnsafeEvalRequire.js @@ -1,5 +1,5 @@ // Import Internal Dependencies -import { isUnsafeConstEvalRequireImport } from "../utils.js"; +import { getCallExpressionIdentifier } from "@nodesecure/estree-ast-utils"; /** * @description Detect unsafe import @@ -7,7 +7,16 @@ import { isUnsafeConstEvalRequireImport } from "../utils.js"; * const stream = eval('require')('stream'); */ function validateNode(node) { - return isUnsafeConstEvalRequireImport(node); + const identifier = getCallExpressionIdentifier(node); + + const isUnsafe = (identifier && + identifier === "eval" && + node.arguments.at(0).value === "stream"); + + return [ + isUnsafe, + isUnsafe && node.arguments.at(0).value + ]; } function main(node, options) { diff --git a/src/utils.js b/src/utils.js index 699eb6a..312bb15 100644 --- a/src/utils.js +++ b/src/utils.js @@ -18,22 +18,6 @@ export function isUnsafeCallee(node) { ]; } -export function isUnsafeConstEvalRequireImport(node) { - const isUnsafe = (node.declarations && - node.declarations[0].init && - node.declarations[0].init.callee && - node.declarations[0].init.callee.callee && - node.declarations[0].init.callee.callee.type === "Identifier" && - node.declarations[0].init.callee.callee.name === "eval" && - node.declarations[0].init.callee.arguments[0].type === "Literal" && - node.declarations[0].init.callee.arguments[0].value === "require"); - - return [ - isUnsafe, - isUnsafe && node.declarations[0].init.arguments[0].value - ]; -} - export function rootLocation() { return { start: { line: 0, column: 0 }, end: { line: 0, column: 0 } }; } diff --git a/test/probes/isUnsafeImport.spec.js b/test/probes/isUnsafeImport.spec.js index 95556b9..d7b7e79 100644 --- a/test/probes/isUnsafeImport.spec.js +++ b/test/probes/isUnsafeImport.spec.js @@ -4,7 +4,7 @@ import assert from "node:assert"; // Import Internal Dependencies import { parseScript, getSastAnalysis } from "../utils/index.js"; -import isUnsafeImport from "../../src/probes/isUnsafeImport.js"; +import isUnsafeImport from "../../src/probes/isUnsafeEvalRequire.js"; import isUnsafeCallee from "../../src/probes/isUnsafeCallee.js"; // CONSTANTS @@ -20,6 +20,7 @@ test("should detect unsafe import", () => { const result = sastAnalysis.getWarning(kWarningUnsafeImport); assert.equal(result.kind, kWarningUnsafeImport); assert.equal(result.value, "stream"); + assert.equal(sastAnalysis.analysis.warnings.length, 1); }); test("should detect unsafe statement", () => { @@ -28,10 +29,10 @@ test("should detect unsafe statement", () => { const ast = parseScript(str); const sastAnalysis = getSastAnalysis(str, isUnsafeCallee) .execute(ast.body); - const result = sastAnalysis.getWarning(kWarningUnsafeStmt); assert.equal(result.kind, kWarningUnsafeStmt); assert.equal(result.value, "eval"); + assert.equal(sastAnalysis.analysis.warnings.length, 1); }); test("should not detect unsafe import", () => { @@ -43,4 +44,5 @@ test("should not detect unsafe import", () => { const result = sastAnalysis.getWarning(kWarningUnsafeImport); assert.equal(result, undefined); + assert.equal(sastAnalysis.analysis.warnings.length, 0); }); From edea684e2929a3a115f62c8a045af0733c9c2f19 Mon Sep 17 00:00:00 2001 From: tchapacan Date: Wed, 20 Dec 2023 00:30:30 +0100 Subject: [PATCH 4/9] refacto test & rename/validate probe --- src/probes/isUnsafeEvalRequire.js | 21 +++++++++------- test/issues/179-UnsafeEvalRequire.spec.js | 24 +++++++++++++++++++ ...rt.spec.js => isUnsafeEvalRequire.spec.js} | 23 ++++-------------- 3 files changed, 42 insertions(+), 26 deletions(-) create mode 100644 test/issues/179-UnsafeEvalRequire.spec.js rename test/probes/{isUnsafeImport.spec.js => isUnsafeEvalRequire.spec.js} (54%) diff --git a/src/probes/isUnsafeEvalRequire.js b/src/probes/isUnsafeEvalRequire.js index 89a76ad..b82894d 100644 --- a/src/probes/isUnsafeEvalRequire.js +++ b/src/probes/isUnsafeEvalRequire.js @@ -1,5 +1,8 @@ // Import Internal Dependencies -import { getCallExpressionIdentifier } from "@nodesecure/estree-ast-utils"; +import { + getCallExpressionArguments, + getCallExpressionIdentifier +} from "@nodesecure/estree-ast-utils"; /** * @description Detect unsafe import @@ -8,14 +11,18 @@ import { getCallExpressionIdentifier } from "@nodesecure/estree-ast-utils"; */ function validateNode(node) { const identifier = getCallExpressionIdentifier(node); + const argument = getCallExpressionArguments(node); - const isUnsafe = (identifier && + const isUnsafeEvalRequire = ( + identifier && identifier === "eval" && - node.arguments.at(0).value === "stream"); + node.callee.arguments && + node.arguments.at(0).value && + node.callee.arguments.at(0).value === "require"); return [ - isUnsafe, - isUnsafe && node.arguments.at(0).value + isUnsafeEvalRequire, + isUnsafeEvalRequire && argument[0] ]; } @@ -23,12 +30,10 @@ function main(node, options) { const { analysis, data: calleeName } = options; analysis.addWarning("unsafe-import", calleeName, node.loc); - - return Symbol.for("breakWalk"); } export default { - name: "isUnsafeImport", + name: "isUnsafeEvalRequire", validateNode, main, breakOnMatch: false diff --git a/test/issues/179-UnsafeEvalRequire.spec.js b/test/issues/179-UnsafeEvalRequire.spec.js new file mode 100644 index 0000000..9b8357c --- /dev/null +++ b/test/issues/179-UnsafeEvalRequire.spec.js @@ -0,0 +1,24 @@ +// 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/179 + */ +// CONSTANTS +const kIncriminedCodeSample = `const stream = eval('require')('stream');`; +const kWarningUnsafeImport = "unsafe-import"; +const kWarningUnsafeStatement = "unsafe-stmt"; + +test("should detect unsafe-import and unsafe-statement", () => { + const sastAnalysis = runASTAnalysis(kIncriminedCodeSample); + + assert.equal(sastAnalysis.warnings.at(0).value, "stream"); + assert.equal(sastAnalysis.warnings.at(0).kind, kWarningUnsafeImport); + assert.equal(sastAnalysis.warnings.at(1).value, "eval"); + assert.equal(sastAnalysis.warnings.at(1).kind, kWarningUnsafeStatement); + assert.equal(sastAnalysis.warnings.length, 2); +}); diff --git a/test/probes/isUnsafeImport.spec.js b/test/probes/isUnsafeEvalRequire.spec.js similarity index 54% rename from test/probes/isUnsafeImport.spec.js rename to test/probes/isUnsafeEvalRequire.spec.js index d7b7e79..f45c4ec 100644 --- a/test/probes/isUnsafeImport.spec.js +++ b/test/probes/isUnsafeEvalRequire.spec.js @@ -4,17 +4,16 @@ import assert from "node:assert"; // Import Internal Dependencies import { parseScript, getSastAnalysis } from "../utils/index.js"; -import isUnsafeImport from "../../src/probes/isUnsafeEvalRequire.js"; -import isUnsafeCallee from "../../src/probes/isUnsafeCallee.js"; +import isUnsafeEvalRequire from "../../src/probes/isUnsafeEvalRequire.js"; // CONSTANTS const kWarningUnsafeImport = "unsafe-import"; -const kWarningUnsafeStmt = "unsafe-stmt"; -test("should detect unsafe import", () => { + +test("should detect unsafe-import", () => { const str = "const stream = eval('require')('stream');"; const ast = parseScript(str); - const sastAnalysis = getSastAnalysis(str, isUnsafeImport) + const sastAnalysis = getSastAnalysis(str, isUnsafeEvalRequire) .execute(ast.body); const result = sastAnalysis.getWarning(kWarningUnsafeImport); @@ -23,23 +22,11 @@ test("should detect unsafe import", () => { assert.equal(sastAnalysis.analysis.warnings.length, 1); }); -test("should detect unsafe statement", () => { - const str = "const stream = eval('require')('stream');"; - - const ast = parseScript(str); - const sastAnalysis = getSastAnalysis(str, isUnsafeCallee) - .execute(ast.body); - const result = sastAnalysis.getWarning(kWarningUnsafeStmt); - assert.equal(result.kind, kWarningUnsafeStmt); - assert.equal(result.value, "eval"); - 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, isUnsafeImport) + const sastAnalysis = getSastAnalysis(str, isUnsafeEvalRequire) .execute(ast.body); const result = sastAnalysis.getWarning(kWarningUnsafeImport); From ecfdf4ddb1ffba89706d0398ba679d5b6147ca2c Mon Sep 17 00:00:00 2001 From: tchapacan Date: Fri, 22 Dec 2023 14:53:21 +0100 Subject: [PATCH 5/9] refacto update isrequire/isunsafecallee, remove new probe --- src/probes/isRequire.js | 50 +++++++++++++++-------- src/probes/isUnsafeCallee.js | 7 +--- src/probes/isUnsafeEvalRequire.js | 40 ------------------ src/utils.js | 3 +- test/issues/179-UnsafeEvalRequire.spec.js | 3 ++ test/probes/isUnsafeEvalRequire.spec.js | 35 ---------------- 6 files changed, 40 insertions(+), 98 deletions(-) delete mode 100644 src/probes/isUnsafeEvalRequire.js delete mode 100644 test/probes/isUnsafeEvalRequire.spec.js diff --git a/src/probes/isRequire.js b/src/probes/isRequire.js index 138fadb..70d406a 100644 --- a/src/probes/isRequire.js +++ b/src/probes/isRequire.js @@ -11,31 +11,50 @@ import { getCallExpressionArguments } from "@nodesecure/estree-ast-utils"; -// Import Internal Dependencies -import { ProbeSignals } from "../ProbeRunner.js"; - -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); @@ -51,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("") @@ -99,7 +120,7 @@ function main(node, options) { analysis.addWarning("unsafe-import", null, node.loc); // We skip walking the tree to avoid anymore warnings... - return ProbeSignals.Skip; + return Symbol.for("skipWalk"); } default: @@ -163,8 +184,5 @@ function walkRequireCallExpression(nodeToWalk, tracer) { export default { name: "isRequire", - validateNode, - main, - breakOnMatch: true, - breakGroup: "import" + validateNode, main, breakOnMatch: true, breakGroup: "import" }; diff --git a/src/probes/isUnsafeCallee.js b/src/probes/isUnsafeCallee.js index 6a6ea0a..62512b3 100644 --- a/src/probes/isUnsafeCallee.js +++ b/src/probes/isUnsafeCallee.js @@ -1,6 +1,5 @@ // Import Internal Dependencies import { isUnsafeCallee } from "../utils.js"; -import { ProbeSignals } from "../ProbeRunner.js"; /** * @description Detect unsafe statement @@ -16,13 +15,9 @@ function main(node, options) { const { analysis, data: calleeName } = options; analysis.addWarning("unsafe-stmt", calleeName, node.loc); - - return ProbeSignals.Skip; } export default { name: "isUnsafeCallee", - validateNode, - main, - breakOnMatch: false + validateNode, main, breakOnMatch: false }; 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); -}); From 9b1b7d5b1847e66de58998afbb6f9ef5dace55ec Mon Sep 17 00:00:00 2001 From: tchapacan Date: Mon, 1 Jan 2024 01:55:27 +0100 Subject: [PATCH 6/9] validateNode as array in isRequire probe & update test utils --- src/probes/isRequire.js | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/src/probes/isRequire.js b/src/probes/isRequire.js index 70d406a..ff1ea87 100644 --- a/src/probes/isRequire.js +++ b/src/probes/isRequire.js @@ -11,35 +11,34 @@ import { getCallExpressionArguments } from "@nodesecure/estree-ast-utils"; -function validateNode(node, option) { +function validateNodeRequire(node, { tracer }) { const id = getCallExpressionIdentifier(node); - const argument = getCallExpressionArguments(node); - if (id === null) { return [false]; } - const data = option.tracer.getDataFromIdentifier(id); + const data = 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 + ]; +} + +function validateNodeEvalRequire(node, options) { + const id = getCallExpressionIdentifier(node); + const argument = getCallExpressionArguments(node); + if (id === null) { + return [false]; } return [ - false, - null + id === "eval" && argument[0] === "require", + options?.identifiersName[0]?.name ?? void 0 ]; } + function main(node, options) { const { analysis, data: calleeName } = options; const { tracer } = analysis; @@ -70,14 +69,14 @@ function main(node, options) { } break; - // require("http") + // require("http") case "Literal": 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("") @@ -184,5 +183,5 @@ function walkRequireCallExpression(nodeToWalk, tracer) { export default { name: "isRequire", - validateNode, main, breakOnMatch: true, breakGroup: "import" + validateNode: [validateNodeRequire, validateNodeEvalRequire], main, breakOnMatch: true, breakGroup: "import" }; From 777f7afad0e75715b958337ec7828aedd3088888 Mon Sep 17 00:00:00 2001 From: tchapacan Date: Fri, 5 Jan 2024 23:06:02 +0100 Subject: [PATCH 7/9] rebase refacto probe test --- test/issues/179-UnsafeEvalRequire.spec.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/issues/179-UnsafeEvalRequire.spec.js b/test/issues/179-UnsafeEvalRequire.spec.js index 8e089c5..f3dad5f 100644 --- a/test/issues/179-UnsafeEvalRequire.spec.js +++ b/test/issues/179-UnsafeEvalRequire.spec.js @@ -16,10 +16,10 @@ const kWarningUnsafeStatement = "unsafe-stmt"; test("should detect unsafe-import and unsafe-statement", () => { const sastAnalysis = runASTAnalysis(kIncriminedCodeSample); - assert.equal(sastAnalysis.warnings.at(0).value, "stream"); - assert.equal(sastAnalysis.warnings.at(0).kind, kWarningUnsafeImport); - assert.equal(sastAnalysis.warnings.at(1).value, "eval"); - assert.equal(sastAnalysis.warnings.at(1).kind, kWarningUnsafeStatement); + assert.equal(sastAnalysis.warnings.at(0).value, "eval"); + assert.equal(sastAnalysis.warnings.at(0).kind, kWarningUnsafeStatement); + assert.equal(sastAnalysis.warnings.at(1).value, "stream"); + assert.equal(sastAnalysis.warnings.at(1).kind, kWarningUnsafeImport); assert.equal(sastAnalysis.warnings.length, 2); assert.equal(sastAnalysis.dependencies.has("stream"), true); assert.equal(sastAnalysis.dependencies.get("stream").unsafe, true); From 9a8165a8c0db6e1814873f221f6ca4c8dbd151c1 Mon Sep 17 00:00:00 2001 From: tchapacan Date: Sun, 7 Jan 2024 01:40:08 +0100 Subject: [PATCH 8/9] update spec, update addDep, clean isRequire & isUnsafeCallee probes --- src/ProbeRunner.js | 2 +- src/SourceFile.js | 6 +++- src/probes/isRequire.js | 40 ++++++++++++++--------- src/probes/isUnsafeCallee.js | 7 +++- src/utils.js | 30 ++++++++++++----- test/issues/179-UnsafeEvalRequire.spec.js | 8 ++--- 6 files changed, 62 insertions(+), 31 deletions(-) diff --git a/src/ProbeRunner.js b/src/ProbeRunner.js index dc314cf..2d2f0b6 100644 --- a/src/ProbeRunner.js +++ b/src/ProbeRunner.js @@ -42,12 +42,12 @@ export class ProbeRunner { * @type {Probe[]} */ static Defaults = [ + isRequire, isUnsafeCallee, isLiteral, isLiteralRegex, isRegexObject, isVariableDeclaration, - isRequire, isImportDeclaration, isMemberExpression, isAssignmentExpression, diff --git a/src/SourceFile.js b/src/SourceFile.js index 853f811..0e3013b 100644 --- a/src/SourceFile.js +++ b/src/SourceFile.js @@ -52,7 +52,7 @@ export class SourceFile { } } - addDependency(name, location = null, unsafe = false) { + addDependency(name, location = null, unsafe = this.dependencyAutoWarning) { if (typeof name !== "string" || name.trim() === "") { return; } @@ -64,6 +64,10 @@ export class SourceFile { inTry: this.inTryStatement, ...(location === null ? {} : { location }) }); + + if (this.dependencyAutoWarning) { + this.addWarning("unsafe-import", dependencyName, location); + } } addWarning(name, value, location = rootLocation()) { diff --git a/src/probes/isRequire.js b/src/probes/isRequire.js index ff1ea87..a91e93e 100644 --- a/src/probes/isRequire.js +++ b/src/probes/isRequire.js @@ -10,6 +10,7 @@ import { getCallExpressionIdentifier, getCallExpressionArguments } from "@nodesecure/estree-ast-utils"; +import { ProbeSignals } from "../ProbeRunner.js"; function validateNodeRequire(node, { tracer }) { const id = getCallExpressionIdentifier(node); @@ -21,23 +22,32 @@ function validateNodeRequire(node, { tracer }) { return [ data !== null && data.name === "require", - data?.identifierOrMemberExpr ?? void 0 + id ?? void 0 ]; } -function validateNodeEvalRequire(node, options) { +function validateNodeEvalRequire(node) { const id = getCallExpressionIdentifier(node); - const argument = getCallExpressionArguments(node); - if (id === null) { + + if (id !== "eval") { return [false]; } + if (node.callee.type !== "CallExpression") { + return [false]; + } + + const args = getCallExpressionArguments(node.callee); return [ - id === "eval" && argument[0] === "require", - options?.identifiersName[0]?.name ?? void 0 + args.length > 0 && args.at(0) === "require", + id ]; } +function teardown({ analysis }) { + analysis.dependencyAutoWarning = false; +} + function main(node, options) { const { analysis, data: calleeName } = options; @@ -47,12 +57,9 @@ function main(node, options) { 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); + if (calleeName === "eval") { + analysis.dependencyAutoWarning = true; } switch (arg.type) { @@ -71,9 +78,7 @@ function main(node, options) { // require("http") case "Literal": - if (arg.value !== "require") { - analysis.addDependency(arg.value, node.loc); - } + analysis.addDependency(arg.value, node.loc); break; // require(["ht", "tp"]) @@ -119,7 +124,7 @@ function main(node, options) { analysis.addWarning("unsafe-import", null, node.loc); // We skip walking the tree to avoid anymore warnings... - return Symbol.for("skipWalk"); + return ProbeSignals.Skip; } default: @@ -183,5 +188,8 @@ function walkRequireCallExpression(nodeToWalk, tracer) { export default { name: "isRequire", - validateNode: [validateNodeRequire, validateNodeEvalRequire], main, breakOnMatch: true, breakGroup: "import" + validateNode: [validateNodeRequire, validateNodeEvalRequire], + main, + breakOnMatch: true, + breakGroup: "import" }; diff --git a/src/probes/isUnsafeCallee.js b/src/probes/isUnsafeCallee.js index 62512b3..6a6ea0a 100644 --- a/src/probes/isUnsafeCallee.js +++ b/src/probes/isUnsafeCallee.js @@ -1,5 +1,6 @@ // Import Internal Dependencies import { isUnsafeCallee } from "../utils.js"; +import { ProbeSignals } from "../ProbeRunner.js"; /** * @description Detect unsafe statement @@ -15,9 +16,13 @@ function main(node, options) { const { analysis, data: calleeName } = options; analysis.addWarning("unsafe-stmt", calleeName, node.loc); + + return ProbeSignals.Skip; } export default { name: "isUnsafeCallee", - validateNode, main, breakOnMatch: false + validateNode, + main, + breakOnMatch: false }; diff --git a/src/utils.js b/src/utils.js index 0f6919c..be3cc8a 100644 --- a/src/utils.js +++ b/src/utils.js @@ -7,16 +7,30 @@ export function notNullOrUndefined(value) { return value !== null && value !== void 0; } -export function isUnsafeCallee(node) { +function isEvalCallee(node) { + const identifier = getCallExpressionIdentifier(node, { + resolveCallExpression: false + }); + + return identifier === "eval"; +} + +function isFunctionCallee(node) { const identifier = getCallExpressionIdentifier(node); - // For Function we are looking for this: `Function("...")();` - // A double CallExpression - return [ - (identifier === "eval" && node.callee.type === "Identifier") || - (identifier === "Function" && node.callee.type === "CallExpression"), - identifier - ]; + return identifier === "Function" && node.callee.type === "CallExpression"; +} + +export function isUnsafeCallee(node) { + if (isEvalCallee(node)) { + return [true, "eval"]; + } + + if (isFunctionCallee(node)) { + return [true, "Function"]; + } + + return [false, null]; } export function rootLocation() { diff --git a/test/issues/179-UnsafeEvalRequire.spec.js b/test/issues/179-UnsafeEvalRequire.spec.js index f3dad5f..8e089c5 100644 --- a/test/issues/179-UnsafeEvalRequire.spec.js +++ b/test/issues/179-UnsafeEvalRequire.spec.js @@ -16,10 +16,10 @@ const kWarningUnsafeStatement = "unsafe-stmt"; test("should detect unsafe-import and unsafe-statement", () => { const sastAnalysis = runASTAnalysis(kIncriminedCodeSample); - assert.equal(sastAnalysis.warnings.at(0).value, "eval"); - assert.equal(sastAnalysis.warnings.at(0).kind, kWarningUnsafeStatement); - assert.equal(sastAnalysis.warnings.at(1).value, "stream"); - assert.equal(sastAnalysis.warnings.at(1).kind, kWarningUnsafeImport); + assert.equal(sastAnalysis.warnings.at(0).value, "stream"); + assert.equal(sastAnalysis.warnings.at(0).kind, kWarningUnsafeImport); + 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); From ec106f205e0969057c13efb73ec766e59e0ea956 Mon Sep 17 00:00:00 2001 From: tchapacan Date: Mon, 8 Jan 2024 20:07:57 +0100 Subject: [PATCH 9/9] fix init dependencyAutoWarning=false & update docs --- docs/unsafe-import.md | 1 + src/SourceFile.js | 1 + 2 files changed, 2 insertions(+) diff --git a/docs/unsafe-import.md b/docs/unsafe-import.md index 1afa365..ef6923d 100644 --- a/docs/unsafe-import.md +++ b/docs/unsafe-import.md @@ -17,6 +17,7 @@ We analyze and trace several ways to require in Node.js (with CJS): - require.main.require - require.mainModule.require - require.resolve +- `const XX = eval('require')('XX');` (dangerous import using eval) ## Example diff --git a/src/SourceFile.js b/src/SourceFile.js index 0e3013b..5f93cf7 100644 --- a/src/SourceFile.js +++ b/src/SourceFile.js @@ -21,6 +21,7 @@ export class SourceFile { inTryStatement = false; hasDictionaryString = false; hasPrefixedIdentifiers = false; + dependencyAutoWarning = false; varkinds = { var: 0, let: 0, const: 0 }; idtypes = { assignExpr: 0, property: 0, variableDeclarator: 0, functionDeclaration: 0 }; counter = {