diff --git a/lib/rules/code_safety.ex b/lib/rules/code_safety.ex index bdc66b9e..6ac1888b 100644 --- a/lib/rules/code_safety.ex +++ b/lib/rules/code_safety.ex @@ -608,7 +608,31 @@ defmodule Hypatia.Rules.CodeSafety do %{ id: :js_exec_sync, severity: :high, - pattern: ~r/execSync\s*\(|child_process/, + # Only calls a shell parses: + # * execSync(, and exec( under a name the file imports from + # node:child_process ({exec}, {exec as run}, {exec: run}) -- + # a local function named exec is not a shell; + # * .exec( / .execSync( where is the name the file binds + # node:child_process to (import * as ns / import ns / require / + # await import), and + # require('child_process').exec( -- any argument shape; + # * spawn/spawnSync/execFile/execFileSync (or a name the file imports + # them as) called with a `shell` option (bare or quoted key) set to + # true or to a shell path. fork() ignores `shell`, so it is not here. + # Importing node:child_process is not a finding, nor is a shell-free + # call with an argv array, RegExp.exec, or a `shell` key outside a + # process call. The calls are matched per file by + # pattern_for/2 (`bound_calls`), so every call counts, on its own line. + # Not covered: a shell option passed through a variable, tainted argv to + # a shell-free call, and call arguments nested more than two + # parentheses deep -- a pattern cannot trace values (mathesis#90). + # Known false positive: a `shell` key nested inside another option of a + # process call (`spawn(c, a, {env: {shell: 'x'}})`) is reported; telling + # object depth apart needs a parser, and a rare false alarm is the safer + # error for a high-severity rule. + pattern: + ~r/\bexecSync\s*\(|require\(\s*['"](?:node:)?child_process['"]\s*\)\s*\.exec(?:Sync)?\s*\(|\b(?:spawn|spawnSync|execFile|execFileSync)\s*\((?:[^;()]|\((?:[^()]|\([^()]*\))*\))*?['"]?\bshell['"]?\s*:\s*(?:true\b|['"`])/, + bound_calls: :child_process, cwe: "CWE-78", description: "Shell execution -- validate input before passing to shell" }, @@ -898,7 +922,7 @@ defmodule Hypatia.Rules.CodeSafety do # line where the match starts) is not a finding; nor is `unsafe {` # inside a string literal. Matches are excused after the scan, so a # block whose brace is on the next line is judged as one block. - matches = Regex.scan(rule.pattern, subject, return: :index) + matches = Regex.scan(pattern_for(rule, subject), subject, return: :index) matches = if rule.id == :unsafe_block, @@ -1624,10 +1648,85 @@ defmodule Hypatia.Rules.CodeSafety do """ def stub_crypto_patterns, do: @stub_crypto_patterns + # A file's bindings of node:child_process. A regex cannot carry a binding + # to every later call (a match consumes it), so the rule's pattern is + # widened per file: + # * namespaces (`import * as cp from`, `import cp from`, + # `const cp = require(...)`) add `cp.exec(` and `cp.execSync(`; + # * destructured exec or execSync (`import {exec, execSync as run} + # from`, `const {exec: run} = require(...)`) adds `exec(` / `run(`; + # * a renamed spawn/spawnSync/execFile/execFileSync (`{spawn as run}`) + # adds `run(` with a `shell` option, like the unrenamed names. + # `require(...)` may also be `import(...)` or `await import(...)`. + # Regex sources, compiled at call time: a compiled regex in a module + # attribute does not survive OTP 28. + @cp_source ~S|['"](?:node:)?child_process['"]| + @cp_load ~S/(?:require|(?:await\s+)?import)\(\s*/ + @cp_namespace ~S|(?:import\s+(?:\*\s+as\s+)?([A-Za-z_$][\w$]*)\s+from\s*| <> + @cp_source <> + ~S/|(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*/ <> + @cp_load <> @cp_source <> ~S|\s*\))| + @cp_destructured ~S|(?:import\s*\{([^}]*)\}\s*from\s*| <> + @cp_source <> + ~S/|(?:const|let|var)\s*\{([^}]*)\}\s*=\s*/ <> + @cp_load <> @cp_source <> ~S|\s*\))| + # The argument span of a call up to a `shell` option set to true or to a + # shell path; arguments may nest parentheses two deep. + @shell_option_tail ~S/\s*\((?:[^;()]|\((?:[^()]|\([^()]*\))*\))*?['"]?\bshell['"]?\s*:\s*(?:true\b|['"`])/ + + @doc false + def pattern_for(%{bound_calls: :child_process} = rule, content) do + namespaces = captures(Regex.compile!(@cp_namespace), content) + + bindings = + @cp_destructured + |> Regex.compile!() + |> captures(content) + |> Enum.flat_map(&String.split(&1, ",")) + |> Enum.flat_map(fn entry -> + case Regex.run( + ~r/^\s*(exec|execSync|spawn|spawnSync|execFile|execFileSync)\s*(?:(?:as|:)\s*([A-Za-z_$][\w$]*))?\s*$/, + entry + ) do + [_, name, local] -> [{name, local}] + [_, name] -> [{name, name}] + nil -> [] + end + end) + |> Enum.uniq() + + # exec/execSync run a shell whatever the arguments; the spawn family + # does only with a `shell` option (unrenamed names are in the pattern). + execs = for {name, local} <- bindings, name in ["exec", "execSync"], do: local + + spawns = + for {name, local} <- bindings, name not in ["exec", "execSync"], local != name, do: local + + extra = + Enum.map(namespaces, &("(? Regex.escape(&1) <> "\\.exec(?:Sync)?\\s*\\(")) ++ + Enum.map(execs, &("(? Regex.escape(&1) <> "\\s*\\(")) ++ + Enum.map(spawns, &("(? Regex.escape(&1) <> @shell_option_tail)) + + case extra do + [] -> rule.pattern + extra -> Regex.compile!(Enum.join([Regex.source(rule.pattern) | extra], "|")) + end + end + + def pattern_for(rule, _content), do: rule.pattern + + defp captures(regex, content) do + regex + |> Regex.scan(content, capture: :all_but_first) + |> List.flatten() + |> Enum.reject(&(&1 == "")) + |> Enum.uniq() + end + @doc "Scan JavaScript/TypeScript content for web security issues" def scan_web_security(content) do Enum.flat_map(@javascript_patterns, fn rule -> - case Regex.scan(rule.pattern, content) do + case Regex.scan(pattern_for(rule, content), content) do [] -> [] diff --git a/test/code_safety_test.exs b/test/code_safety_test.exs index f51f0d6d..f45d5426 100644 --- a/test/code_safety_test.exs +++ b/test/code_safety_test.exs @@ -867,4 +867,114 @@ defmodule Hypatia.Rules.CodeSafetyTest do end end end + + describe "js_exec_sync flags only calls a shell parses (mathesis#90)" do + defp exec_sync?(code), + do: Enum.any?(CodeSafety.scan_content(code, "javascript"), &(&1.rule == :js_exec_sync)) + + test "importing node:child_process is not a finding" do + refute exec_sync?("import {spawnSync} from 'node:child_process';\n") + refute exec_sync?("const cp = require('child_process');\n") + end + + test "spawnSync and execFileSync with an argv array and no shell are not findings" do + code = """ + import {spawnSync, execFileSync} from 'node:child_process'; + const git = findTool('git'); + const r = spawnSync(git, ['ls-remote', `https://github.com/${repo}`], {encoding: 'utf8'}); + execFileSync('/usr/bin/sha256sum', [path], {encoding: 'utf8'}); + """ + + refute exec_sync?(code) + end + + test "RegExp.exec and a function named exec are not findings" do + refute exec_sync?("const m = /^v(\\d+)/.exec(tag);\nconst n = re.exec(line);\n") + refute exec_sync?("const t = re.exec(`${prefix}-${name}`);\n") + refute exec_sync?("function exec(cmd) { return cmd; }\nexec('safe');\n") + refute exec_sync?("const exec = (c) => c;\nexec(cmd);\n") + end + + test "a shell key outside a process call is not a finding" do + refute exec_sync?("const preferences = {shell: true};\n") + refute exec_sync?("spawnSync(git, ['status'])\nconst prefs = {shell: true}\n") + end + + test "execSync and exec run a shell" do + assert exec_sync?("const out = execSync(`git log ${ref}`);\n") + assert exec_sync?("import {exec} from 'node:child_process';\nexec(cmd, cb);\n") + assert exec_sync?("import {spawn, exec as run} from 'child_process';\nrun(cmd);\n") + assert exec_sync?("const { exec: sh } = require('node:child_process');\nsh(cmd);\n") + assert exec_sync?("import {execSync as run} from 'node:child_process';\nrun(cmd);\n") + assert exec_sync?("const {execSync: run} = require('child_process');\nrun(cmd);\n") + refute exec_sync?("import {spawnSync} from 'node:child_process';\nexec(cmd);\n") + assert exec_sync?("require('child_process').exec(`ls ${dir}`, cb);\n") + end + + test "exec through the name child_process is bound to, any argument shape" do + assert exec_sync?("import * as cp from 'node:child_process';\ncp.exec('rm -rf ' + dir);\n") + assert exec_sync?("import cp from 'child_process';\ncp.execSync(cmd);\n") + assert exec_sync?("const proc = require('child_process');\nproc.exec(cmd, cb);\n") + refute exec_sync?("import * as cp from 'node:child_process';\nother.exec(cmd);\n") + end + + test "the finding is on the call's line, not the import's" do + code = "import * as cp from 'node:child_process';\n\n\ncp.exec(cmd);\n" + [f] = Enum.filter(CodeSafety.scan_content(code, "javascript"), &(&1.rule == :js_exec_sync)) + assert f.lines == [4] + end + + test "every call through a bound name counts, each on its own line" do + code = "const cp = require('child_process');\ncp.exec('first');\ncp.exec('second');\n" + [f] = Enum.filter(CodeSafety.scan_content(code, "javascript"), &(&1.rule == :js_exec_sync)) + assert f.occurrences == 2 + assert f.lines == [2, 3] + + [w] = Enum.filter(CodeSafety.scan_web_security(code), &(&1.rule == :js_exec_sync)) + assert w.occurrences == 2 + end + + test "two bindings in one file are both followed" do + code = + "import * as cp from 'node:child_process';\nconst proc = require('child_process');\n" <> + "proc.execSync(a);\ncp.exec(b);\n" + + [f] = Enum.filter(CodeSafety.scan_content(code, "javascript"), &(&1.rule == :js_exec_sync)) + assert f.lines == [3, 4] + end + + test "a shell option makes spawn and execFile shell calls" do + assert exec_sync?("spawnSync(cmd, args, {shell: true});\n") + assert exec_sync?("spawn(cmd, { shell : '/bin/bash' });\n") + assert exec_sync?("execFileSync(cmd, args, {encoding: 'utf8', shell: true});\n") + assert exec_sync?("spawnSync(cmd, args, {'shell': true});\n") + assert exec_sync?("spawnSync(cmd, args, {\"shell\": true});\n") + assert exec_sync?("spawn(path.join(dir, 'x'), [a], {\n shell: true\n});\n") + refute exec_sync?("spawnSync(cmd, args, {shell: false});\n") + assert exec_sync?("spawn(resolve(path.join(dir, 'x')), args, {shell: true});\n") + end + + test "fork ignores the shell option" do + refute exec_sync?( + "import {fork} from 'node:child_process';\nfork(mod, args, {shell: true});\n" + ) + end + + test "a renamed spawn or execFile with a shell option is a shell call" do + assert exec_sync?( + "import {spawn as run} from 'node:child_process';\nrun(cmd, a, {shell: true});\n" + ) + + assert exec_sync?( + "const {execFileSync: x} = require('child_process');\nx(cmd, a, {shell: true});\n" + ) + + refute exec_sync?("import {spawn as run} from 'node:child_process';\nrun(cmd, a);\n") + end + + test "a dynamic import binds child_process too" do + assert exec_sync?("const cp = await import('node:child_process');\ncp.exec(command);\n") + assert exec_sync?("const {exec: sh} = await import('child_process');\nsh(command);\n") + end + end end