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/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/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..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 = { @@ -52,7 +53,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 +65,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 138fadb..a91e93e 100644 --- a/src/probes/isRequire.js +++ b/src/probes/isRequire.js @@ -10,11 +10,9 @@ import { getCallExpressionIdentifier, getCallExpressionArguments } from "@nodesecure/estree-ast-utils"; - -// Import Internal Dependencies import { ProbeSignals } from "../ProbeRunner.js"; -function validateNode(node, { tracer }) { +function validateNodeRequire(node, { tracer }) { const id = getCallExpressionIdentifier(node); if (id === null) { return [false]; @@ -24,12 +22,35 @@ function validateNode(node, { tracer }) { return [ data !== null && data.name === "require", - data?.identifierOrMemberExpr ?? void 0 + id ?? void 0 + ]; +} + +function validateNodeEvalRequire(node) { + const id = getCallExpressionIdentifier(node); + + if (id !== "eval") { + return [false]; + } + if (node.callee.type !== "CallExpression") { + return [false]; + } + + const args = getCallExpressionArguments(node.callee); + + return [ + args.length > 0 && args.at(0) === "require", + id ]; } +function teardown({ analysis }) { + analysis.dependencyAutoWarning = false; +} + + function main(node, options) { - const { analysis } = options; + const { analysis, data: calleeName } = options; const { tracer } = analysis; if (node.arguments.length === 0) { @@ -37,6 +58,10 @@ function main(node, options) { } const arg = node.arguments.at(0); + if (calleeName === "eval") { + analysis.dependencyAutoWarning = true; + } + switch (arg.type) { // const foo = "http"; require(foo); case "Identifier": @@ -163,7 +188,7 @@ function walkRequireCallExpression(nodeToWalk, tracer) { export default { name: "isRequire", - validateNode, + validateNode: [validateNodeRequire, validateNodeEvalRequire], main, breakOnMatch: true, breakGroup: "import" diff --git a/src/utils.js b/src/utils.js index 312bb15..be3cc8a 100644 --- a/src/utils.js +++ b/src/utils.js @@ -7,15 +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" || (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 new file mode 100644 index 0000000..8e089c5 --- /dev/null +++ b/test/issues/179-UnsafeEvalRequire.spec.js @@ -0,0 +1,27 @@ +// 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); + assert.equal(sastAnalysis.dependencies.has("stream"), true); + assert.equal(sastAnalysis.dependencies.get("stream").unsafe, true); + assert.equal(sastAnalysis.dependencies.size, 1); +});