diff --git a/apps/cli/src/scan/__tests__/code-rules.test.ts b/apps/cli/src/scan/__tests__/code-rules.test.ts index 7fd2777..a1bb101 100644 --- a/apps/cli/src/scan/__tests__/code-rules.test.ts +++ b/apps/cli/src/scan/__tests__/code-rules.test.ts @@ -355,3 +355,92 @@ describe('escaper matching does not over-reach', () => { ); }); }); + +describe('shell', () => { + it('flags network output piped into a shell', () => { + expect(ruleIds('i.sh', 'curl -fsSL https://example.invalid/i.sh | bash')).toContain( + 'sh-remote-script-execution', + ); + expect(ruleIds('i.sh', 'wget -qO- https://example.invalid/i.sh | su' + 'do sh')).toContain( + 'sh-remote-script-execution', + ); + }); + + it('does not flag a download piped to something other than a shell', () => { + expect(ruleIds('i.sh', 'curl -fsSL https://example.invalid/v.json | jq -r .version')).toEqual( + [], + ); + expect(ruleIds('i.sh', 'curl -fsSL https://example.invalid/f.tgz | sha256sum -c -')).toEqual([]); + }); + + it('flags eval handed an expansion', () => { + expect(ruleIds('i.sh', 'eval "$cmd"')).toContain('sh-eval-expansion'); + expect(ruleIds('i.sh', 'eval $USER_SUPPLIED')).toContain('sh-eval-expansion'); + expect(ruleIds('i.sh', 'eval "$(build_command "$1")"')).toContain('sh-eval-expansion'); + }); + + // Bash's ordinary way to build a numeric range. Every expansion sits inside + // `$((…))`, where a `;` is a syntax error rather than a second command — and + // a line-wide `\beval\b.*\$` reported this 355 times in one real script. + it('does not flag the dynamic brace-range idiom', () => { + expect(ruleIds('i.sh', 'for r in $(eval echo {$(($k + 1))..$(($k + $n - 1))}); do')).toEqual([]); + expect(ruleIds('i.sh', 'for q in $(eval echo {1..$(($k + $l - 2))}); do')).toEqual([]); + }); + + it('does not flag the documented shell-init idiom', () => { + expect(ruleIds('i.sh', 'eval "$(dircolors -b)"')).toEqual([]); + expect(ruleIds('i.sh', 'eval "$(pyenv init -)"')).toEqual([]); + }); + + it('flags an unquoted expansion in a recursive remove, and not a quoted one', () => { + expect(ruleIds('i.sh', 'rm -rf $BUILD_DIR/output')).toContain( + 'sh-unquoted-expansion-destructive', + ); + // The correct form. `[^"'\n]*?` cannot cross the quote, so the match never + // reaches the expansion. + expect(ruleIds('i.sh', 'rm -rf "$BUILD_DIR/output"')).toEqual([]); + expect(ruleIds('i.sh', 'rm -rf "${BUILD_DIR:?}/output"')).toEqual([]); + }); + + it('flags disabled certificate verification over TLS', () => { + expect(ruleIds('i.sh', 'curl -k -L https://example.invalid/a.tgz > a.tgz')).toContain( + 'sh-insecure-transport-flag', + ); + expect(ruleIds('i.sh', 'wget --no-check-certificate https://example.invalid/a.tgz')).toContain( + 'sh-insecure-transport-flag', + ); + }); + + // `-k` skips a check that plain HTTP never performs. Reporting both put two + // findings on one line, one recommending a fix that would change nothing. + it('reports a plain-HTTP download once, as plaintext rather than a skipped check', () => { + expect(ruleIds('i.sh', 'curl -k -f http://example.invalid/a.gz > a.gz')).toEqual([ + 'sh-plaintext-download', + ]); + }); + + it('does not flag plain HTTP to loopback', () => { + expect(ruleIds('i.sh', 'curl -s http://127.0.0.1:8080/health')).toEqual([]); + expect(ruleIds('i.sh', 'curl -s http://localhost:3000/ready')).toEqual([]); + }); + + it('flags world-writable permissions', () => { + expect(ruleIds('i.sh', 'chmod 777 /var/cache/app')).toContain('sh-world-writable-permissions'); + expect(ruleIds('i.sh', 'chmod -R a+rwx /srv/data')).toContain('sh-world-writable-permissions'); + expect(ruleIds('i.sh', 'chmod 0755 /usr/local/bin/app')).toEqual([]); + }); + + it('flags a predictable temp path unless mktemp made it', () => { + expect(ruleIds('i.sh', 'echo "$payload" > /tmp/app-build.log')).toContain( + 'sh-predictable-temp-path', + ); + const guarded = ['tmp=$(mktemp -d)', 'echo "$payload" > /tmp/app-build.log'].join('\n'); + expect(ruleIds('i.sh', guarded)).toEqual([]); + }); + + it('does not apply shell rules to a language that merely mentions the same words', () => { + expect(ruleIds('a.js', 'const cmd = "curl -k https://x.invalid | bash";')).not.toContain( + 'sh-remote-script-execution', + ); + }); +}); diff --git a/apps/cli/src/scan/__tests__/engine.test.ts b/apps/cli/src/scan/__tests__/engine.test.ts index 51cc50c..05ad3df 100644 --- a/apps/cli/src/scan/__tests__/engine.test.ts +++ b/apps/cli/src/scan/__tests__/engine.test.ts @@ -1,6 +1,12 @@ import { describe, expect, it } from 'vitest'; import { parseFailOn } from '../../commands/scan.js'; -import { collectSuppressions, languageOf, meetsFailThreshold, scanText } from '../engine.js'; +import { + collectSuppressions, + languageOf, + languageOfShebang, + meetsFailThreshold, + scanText, +} from '../engine.js'; import { detectTyposquat, editDistance, scanPackageJson, scanRequirementsTxt } from '../manifest-rules.js'; import { isKnownPlaceholder, redactSecret } from '../secret-rules.js'; import type { ScanFinding } from '../types.js'; @@ -14,6 +20,32 @@ describe('language detection', () => { }); }); +describe('shebang detection', () => { + // An executable is named for the command it provides, not the language it is + // written in. `debtap` is 3,500 lines of bash with no extension, and + // extension-only detection scanned zero of them while reporting success. + it('reads the interpreter from a shebang', () => { + expect(languageOfShebang('#!/usr/bin/bash')).toBe('shell'); + expect(languageOfShebang('#!/bin/sh')).toBe('shell'); + expect(languageOfShebang('#!/usr/bin/env bash')).toBe('shell'); + expect(languageOfShebang('#!/usr/bin/env python3')).toBe('python'); + expect(languageOfShebang('#!/usr/bin/env node')).toBe('javascript'); + expect(languageOfShebang('#!/usr/bin/ruby')).toBe('ruby'); + }); + + it('tolerates a version suffix and extra whitespace', () => { + expect(languageOfShebang('#! /bin/bash')).toBe('shell'); + expect(languageOfShebang('#!/usr/bin/python3.11')).toBe('python'); + }); + + it('returns null for anything that is not a recognised interpreter', () => { + expect(languageOfShebang('#!/usr/bin/env perl')).toBeNull(); + expect(languageOfShebang('# a comment, not a shebang')).toBeNull(); + expect(languageOfShebang('')).toBeNull(); + expect(languageOfShebang('\x7fELF\x02\x01')).toBeNull(); + }); +}); + describe('secret redaction', () => { it('never emits the matched credential', () => { const redacted = redactSecret('AWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE'); diff --git a/apps/cli/src/scan/code-rules.ts b/apps/cli/src/scan/code-rules.ts index 98d6d16..4bd6196 100644 --- a/apps/cli/src/scan/code-rules.ts +++ b/apps/cli/src/scan/code-rules.ts @@ -128,8 +128,21 @@ const UNTRUSTED_GO = const UNTRUSTED_JAVA = /\bgetParameter\s*\(|\bgetQueryString\s*\(|\bgetHeader\s*\(|\bgetInputStream\s*\(|\bgetCookies\s*\(|\b@RequestParam\b|\b@PathVariable\b/; +/** + * Untrusted input to a shell script: what the caller controls. + * + * Positional parameters and `read` are the whole surface. Environment + * variables are deliberately absent — a script's own configuration arrives + * that way, so treating `$PREFIX` as attacker-controlled would mark every + * line of every installer. + */ +const UNTRUSTED_SH = + /\$\{?[1-9]\d*\b|\$[@*]|\$\{@\}|\bread\s+(?:-\S+\s+)*[A-Za-z_]\w*|\$\{?REPLY\b|\$\{?QUERY_STRING\b/; + export function untrustedPatternFor(language: ScanLanguage): RegExp { switch (language) { + case 'shell': + return UNTRUSTED_SH; case 'python': return UNTRUSTED_PY; case 'ruby': @@ -709,6 +722,127 @@ export const CODE_RULES: readonly CodeRule[] = [ pattern: /\[\s*['"]__proto__['"]\s*\]|\bObject\s*\.\s*assign\s*\(\s*[\w.$]*\.prototype\b|\.\s*__proto__\s*=/, }, + + // ── Shell ──────────────────────────────────────────────────────────────── + // + // `shell` was a language the type system knew about and no rule targeted, so + // a repository written entirely in bash got secret detection and nothing + // else. Installers, CI helpers and packaging scripts are where a great deal + // of privileged work actually happens, and they run as whoever invoked them. + { + id: 'sh-remote-script-execution', + title: 'network output piped into a shell', + consequence: + 'Whatever that URL serves at the moment this runs is executed as the invoking user. There is no version, no signature, and no review — a compromise of the host, or anyone able to answer for it, is a compromise of every machine that runs the script.', + cwe: 'CWE-494', + severity: 'high', + languages: ['shell'], + // The pipe must be the *next* thing: `curl -o f url && sh f` is a different + // (and checkable) shape, and `curl url | jq` is not an execution at all. + pattern: /\b(?:curl|wget)\b[^|\n]*\|\s*(?:sudo\s+(?:-\S+\s+)*)?(?:\/bin\/|\/usr\/bin\/)?(?:ba|da|k|z|a)?sh\b/, + // Nothing on this line can exonerate it. Integrity checking happens in a + // separate step by construction, so a window guard would only mislead. + guard: false, + }, + { + id: 'sh-eval-expansion', + title: 'eval on an expanded string', + consequence: + 'The expansion is re-parsed as shell source, so a `;` or `$(…)` anywhere in the value runs as a command rather than arriving as data.', + cwe: 'CWE-78', + severity: 'high', + languages: ['shell'], + // Matched against eval's *argument*, not against the rest of the line. + // + // `\beval\b.*\$` looks equivalent and is not: bash's ordinary dynamic-range + // idiom, `eval echo {$((k + 1))..$((k + n))}`, contains `$k` inside the + // arithmetic, so a line-wide search finds an expansion in a construct that + // cannot carry a command — `$((…))` is parsed as an expression, where a `;` + // is a syntax error rather than a second command. That spelling reported + // 355 findings in one 3,500-line script, all of them the same safe loop. + // + // What is left is the form where eval is handed a value directly — + // `eval "$cmd"`, `eval $cmd`, `eval "$(…)"` — which is the shape that + // actually re-parses untrusted text as source. `eval echo $x` is not + // covered; catching it without also catching the range idiom needs to know + // which expansions are arithmetic, which is parsing, not matching. + pattern: /\beval\s+(?:-\S+\s+)*(?:"\s*)?\$(?:\{?[A-Za-z_]\w*|\((?!\())/, + // The shell-init idiom `eval "$(tool init -)"` is the documented interface + // of most version managers. It is still eval of program output, but the + // program is a fixed local binary, and flagging it reports every developer + // dotfile in existence. + lineGuard: + /\beval\s+"?\$\(\s*(?:ssh-agent|dircolors|direnv|rbenv|pyenv|nodenv|goenv|jenv|tfenv|opam|luarocks|conda|mamba|zoxide|starship|mise|asdf|fnm|nvm|brew|thefuck|register-python-argcomplete|_\w+_completion)\b/, + }, + { + id: 'sh-unquoted-expansion-destructive', + title: 'unquoted expansion in a destructive command', + consequence: + 'An unquoted expansion is word-split and glob-expanded before the command sees it. A value with a space removes two paths instead of one; an empty value removes the argument entirely, which is how `rm -rf $DIR/` becomes `rm -rf /`.', + cwe: 'CWE-78', + severity: 'high', + languages: ['shell'], + // `[^"'\n]*?` cannot cross a quote, so `rm -rf "$dir"` — the correct form — + // never reaches the `$` and never matches. Only a genuinely bare expansion + // does. Restricted to recursive/forced removal: a bare `$f` in `rm $f` is + // sloppy, but it is not the shape that erases a filesystem. + pattern: /\brm\s+(?:-[a-zA-Z-]*[rRf][a-zA-Z-]*\s+)+[^"'\n]*?\$\{?[A-Za-z_]/, + }, + { + id: 'sh-insecure-transport-flag', + title: 'certificate verification disabled', + consequence: + 'Anyone positioned between this host and the server can substitute the response. When the response is a package, a key or a script, that is remote code execution with the transport doing nothing to stop it.', + cwe: 'CWE-295', + severity: 'high', + languages: ['shell'], + pattern: + /\b(?:curl|wget)\b[^\n|]*(?:\s-k(?=\s|$)|\s--insecure\b|\s--no-check-certificate\b)/, + // Over plain HTTP there is no certificate to skip, so the flag is inert and + // this rule has nothing to say — `sh-plaintext-download` is the finding + // that fits. Reporting both put two entries on one line, one of which + // recommended a fix that would change nothing. + lineGuard: /^(?!.*https:\/\/).*\bhttp:\/\//, + }, + { + id: 'sh-plaintext-download', + title: 'download over plain HTTP', + consequence: + 'The response arrives unauthenticated over a channel any intermediary can rewrite. Where the payload is an archive, a package list or a key, substituting it is straightforward and leaves nothing for the script to notice.', + cwe: 'CWE-319', + severity: 'high', + languages: ['shell'], + pattern: /\b(?:curl|wget)\b[^\n|]*\bhttp:\/\//, + // Loopback and link-local are not carried over a network anyone can sit on. + lineGuard: + /http:\/\/(?:localhost|127\.0\.0\.1|0\.0\.0\.0|\[::1\]|169\.254\.|host\.docker\.internal)\b/, + }, + { + id: 'sh-world-writable-permissions', + title: 'world-writable permissions', + consequence: + 'Any local account can rewrite the file. If it is a script, a config or anything on a privileged path, the next process to read it runs someone else’s content.', + cwe: 'CWE-732', + severity: 'medium', + languages: ['shell'], + pattern: /\bchmod\s+(?:-[a-zA-Z-]+\s+)*(?:0?777|a\+rwx|ugo\+rwx|a=rwx)\b/, + }, + { + id: 'sh-predictable-temp-path', + title: 'predictable temporary file', + consequence: + 'The name is guessable, so a local attacker can create it first — as a symlink to somewhere that matters — and the script writes through it with its own privileges.', + cwe: 'CWE-377', + severity: 'medium', + languages: ['shell'], + // Redirection or an explicit write into a literal `/tmp` path. A `$$` or + // `$RANDOM` suffix is still predictable, so it is not treated as a fix; + // `mktemp` is, and it is the guard below. + pattern: /(?:>{1,2}\s*|\b(?:tee|touch|cp|mv|install)\s+(?:-\S+\s+)*)\/tmp\/[\w.$-]+/, + guard: /\bmktemp\b/, + guardBack: 6, + guardForward: 2, + }, ]; /** diff --git a/apps/cli/src/scan/engine.ts b/apps/cli/src/scan/engine.ts index 7ff5efe..81b8114 100644 --- a/apps/cli/src/scan/engine.ts +++ b/apps/cli/src/scan/engine.ts @@ -7,7 +7,9 @@ * without three implementations drifting apart. */ -import { closeSync, fstatSync, openSync, readdirSync, readFileSync, statSync } from 'node:fs'; +import { + closeSync, fstatSync, openSync, readdirSync, readFileSync, readSync, statSync, +} from 'node:fs'; import { basename, dirname, extname, join, relative, sep } from 'node:path'; import { CODE_RULES, evaluateRule, proseLines } from './code-rules.js'; import { scanPackageJson, scanRequirementsTxt } from './manifest-rules.js'; @@ -50,6 +52,43 @@ export function languageOf(filename: string): ScanLanguage { return LANGUAGE_BY_EXTENSION[extname(filename).toLowerCase()] ?? 'other'; } +/** Interpreters worth recognising, by the language their scripts are written in. */ +const LANGUAGE_BY_INTERPRETER: Record = { + sh: 'shell', bash: 'shell', zsh: 'shell', dash: 'shell', ksh: 'shell', ash: 'shell', + python: 'python', python2: 'python', python3: 'python', + ruby: 'ruby', + node: 'javascript', nodejs: 'javascript', deno: 'javascript', bun: 'javascript', + php: 'php', +}; + +/** + * The language a `#!` line declares, or `null` if the line is not a shebang. + * + * An executable in a repository root is routinely named for the command it + * provides rather than the language it is written in — `debtap`, `configure`, + * `gradlew`. Extension-based detection skips every one of them, which is worst + * exactly where it matters: a project whose entire source is one extensionless + * script gets a clean scan because nothing was read. + * + * The shebang is the authoritative answer to a question the filename cannot + * answer, and the kernel already treats it that way. + */ +export function languageOfShebang(firstLine: string): ScanLanguage | null { + const match = /^#!\s*(\S+)(?:\s+(\S+))?/.exec(firstLine); + if (!match) return null; + + // `#!/usr/bin/env bash` names the interpreter in the argument, not the path. + const command = basename(match[1]!); + const name = command === 'env' && match[2] ? basename(match[2]) : command; + + const exact = LANGUAGE_BY_INTERPRETER[name]; + if (exact) return exact; + + // `python3.11` and `bash5` are the same interpreters with a version glued on. + const stripped = name.replace(/[\d.]+$/, ''); + return (stripped ? LANGUAGE_BY_INTERPRETER[stripped] : undefined) ?? null; +} + export interface ScanOptions { /** Skip files larger than this. Defaults to 1 MiB. */ maxFileBytes?: number; @@ -272,7 +311,16 @@ export function scanPath(targetPath: string, options: ScanOptions = {}): ScanRep const isManifest = filename === 'package.json' || filename === 'requirements.txt'; const scannable = SCAN_EXTENSIONS.has(extension) || filename.startsWith('.env'); - if (!scannable && !isManifest) { + // A file with no extension gets one question asked of it before being + // dismissed: does it start with a shebang? Executables are habitually named + // for what they do rather than what they are written in, and skipping them + // silently is how a repository whose only source file is `debtap` scans + // clean. Files that carry an unrecognised extension are still skipped — + // `.png` is not a script, and sniffing every one of them would mean reading + // the whole tree. + const mayDeclareInterpreter = !scannable && !isManifest && extension === ''; + + if (!scannable && !isManifest && !mayDeclareInterpreter) { recordSensitiveFile(filename, relativePath, findings, []); return; } @@ -290,6 +338,7 @@ export function scanPath(targetPath: string, options: ScanOptions = {}): ScanRep // right in its own walker. let text: string; let handle: number; + let declared: ScanLanguage | null = null; try { handle = openSync(fullPath, 'r'); } catch { @@ -299,6 +348,17 @@ export function scanPath(targetPath: string, options: ScanOptions = {}): ScanRep try { if (fstatSync(handle).size > maxFileBytes) return; + + // Sniff the shebang from a short prefix rather than the whole file, so an + // extensionless blob — a checked-in binary, a data file — costs one small + // read instead of a megabyte decoded as UTF-8 and thrown away. + if (mayDeclareInterpreter) { + const prefix = Buffer.alloc(128); + const read = readSync(handle, prefix, 0, prefix.length, 0); + declared = languageOfShebang(prefix.subarray(0, read).toString('utf-8').split('\n', 1)[0] ?? ''); + if (!declared) return; + } + text = readFileSync(handle, 'utf-8'); } catch { unreadable.push(relativePath); @@ -316,7 +376,7 @@ export function scanPath(targetPath: string, options: ScanOptions = {}): ScanRep suppressed += collectSuppressions(text.split('\n')).count; const fileFindings = [ - ...scanText(relativePath, text, languageOf(filename)), + ...scanText(relativePath, text, declared ?? languageOf(filename)), ...(isManifest ? scanManifest(relativePath, filename, text) : []), ];