From b83e487cbf8a7b3369285129299705dc8cad2308 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 10 Aug 2026 17:14:57 +0000 Subject: [PATCH] feat(scan): scan shebang scripts, and give shell a rule set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. Worse, language detection was extension-only: an executable named for the command it provides rather than the language it is written in was never opened at all. Both together meant `ralyodio/debtap` — 3,511 lines of bash in a file called `debtap` — scanned clean by scanning nothing, and reported success while doing it. - Read the interpreter from a `#!` line for extensionless files, sniffed from a 128-byte prefix so a checked-in blob costs one small read rather than a megabyte decoded and discarded. Unrecognised extensions are still skipped; `.png` is not a script. - Seven shell rules: remote script execution, eval on an expansion, unquoted expansion in a recursive remove, disabled certificate verification, plain-HTTP download, world-writable permissions and predictable temp paths. Each rule is built against the corrected shape as well as the vulnerable one. The eval rule matches eval's argument rather than the whole line, because `\beval\b.*\$` counts bash's dynamic-range idiom — where every expansion is arithmetic and cannot carry a command — and reported it 355 times in debtap alone. On debtap: 0 findings (nothing scanned) to 8, all genuine — `curl -k` against HTTPS and plain-HTTP fetches whose payloads build packages. Capacitor is unchanged at 13. 83 tests pass, up from 68. Co-Authored-By: Claude Opus 5 --- .../cli/src/scan/__tests__/code-rules.test.ts | 89 ++++++++++++ apps/cli/src/scan/__tests__/engine.test.ts | 34 ++++- apps/cli/src/scan/code-rules.ts | 134 ++++++++++++++++++ apps/cli/src/scan/engine.ts | 66 ++++++++- 4 files changed, 319 insertions(+), 4 deletions(-) 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) : []), ];