From 9fcc1e351c62a93a9ec4e7165410d90ba55c7241 Mon Sep 17 00:00:00 2001 From: Thomas GENTILHOMME Date: Sat, 29 Oct 2022 17:24:54 +0200 Subject: [PATCH 1/2] refactor: implement new VariableTracer --- README.md | 2 - docs/unsafe-assign.md | 19 ----- package-lock.json | 14 ++-- package.json | 2 +- src/Analysis.js | 6 +- src/constants.js | 35 -------- src/obfuscators/trojan-source.js | 21 ++++- src/probes/index.js | 2 - src/probes/isAssignmentExprOrMemberExpr.js | 29 ------- src/probes/isLiteral.js | 5 +- src/probes/isLiteralRegex.js | 4 +- src/probes/isRegexObject.js | 4 +- src/probes/isRequire.js | 49 +++--------- src/probes/isVariableDeclaration.js | 79 +------------------ src/probes/isWeakCrypto.js | 34 ++++---- src/utils.js | 17 +--- src/warnings.js | 4 - .../weakCrypto/strongAlgorithms/sha256.js | 3 - test/probes/weakCrypto.spec.js | 7 +- test/searchRuntimeDependencies.spec.js | 29 ++----- types/warnings.d.ts | 1 - 21 files changed, 80 insertions(+), 286 deletions(-) delete mode 100644 docs/unsafe-assign.md delete mode 100644 src/constants.js delete mode 100644 src/probes/isAssignmentExprOrMemberExpr.js delete mode 100644 test/probes/fixtures/weakCrypto/strongAlgorithms/sha256.js diff --git a/README.md b/README.md index 8e0f0726..a20efa42 100644 --- a/README.md +++ b/README.md @@ -80,7 +80,6 @@ type WarningName = "parsing-error" | "encoded-literal" | "unsafe-regex" | "unsafe-stmt" -| "unsafe-assign" | "short-identifiers" | "suspicious-literal" | "obfuscated-code" @@ -115,7 +114,6 @@ This section describe all the possible warnings returned by JSXRay. Click on the | [unsafe-import](./docs/unsafe-import.md) | ❌ | Unable to follow an import (require, require.resolve) statement/expr. | | [unsafe-regex](./docs/unsafe-regex.md) | ❌ | A RegEx as been detected as unsafe and may be used for a ReDoS Attack. | | [unsafe-stmt](./docs//unsafe-stmt.md) | ❌ | Usage of dangerous statement like `eval()` or `Function("")`. | -| [unsafe-assign](./docs/unsafe-assign.md) | ❌ | Assignment of a protected global like `process` or `require`. | | [encoded-literal](./docs/encoded-literal.md) | ❌ | An encoded literal has been detected (it can be an hexa value, unicode sequence or a base64 string) | | [short-identifiers](./docs/short-identifiers.md) | ❌ | This mean that all identifiers has an average length below 1.5. | | [suspicious-literal](./docs/suspicious-literal.md) | ❌ | A suspicious literal has been found in the source code. | diff --git a/docs/unsafe-assign.md b/docs/unsafe-assign.md deleted file mode 100644 index be213ae9..00000000 --- a/docs/unsafe-assign.md +++ /dev/null @@ -1,19 +0,0 @@ -# Unsafe Assignment - -| Code | Severity | i18n | Experimental | -| --- | --- | --- | :-: | -| unsafe-assign | `Warning` | `sast_warnings.unsafe_assign` | ❌ | - -## Introduction - -The SAST scanner traces the assignment of several global variables considered to be dangerous. They can often be used for malicious purposes and hide information from tools like ours. - -On Node.js we track the use of `require` and `process` (and particulary things like `process.mainModule.require`). With the example below the analysis will still be able to trace the use of require: - -```js -const b = process; -const c = b.mainModule; -c.require("http"); -``` - -> **Note** We may remove this warning in future release (it generate to much noise for almost no additional value). diff --git a/package-lock.json b/package-lock.json index 37543b8b..a6ec0135 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,7 +9,7 @@ "version": "5.1.0", "license": "MIT", "dependencies": { - "@nodesecure/estree-ast-utils": "^1.1.0", + "@nodesecure/estree-ast-utils": "^1.2.1", "@nodesecure/sec-literal": "^1.1.0", "estree-walker": "^3.0.1", "is-minified-code": "^2.0.0", @@ -541,9 +541,9 @@ } }, "node_modules/@nodesecure/estree-ast-utils": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/@nodesecure/estree-ast-utils/-/estree-ast-utils-1.1.0.tgz", - "integrity": "sha512-V5a1aNv1AuziVKbdXiUYrIV9p8tWO5/srv6vvOs5QeKmTbOeeMB5R6QhinJtD6rUoHZR8LpXcZXKkMs/mqtqNg==", + "version": "1.2.1", + "resolved": "https://registry.npmjs.org/@nodesecure/estree-ast-utils/-/estree-ast-utils-1.2.1.tgz", + "integrity": "sha512-Lb+fkNj3imlygZOtXDRrgQ7kjgcklghF95ri2jYuwgrw51CZzdqsPNuIm8BH5RBpY2WSGEJOLVd79b1K9IqzhQ==", "dependencies": { "@nodesecure/sec-literal": "^1.1.0" } @@ -4800,9 +4800,9 @@ } }, "@nodesecure/estree-ast-utils": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/@nodesecure/estree-ast-utils/-/estree-ast-utils-1.1.0.tgz", - "integrity": "sha512-V5a1aNv1AuziVKbdXiUYrIV9p8tWO5/srv6vvOs5QeKmTbOeeMB5R6QhinJtD6rUoHZR8LpXcZXKkMs/mqtqNg==", + "version": "1.2.1", + "resolved": "https://registry.npmjs.org/@nodesecure/estree-ast-utils/-/estree-ast-utils-1.2.1.tgz", + "integrity": "sha512-Lb+fkNj3imlygZOtXDRrgQ7kjgcklghF95ri2jYuwgrw51CZzdqsPNuIm8BH5RBpY2WSGEJOLVd79b1K9IqzhQ==", "requires": { "@nodesecure/sec-literal": "^1.1.0" } diff --git a/package.json b/package.json index 4251bf22..74989757 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ }, "homepage": "https://github.com/NodeSecure/js-x-ray#readme", "dependencies": { - "@nodesecure/estree-ast-utils": "^1.1.0", + "@nodesecure/estree-ast-utils": "^1.2.1", "@nodesecure/sec-literal": "^1.1.0", "estree-walker": "^3.0.1", "is-minified-code": "^2.0.0", diff --git a/src/Analysis.js b/src/Analysis.js index de5e71c9..029618ea 100644 --- a/src/Analysis.js +++ b/src/Analysis.js @@ -5,7 +5,6 @@ import { VariableTracer } from "@nodesecure/estree-ast-utils"; // Import Internal Dependencies import { rootLocation, toArrayLocation } from "./utils.js"; import { generateWarning } from "./warnings.js"; -import { processMainModuleRequire } from "./constants.js"; import ASTDeps from "./ASTDeps.js"; import { isObfuscatedCode, hasTrojanSource } from "./obfuscators/index.js"; import { runOnProbes } from "./probes/index.js"; @@ -37,12 +36,9 @@ export default class Analysis { this.tracer = new VariableTracer() .enableDefaultTracing() .trace("crypto.createHash", { followConsecutiveAssignment: true }); - this.dependencies = new ASTDeps(); - this.globalParts = new Map(); + this.dependencies = new ASTDeps(); this.handledEncodedLiteralValues = new Map(); - - this.requireIdentifiers = new Set(["require", processMainModuleRequire]); this.warnings = []; this.literalScores = []; } diff --git a/src/constants.js b/src/constants.js deleted file mode 100644 index 3c555074..00000000 --- a/src/constants.js +++ /dev/null @@ -1,35 +0,0 @@ -/** - * This is one of the way to get a valid require. - * - * @see https://nodejs.org/api/process.html#process_process_mainmodule - */ -export const processMainModuleRequire = "process.mainModule.require"; - -/** - * JavaScript dangerous global identifiers that can be used by hackers - */ -export const globalIdentifiers = new Set(["global", "globalThis", "root", "GLOBAL", "window"]); - -/** - * Dangerous Global identifiers parts - */ -export const globalParts = new Set([...globalIdentifiers, "process", "mainModule", "require"]); - -/** - * Dangerous Unicode control characters that can be used by hackers - * to perform trojan source. - */ -export const unsafeUnicodeControlCharacters = [ - "\u202A", - "\u202B", - "\u202D", - "\u202E", - "\u202C", - "\u2066", - "\u2067", - "\u2068", - "\u2069", - "\u200E", - "\u200F", - "\u061C" -]; diff --git a/src/obfuscators/trojan-source.js b/src/obfuscators/trojan-source.js index 620b8cf2..bdc1b180 100644 --- a/src/obfuscators/trojan-source.js +++ b/src/obfuscators/trojan-source.js @@ -1,7 +1,24 @@ -import { unsafeUnicodeControlCharacters } from "../constants.js"; +/** + * Dangerous Unicode control characters that can be used by hackers + * to perform trojan source. + */ +const kUnsafeUnicodeControlCharacters = [ + "\u202A", + "\u202B", + "\u202D", + "\u202E", + "\u202C", + "\u2066", + "\u2067", + "\u2068", + "\u2069", + "\u200E", + "\u200F", + "\u061C" +]; export function verify(sourceString) { - for (const unsafeCharacter of unsafeUnicodeControlCharacters) { + for (const unsafeCharacter of kUnsafeUnicodeControlCharacters) { if (sourceString.includes(unsafeCharacter)) { return true; } diff --git a/src/probes/index.js b/src/probes/index.js index acceaa4c..61c11676 100644 --- a/src/probes/index.js +++ b/src/probes/index.js @@ -4,7 +4,6 @@ import isLiteral from "./isLiteral.js"; import isLiteralRegex from "./isLiteralRegex.js"; import isRegexObject from "./isRegexObject.js"; import isVariableDeclaration from "./isVariableDeclaration.js"; -import isAssignmentExprOrMemberExpr from "./isAssignmentExprOrMemberExpr.js"; import isRequire from "./isRequire.js"; import isImportDeclaration from "./isImportDeclaration.js"; import isMemberExpression from "./isMemberExpression.js"; @@ -22,7 +21,6 @@ const kListOfProbes = [ isLiteralRegex, isRegexObject, isVariableDeclaration, - isAssignmentExprOrMemberExpr, isRequire, isImportDeclaration, isMemberExpression, diff --git a/src/probes/isAssignmentExprOrMemberExpr.js b/src/probes/isAssignmentExprOrMemberExpr.js deleted file mode 100644 index 3096fec6..00000000 --- a/src/probes/isAssignmentExprOrMemberExpr.js +++ /dev/null @@ -1,29 +0,0 @@ -// Import Third-party Dependencies -import { getMemberExpressionIdentifier } from "@nodesecure/estree-ast-utils"; - -// Search for unsafe assignment and member expression like 'require.cache' -function validateNode(node) { - return [ - node.type === "AssignmentExpression" && node.left.type === "MemberExpression" - ]; -} - -function main(node, options) { - const { analysis } = options; - const { tracer } = analysis; - - /** - * TODO (Note) - * - * No test throw for this, the code probably need to be removed with the new VariableTracer - */ - const assignName = [...getMemberExpressionIdentifier(node.left, { tracer })].join(""); - if (node.right.type === "Identifier" && analysis.requireIdentifiers.has(node.right.name)) { - analysis.requireIdentifiers.add(assignName); - } -} - -export default { - name: "isAssignmentExprOrMemberExpr", - validateNode, main, breakOnMatch: false -}; diff --git a/src/probes/isLiteral.js b/src/probes/isLiteral.js index af6c4bcf..9c48349a 100644 --- a/src/probes/isLiteral.js +++ b/src/probes/isLiteral.js @@ -4,9 +4,6 @@ import { builtinModules } from "repl"; // Import Third-party Dependencies import { Hex } from "@nodesecure/sec-literal"; -// Import Internal Dependencies -import { globalParts } from "../constants.js"; - // CONSTANTS const kNodeDeps = new Set(builtinModules); @@ -36,7 +33,7 @@ function main(node, options) { analysis.dependencies.add(value, node.loc); analysis.addWarning("unsafe-import", null, node.loc); } - else if (globalParts.has(value) || !Hex.isSafe(node.value)) { + else if (value === "require" || !Hex.isSafe(node.value)) { analysis.addWarning("encoded-literal", node.value, node.loc); } } diff --git a/src/probes/isLiteralRegex.js b/src/probes/isLiteralRegex.js index 1c405401..95406f20 100644 --- a/src/probes/isLiteralRegex.js +++ b/src/probes/isLiteralRegex.js @@ -1,7 +1,5 @@ -// Require Internal Dependencies -import { isLiteralRegex } from "../utils.js"; - // Require Third-party Dependencies +import { isLiteralRegex } from "@nodesecure/estree-ast-utils"; import safeRegex from "safe-regex"; /** diff --git a/src/probes/isRegexObject.js b/src/probes/isRegexObject.js index a55ef797..9169d23a 100644 --- a/src/probes/isRegexObject.js +++ b/src/probes/isRegexObject.js @@ -1,7 +1,5 @@ -// Import Internal Dependencies -import { isLiteralRegex } from "../utils.js"; - // Import Third-party Dependencies +import { isLiteralRegex } from "@nodesecure/estree-ast-utils"; import safeRegex from "safe-regex"; /** diff --git a/src/probes/isRequire.js b/src/probes/isRequire.js index 37b064c2..e9d927de 100644 --- a/src/probes/isRequire.js +++ b/src/probes/isRequire.js @@ -1,59 +1,34 @@ /* eslint-disable consistent-return */ -// Import Internal Dependencies -import { isRequireGlobalMemberExpr } from "../utils.js"; - // Import Third-party Dependencies import { Hex } from "@nodesecure/sec-literal"; import { walk } from "estree-walker"; import { concatBinaryExpression, arrayExpressionToString, - getMemberExpressionIdentifier + getMemberExpressionIdentifier, + getCallExpressionIdentifier } from "@nodesecure/estree-ast-utils"; -function validateNode(node, analysis) { - return [ - isRequireIdentifiers(node, analysis) || - isRequireResolve(node) || - isRequireMemberExpr(node) - ]; -} - -function isRequireResolve(node) { - if (node.type !== "CallExpression" || node.callee.type !== "MemberExpression") { - return false; - } - - return node.callee.object.name === "require" && node.callee.property.name === "resolve"; -} - -function isRequireMemberExpr(node) { - if (node.type !== "CallExpression" || node.callee.type !== "MemberExpression") { - return false; +function validateNode(node, { tracer }) { + const id = getCallExpressionIdentifier(node); + if (id === null) { + return [false]; } - return isRequireGlobalMemberExpr( - [...getMemberExpressionIdentifier(node.callee)].join(".") - ); -} + const data = tracer.getDataFromIdentifier(id); -function isRequireIdentifiers(node, analysis) { - if (node.type !== "CallExpression") { - return false; - } - const fullName = node.callee.type === "MemberExpression" ? - [...getMemberExpressionIdentifier(node.callee)].join(".") : - node.callee.name; - - return analysis.requireIdentifiers.has(fullName); + return [ + data !== null && data.name === "require", + data?.identifierOrMemberExpr ?? void 0 + ]; } function main(node, options) { const { analysis } = options; const { tracer } = analysis; - const arg = node.arguments[0]; + const arg = node.arguments.at(0); switch (arg.type) { // const foo = "http"; require(foo); case "Identifier": diff --git a/src/probes/isVariableDeclaration.js b/src/probes/isVariableDeclaration.js index 7646c79c..a9e00309 100644 --- a/src/probes/isVariableDeclaration.js +++ b/src/probes/isVariableDeclaration.js @@ -1,16 +1,8 @@ // Import Third-party Dependencies import { - getVariableDeclarationIdentifiers, - getMemberExpressionIdentifier + getVariableDeclarationIdentifiers } from "@nodesecure/estree-ast-utils"; -// Require Internal Dependencies -import { isUnsafeCallee, isRequireGlobalMemberExpr } from "../utils.js"; -import { globalParts, processMainModuleRequire } from "../constants.js"; - -// CONSTANTS -const kUnsafeCallee = new Set(["eval", "Function"]); - // In case we are matching a Variable declaration, we have to save the identifier // This allow the AST Analysis to retrieve required dependency when the stmt is mixed with variables. function validateNode(node) { @@ -21,7 +13,6 @@ function validateNode(node) { function main(mainNode, options) { const { analysis } = options; - const { tracer } = analysis; analysis.varkinds[mainNode.kind]++; @@ -30,77 +21,9 @@ function main(mainNode, options) { for (const { name } of getVariableDeclarationIdentifiers(node.id)) { analysis.identifiersName.push({ name, type: "variableDeclarator" }); } - - if (node.init === null || node.id.type !== "Identifier") { - continue; - } - - // Searching for someone who assign require to a variable, ex: - // const r = require - if (node.init.type === "Identifier") { - if (kUnsafeCallee.has(node.init.name)) { - analysis.addWarning("unsafe-assign", node.init.name, node.loc); - } - else if (analysis.requireIdentifiers.has(node.init.name)) { - analysis.requireIdentifiers.add(node.id.name); - analysis.addWarning("unsafe-assign", node.init.name, node.loc); - } - else if (globalParts.has(node.init.name)) { - analysis.globalParts.set(node.id.name, node.init.name); - getRequirablePatterns(analysis.globalParts) - .forEach((name) => analysis.requireIdentifiers.add(name)); - } - } - - // Same as before but for pattern like process.mainModule and require.resolve - else if (node.init.type === "MemberExpression") { - const members = [...getMemberExpressionIdentifier(node.init, { tracer })]; - const value = members.join(""); - - if (analysis.globalParts.has(members[0]) || members.every((part) => globalParts.has(part))) { - analysis.globalParts.set(node.id.name, members.slice(1).join(".")); - analysis.addWarning("unsafe-assign", value, node.loc); - } - getRequirablePatterns(analysis.globalParts) - .forEach((name) => analysis.requireIdentifiers.add(name)); - - if (isRequireStatement(value)) { - analysis.requireIdentifiers.add(node.id.name); - analysis.addWarning("unsafe-assign", value, node.loc); - } - } - else if (isUnsafeCallee(node.init)[0]) { - analysis.globalParts.set(node.id.name, "global"); - globalParts.add(node.id.name); - analysis.requireIdentifiers.add(`${node.id.name}.${processMainModuleRequire}`); - } } } -function isRequireStatement(value) { - return value.startsWith("require") || - value.startsWith(processMainModuleRequire) || - isRequireGlobalMemberExpr(value); -} - -function getRequirablePatterns(parts) { - const result = new Set(); - - for (const [id, path] of parts.entries()) { - if (path === "process") { - result.add(`${id}.mainModule.require`); - } - else if (path === "mainModule") { - result.add(`${id}.require`); - } - else if (path.includes("require")) { - result.add(id); - } - } - - return [...result]; -} - export default { name: "isVariableDeclaration", validateNode, main, breakOnMatch: false diff --git a/src/probes/isWeakCrypto.js b/src/probes/isWeakCrypto.js index 7abefd1b..0c4a73c6 100644 --- a/src/probes/isWeakCrypto.js +++ b/src/probes/isWeakCrypto.js @@ -1,26 +1,30 @@ +// Import Third-party Dependencies +import { getCallExpressionIdentifier } from "@nodesecure/estree-ast-utils"; + // CONSTANTS -const kWeakAlgorithms = new Set(["md5", "sha1", "ripemd160", "md4", "md2"]); +const kWeakAlgorithms = new Set([ + "md5", + "sha1", + "ripemd160", + "md4", + "md2" +]); + +function validateNode(node, { tracer }) { + const id = getCallExpressionIdentifier(node); + if (id === null) { + return [false]; + } -function validateNode(node) { - const isCallExpression = node.type === "CallExpression"; - const isSimpleIdentifier = isCallExpression && - node.callee.type === "Identifier" && - node.callee.name === "createHash"; - const isMemberExpression = isCallExpression && - node.callee.type === "MemberExpression" && - node.callee.property.name === "createHash"; + const data = tracer.getDataFromIdentifier(id); - return [isSimpleIdentifier || isMemberExpression]; + return [data !== null && data.identifierOrMemberExpr === "crypto.createHash"]; } function main(node, { analysis }) { const arg = node.arguments.at(0); - const isCryptoImported = analysis.dependencies.has("crypto"); - if ( - kWeakAlgorithms.has(arg.value) && - isCryptoImported - ) { + if (kWeakAlgorithms.has(arg.value)) { analysis.addWarning("weak-crypto", arg.value, node.loc); } } diff --git a/src/utils.js b/src/utils.js index d34c5e83..efc67e0e 100644 --- a/src/utils.js +++ b/src/utils.js @@ -3,18 +3,10 @@ import { getCallExpressionIdentifier } from "@nodesecure/estree-ast-utils"; -// Import Internal Dependencies -import { globalIdentifiers, processMainModuleRequire } from "./constants.js"; - export function notNullOrUndefined(value) { return value !== null && value !== void 0; } -export function isRequireGlobalMemberExpr(value) { - return [...globalIdentifiers] - .some((name) => value.startsWith(`${name}.${processMainModuleRequire}`)); -} - export function isUnsafeCallee(node) { const identifier = getCallExpressionIdentifier(node); @@ -26,10 +18,6 @@ export function isUnsafeCallee(node) { ]; } -export function isLiteralRegex(node) { - return node.type === "Literal" && "regex" in node; -} - export function rootLocation() { return { start: { line: 0, column: 0 }, end: { line: 0, column: 0 } }; } @@ -37,5 +25,8 @@ export function rootLocation() { export function toArrayLocation(location = rootLocation()) { const { start, end = start } = location; - return [[start.line || 0, start.column || 0], [end.line || 0, end.column || 0]]; + return [ + [start.line || 0, start.column || 0], + [end.line || 0, end.column || 0] + ]; } diff --git a/src/warnings.js b/src/warnings.js index df78664b..ac40259f 100644 --- a/src/warnings.js +++ b/src/warnings.js @@ -19,10 +19,6 @@ export const warnings = Object.freeze({ i18n: "sast_warnings.unsafe_stmt", severity: "Warning" }, - "unsafe-assign": { - i18n: "sast_warnings.unsafe_assign", - severity: "Warning" - }, "encoded-literal": { i18n: "sast_warnings.encoded_literal", severity: "Information" diff --git a/test/probes/fixtures/weakCrypto/strongAlgorithms/sha256.js b/test/probes/fixtures/weakCrypto/strongAlgorithms/sha256.js deleted file mode 100644 index 89aa584c..00000000 --- a/test/probes/fixtures/weakCrypto/strongAlgorithms/sha256.js +++ /dev/null @@ -1,3 +0,0 @@ -import crypto from 'crypto'; - -crypto.createHash('sha256'); diff --git a/test/probes/weakCrypto.spec.js b/test/probes/weakCrypto.spec.js index 5727fc57..24d8141a 100644 --- a/test/probes/weakCrypto.spec.js +++ b/test/probes/weakCrypto.spec.js @@ -46,8 +46,11 @@ test("it should report a warning in case of `[expression]createHash() }); test("it should NOT report a warning in case of `[expression]createHash('sha256')` usage", (tape) => { - const md5Usage = readFileSync(new URL("strongAlgorithms/sha256.js", FIXTURE_URL), "utf-8"); - const { warnings: outputWarnings } = runASTAnalysis(md5Usage); + const code = ` + import crypto from 'crypto'; + crypto.createHash('sha256'); + `; + const { warnings: outputWarnings } = runASTAnalysis(code); tape.strictEqual(outputWarnings.length, 0); tape.end(); diff --git a/test/searchRuntimeDependencies.spec.js b/test/searchRuntimeDependencies.spec.js index 683e0452..f4c46ff9 100644 --- a/test/searchRuntimeDependencies.spec.js +++ b/test/searchRuntimeDependencies.spec.js @@ -122,7 +122,7 @@ test("should return an unsafe-assign warning when a protected global is assigned r("http"); `); - tape.deepEqual(getWarningKind(warnings), ["unsafe-assign"].sort()); + tape.deepEqual(getWarningKind(warnings), []); tape.deepEqual([...dependencies], ["http"]); tape.end(); }); @@ -134,7 +134,7 @@ test("should succesfully follow the require stmt when assigned multiple times an b("http"); `); - tape.deepEqual(getWarningKind(warnings), ["unsafe-assign", "unsafe-assign"].sort()); + tape.deepEqual(getWarningKind(warnings), []); tape.deepEqual([...dependencies], ["http"]); tape.end(); }); @@ -169,27 +169,16 @@ test("should detect unsafe Function statments", (tape) => { tape.end(); }); -test("should detect unsafe-assign of eval", (tape) => { - const { warnings } = runASTAnalysis(` - const e = eval; - `); - - tape.deepEqual(getWarningKind(warnings), ["unsafe-assign"].sort()); - tape.end(); -}); - test("should be capable of following global parts", (tape) => { const { warnings, dependencies } = runASTAnalysis(` - const g = global.process; - const r = g.mainModule; - const c = r.require; - c("http"); - r.require("fs"); + const g = global.process; + const r = g.mainModule; + const c = r.require; + c("http"); + r.require("fs"); `); - tape.deepEqual(getWarningKind(warnings), [ - "unsafe-assign", "unsafe-assign", "unsafe-assign" - ].sort()); + tape.deepEqual(getWarningKind(warnings), []); tape.deepEqual([...dependencies], ["http", "fs"]); tape.end(); }); @@ -226,8 +215,6 @@ test("should be capable to follow hexa computation members expr", (tape) => { tape.deepEqual(getWarningKind(warnings), [ "encoded-literal", - "unsafe-assign", - "unsafe-assign", "unsafe-import", "unsafe-stmt" ].sort()); diff --git a/types/warnings.d.ts b/types/warnings.d.ts index 5fe70948..26aa722c 100644 --- a/types/warnings.d.ts +++ b/types/warnings.d.ts @@ -11,7 +11,6 @@ type WarningNameWithValue = "parsing-error" | "encoded-literal" | "unsafe-regex" | "unsafe-stmt" -| "unsafe-assign" | "short-identifiers" | "suspicious-literal" | "obfuscated-code" From 2b504eaf7dadff4b5736a448f0d0f717114956f6 Mon Sep 17 00:00:00 2001 From: fraxken Date: Sun, 1 Jan 2023 06:46:01 +0100 Subject: [PATCH 2/2] refactor: remove warnings if crypto module is not imported --- package-lock.json | 14 +++++++------- package.json | 2 +- src/Analysis.js | 4 +++- src/probes/isWeakCrypto.js | 2 +- test/probes/weakCrypto.spec.js | 13 +++++++++++++ 5 files changed, 25 insertions(+), 10 deletions(-) diff --git a/package-lock.json b/package-lock.json index a6ec0135..83f50a6f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,7 +9,7 @@ "version": "5.1.0", "license": "MIT", "dependencies": { - "@nodesecure/estree-ast-utils": "^1.2.1", + "@nodesecure/estree-ast-utils": "^1.3.0", "@nodesecure/sec-literal": "^1.1.0", "estree-walker": "^3.0.1", "is-minified-code": "^2.0.0", @@ -541,9 +541,9 @@ } }, "node_modules/@nodesecure/estree-ast-utils": { - "version": "1.2.1", - "resolved": "https://registry.npmjs.org/@nodesecure/estree-ast-utils/-/estree-ast-utils-1.2.1.tgz", - "integrity": "sha512-Lb+fkNj3imlygZOtXDRrgQ7kjgcklghF95ri2jYuwgrw51CZzdqsPNuIm8BH5RBpY2WSGEJOLVd79b1K9IqzhQ==", + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@nodesecure/estree-ast-utils/-/estree-ast-utils-1.3.0.tgz", + "integrity": "sha512-o7J+OM8+XHm61WdvF/K8mynPRS3SWUgNmynQm0MVnS3Pbz7yGbj+cKDvpg5EZ2zDpZhVDa5Y0u1+MS77eQl8BA==", "dependencies": { "@nodesecure/sec-literal": "^1.1.0" } @@ -4800,9 +4800,9 @@ } }, "@nodesecure/estree-ast-utils": { - "version": "1.2.1", - "resolved": "https://registry.npmjs.org/@nodesecure/estree-ast-utils/-/estree-ast-utils-1.2.1.tgz", - "integrity": "sha512-Lb+fkNj3imlygZOtXDRrgQ7kjgcklghF95ri2jYuwgrw51CZzdqsPNuIm8BH5RBpY2WSGEJOLVd79b1K9IqzhQ==", + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@nodesecure/estree-ast-utils/-/estree-ast-utils-1.3.0.tgz", + "integrity": "sha512-o7J+OM8+XHm61WdvF/K8mynPRS3SWUgNmynQm0MVnS3Pbz7yGbj+cKDvpg5EZ2zDpZhVDa5Y0u1+MS77eQl8BA==", "requires": { "@nodesecure/sec-literal": "^1.1.0" } diff --git a/package.json b/package.json index 74989757..51765a01 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ }, "homepage": "https://github.com/NodeSecure/js-x-ray#readme", "dependencies": { - "@nodesecure/estree-ast-utils": "^1.2.1", + "@nodesecure/estree-ast-utils": "^1.3.0", "@nodesecure/sec-literal": "^1.1.0", "estree-walker": "^3.0.1", "is-minified-code": "^2.0.0", diff --git a/src/Analysis.js b/src/Analysis.js index 029618ea..e85a4ef6 100644 --- a/src/Analysis.js +++ b/src/Analysis.js @@ -35,7 +35,9 @@ export default class Analysis { constructor() { this.tracer = new VariableTracer() .enableDefaultTracing() - .trace("crypto.createHash", { followConsecutiveAssignment: true }); + .trace("crypto.createHash", { + followConsecutiveAssignment: true, moduleName: "crypto" + }); this.dependencies = new ASTDeps(); this.handledEncodedLiteralValues = new Map(); diff --git a/src/probes/isWeakCrypto.js b/src/probes/isWeakCrypto.js index 0c4a73c6..1bbdbd2b 100644 --- a/src/probes/isWeakCrypto.js +++ b/src/probes/isWeakCrypto.js @@ -12,7 +12,7 @@ const kWeakAlgorithms = new Set([ function validateNode(node, { tracer }) { const id = getCallExpressionIdentifier(node); - if (id === null) { + if (id === null || !tracer.importedModules.has("crypto")) { return [false]; } diff --git a/test/probes/weakCrypto.spec.js b/test/probes/weakCrypto.spec.js index 24d8141a..f326af1f 100644 --- a/test/probes/weakCrypto.spec.js +++ b/test/probes/weakCrypto.spec.js @@ -55,3 +55,16 @@ test("it should NOT report a warning in case of `[expression]createHash('sha256' tape.strictEqual(outputWarnings.length, 0); tape.end(); }); + +test("it should NOT report a warning if crypto.createHash is not imported", (tape) => { + const code = ` + const crypto = { + createHash() {} + } + crypto.createHash('md5'); + `; + const { warnings: outputWarnings } = runASTAnalysis(code); + + tape.strictEqual(outputWarnings.length, 0); + tape.end(); +});