From 0810e60b4e5f233054ea840838abd8e35839e09b Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Tue, 28 Jul 2026 17:18:43 +0000 Subject: [PATCH] feat(code-scanner): add the sast subsystem MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements PRD 0004: line-oriented rules for dangerous constructs — dynamic code execution, shell/SQL interpolation, unsafe HTML, disabled TLS verification, Math.random for tokens, path traversal, prototype pollution. The design constraint is honesty about what a regex can know. CodeQL and Semgrep already run in this repo's CI and do real interprocedural analysis; this covers what they cannot see — source on a running server that never passed through the repository. So every finding carries a confidence: pattern the construct exists. Capped at medium severity, enforced in severityFor() rather than per-rule so no future rule can opt out of the cap by accident. contextual the construct sits alongside apparent untrusted input. The only level allowed to escalate to high or critical. Presenting a bare pattern match with the confidence of a proven vulnerability is the failure mode that discredits a scanner and every accurate finding standing next to it. There is a test asserting the cap holds for every rule. Noise control is the rest of the work: literal `exec('ls')` is not a finding while `exec(\`${x}\`)` is; Math.random is only flagged when assigned to something security-shaped; comment lines are skipped, without which the subsystem reports findings about its own rule descriptions; rules describing ordinary operations (reading a computed path) require context before firing at all. Suppressions are inline, per-rule, carry a reason, and are counted and reported — a quiet scan full of suppressions is not a clean one. 86 tests (20 new). Co-Authored-By: Claude Opus 4.8 --- modules/code-scanner/README.md | 2 +- modules/code-scanner/mod.toml | 20 ++ .../code-scanner/src/__tests__/sast.test.ts | 178 +++++++++++++ modules/code-scanner/src/index.ts | 84 +++++- modules/code-scanner/src/sast/index.ts | 160 ++++++++++++ modules/code-scanner/src/sast/rules.ts | 241 ++++++++++++++++++ ...rns-without-pretending-to-be-a-compiler.md | 215 ++++++++++++++++ prd/README.md | 1 + 8 files changed, 898 insertions(+), 3 deletions(-) create mode 100644 modules/code-scanner/src/__tests__/sast.test.ts create mode 100644 modules/code-scanner/src/sast/index.ts create mode 100644 modules/code-scanner/src/sast/rules.ts create mode 100644 prd/0004-find-dangerous-code-patterns-without-pretending-to-be-a-compiler.md diff --git a/modules/code-scanner/README.md b/modules/code-scanner/README.md index e42425f..f8df60b 100644 --- a/modules/code-scanner/README.md +++ b/modules/code-scanner/README.md @@ -8,7 +8,7 @@ same directories, so the module is a thin host and each one is a **subsystem**: | --- | --- | --- | | `deps/` | Dependency advisories, malicious install scripts, lockfile drift | **implemented** ([PRD 0002](../../prd/0002-detect-vulnerable-and-malicious-dependencies-on-running-servers.md)) | | `secrets/` | Hardcoded credentials, redacted by construction | **implemented** ([PRD 0003](../../prd/0003-detect-hardcoded-secrets-before-they-are-committed-or-served.md)) | -| `sast/` | Source-level vulnerability analysis | not yet built | +| `sast/` | Dangerous source patterns, with stated confidence | **implemented** ([PRD 0004](../../prd/0004-find-dangerous-code-patterns-without-pretending-to-be-a-compiler.md)) | | `config/` | Misconfiguration checks | not yet built | ```bash diff --git a/modules/code-scanner/mod.toml b/modules/code-scanner/mod.toml index b3019e1..0071e11 100644 --- a/modules/code-scanner/mod.toml +++ b/modules/code-scanner/mod.toml @@ -42,6 +42,26 @@ max_alerts = 25 # truth is "nothing was read". An unexamined project is not a clean one. fail_on_unparseable = true +# --- sast subsystem: dangerous source patterns (PRD 0004) ----------------- +sast_enabled = true + +# Third-party, generated and minified code: +# rank_down demote one severity level (default) +# equal treat identically to first-party source +# Never hidden — a vulnerability in vendor/ still executes. +sast_vendored = "rank_down" + +# Minimum confidence to report: +# pattern the dangerous construct exists (capped at medium severity) +# contextual the construct sits alongside apparent untrusted input +# This subsystem is line-oriented pattern matching, not data-flow analysis, and +# will not present a bare pattern match as a proven vulnerability. CodeQL and +# Semgrep do the real analysis in CI; this covers source on a running server +# that never passed through the repository. +sast_min_confidence = "pattern" + +sast_max_file_bytes = 2000000 + # --- secrets subsystem: hardcoded credential detection (PRD 0003) --------- secrets_enabled = true diff --git a/modules/code-scanner/src/__tests__/sast.test.ts b/modules/code-scanner/src/__tests__/sast.test.ts new file mode 100644 index 0000000..9441c24 --- /dev/null +++ b/modules/code-scanner/src/__tests__/sast.test.ts @@ -0,0 +1,178 @@ +import { describe, expect, it } from 'vitest'; +import { SAST_RULES, isVendored, scanSource, severityFor } from '../sast/index.js'; + +const rule = (id: string) => SAST_RULES.find((r) => r.id === id)!; + +/** + * THE OVERCLAIMING TEST (PRD 0004 R13). + * + * A regex is not data-flow analysis. The failure mode that would discredit this + * subsystem — and the accurate findings standing next to it — is presenting a + * bare pattern match with the confidence of a proven vulnerability. The cap is + * enforced in `severityFor` rather than per-rule, so no future rule can opt out + * of it by accident, and this test is what keeps that true. + */ +describe('confidence caps severity', () => { + it('never lets a pattern-only match exceed medium', () => { + for (const r of SAST_RULES) { + const capped = severityFor(r, 'pattern'); + expect(['low', 'medium'], `${r.id} escalated on a bare pattern`).toContain(capped); + } + }); + + it('allows the rule severity once there is context', () => { + expect(severityFor(rule('js-eval-call'), 'contextual')).toBe('critical'); + expect(severityFor(rule('js-eval-call'), 'pattern')).toBe('medium'); + }); + + it('holds for every finding a real scan produces', () => { + const source = [ + 'eval(userInput);', + 'exec(`convert ${req.body.file} out.png`);', + 'const x = 1;', + ].join('\n'); + + for (const finding of scanSource(source).findings) { + if (finding.confidence === 'pattern') { + expect(['low', 'medium'], `${finding.ruleId} overclaimed`).toContain(finding.severity); + } + } + }); +}); + +describe('rule detection', () => { + it('finds dynamic code execution', () => { + expect(scanSource('eval(payload);').findings.map((f) => f.ruleId)).toContain('js-eval-call'); + expect(scanSource('const f = new Function("return 1");').findings.map((f) => f.ruleId)).toContain( + 'js-eval-call', + ); + }); + + it('finds shell execution only when the command is interpolated', () => { + const interpolated = scanSource('exec(`rm -rf ${dir}`);'); + expect(interpolated.findings.map((f) => f.ruleId)).toContain('js-exec-interpolation'); + + // A fixed command string is not a finding; flagging it is how a scanner + // earns its reputation for noise. + const literal = scanSource("exec('ls -la');"); + expect(literal.findings.map((f) => f.ruleId)).not.toContain('js-exec-interpolation'); + }); + + it('finds SQL built by interpolation', () => { + const found = scanSource('db.query(`SELECT * FROM users WHERE id = ${id}`);'); + expect(found.findings.map((f) => f.ruleId)).toContain('js-sql-concatenation'); + }); + + it('finds disabled TLS verification', () => { + expect(scanSource('const a = { rejectUnauthorized: false };').findings.map((f) => f.ruleId)).toContain( + 'js-tls-verification-disabled', + ); + }); + + it('finds Math.random used for a security value', () => { + const found = scanSource('const token = Math.random().toString(36);'); + expect(found.findings.map((f) => f.ruleId)).toContain('js-insecure-randomness'); + + // Ordinary randomness is not a security bug. + expect(scanSource('const jitter = Math.random() * 100;').findings).toHaveLength(0); + }); + + it('finds unsafe HTML rendering', () => { + expect( + scanSource('el.innerHTML = userProvided;').findings.map((f) => f.ruleId), + ).toContain('js-unsafe-html'); + }); + + it('finds prototype pollution shapes', () => { + expect(scanSource('target["__proto__"] = source;').findings.map((f) => f.ruleId)).toContain( + 'js-prototype-pollution', + ); + }); + + it('every rule carries a CWE and a consequence, not just a name', () => { + for (const r of SAST_RULES) { + expect(r.cwe, r.id).toMatch(/^CWE-\d+$/); + expect(r.consequence.length, r.id).toBeGreaterThan(20); + } + }); +}); + +describe('context gating', () => { + it('escalates when untrusted input is on the same line', () => { + const found = scanSource('exec(`convert ${req.body.file} out.png`);'); + expect(found.findings[0]?.confidence).toBe('contextual'); + expect(found.findings[0]?.severity).toBe('critical'); + }); + + it('stays at pattern confidence without an untrusted source', () => { + const found = scanSource('exec(`convert ${localFile} out.png`);'); + expect(found.findings[0]?.confidence).toBe('pattern'); + expect(found.findings[0]?.severity).toBe('medium'); + }); + + it('suppresses needsContext rules entirely when there is no context', () => { + // Reading a file from a computed path is just software. + expect(scanSource('readFile(`${dir}/config.json`, cb);').findings).toHaveLength(0); + // With a request field it becomes a traversal candidate. + expect( + scanSource('readFile(`${req.query.path}/config.json`, cb);').findings.map((f) => f.ruleId), + ).toContain('js-path-traversal'); + }); +}); + +describe('noise control', () => { + it('reports nothing on ordinary application code', () => { + const ordinary = [ + 'import express from "express";', + 'const app = express();', + 'app.get("/health", (req, res) => res.json({ ok: true }));', + 'const total = items.reduce((a, b) => a + b.price, 0);', + 'export default app;', + ].join('\n'); + expect(scanSource(ordinary).findings).toHaveLength(0); + }); + + it('ignores comments, including rule documentation', () => { + // Without this the subsystem reports findings about its own rule file. + const commented = ['// eval(payload) is dangerous', ' * exec(`${x}`) runs a shell'].join('\n'); + expect(scanSource(commented).findings).toHaveLength(0); + }); +}); + +describe('suppressions', () => { + it('honours an inline disable for the named rule only', () => { + const source = [ + '// threatcrush-disable-next-line js-eval-call sandboxed by design', + 'eval(sandboxedExpression);', + ].join('\n'); + const result = scanSource(source); + expect(result.findings).toHaveLength(0); + expect(result.suppressions[0]).toMatchObject({ + ruleId: 'js-eval-call', + reason: 'sandboxed by design', + }); + }); + + it('does not suppress a different rule on the same line', () => { + const source = [ + '// threatcrush-disable-next-line js-sql-concatenation', + 'eval(payload);', + ].join('\n'); + expect(scanSource(source).findings.map((f) => f.ruleId)).toContain('js-eval-call'); + }); + + it('records a placeholder when no reason is given', () => { + const source = ['// threatcrush-disable-next-line js-eval-call', 'eval(x);'].join('\n'); + // Counted and reported: a quiet scan full of suppressions is not clean. + expect(scanSource(source).suppressions[0]?.reason).toBe('(no reason given)'); + }); +}); + +describe('vendored classification', () => { + it('recognises third-party and generated locations', () => { + expect(isVendored('/srv/app/vendor/lib.js')).toBe(true); + expect(isVendored('/srv/app/dist/bundle.js')).toBe(true); + expect(isVendored('/srv/app/static/app.min.js')).toBe(true); + expect(isVendored('/srv/app/src/routes/admin.ts')).toBe(false); + }); +}); diff --git a/modules/code-scanner/src/index.ts b/modules/code-scanner/src/index.ts index ab2f6c3..d20b3d0 100644 --- a/modules/code-scanner/src/index.ts +++ b/modules/code-scanner/src/index.ts @@ -8,7 +8,7 @@ * * deps/ dependency and supply-chain scanning — PRD 0002, implemented * secrets/ hardcoded credential detection — PRD 0003, implemented - * sast/ source-level vulnerability analysis — not yet built + * sast/ source-level vulnerability analysis — PRD 0004, implemented * config/ misconfiguration checks — not yet built * * PRD 0002 originally proposed dependency scanning as a standalone @@ -38,6 +38,12 @@ import { type DepsResult, type Finding, } from './deps/index.js'; +import { + SAST_DEFAULTS, + scanSast, + type SastOptions, + type SastResult, +} from './sast/index.js'; import { SECRETS_DEFAULTS, scanSecrets, @@ -56,6 +62,7 @@ const DEFAULTS = { export interface ScanResult { deps: DepsResult | null; secrets: SecretsResult | null; + sast: SastResult | null; /** True when any subsystem could not complete — never report "clean". */ incomplete: boolean; } @@ -122,7 +129,8 @@ export default class CodeScannerModule implements ThreatCrushModule { async scan(): Promise { const deps = this.depsEnabled() ? await scanDependencies(this.depsOptions()) : null; const secrets = this.secretsEnabled() ? await scanSecrets(this.secretsOptions()) : null; - return { deps, secrets, incomplete: Boolean(deps?.incomplete) }; + const sast = this.sastEnabled() ? await scanSast(this.sastOptions()) : null; + return { deps, secrets, sast, incomplete: Boolean(deps?.incomplete) }; } private depsEnabled(): boolean { @@ -133,6 +141,23 @@ export default class CodeScannerModule implements ThreatCrushModule { return this.ctx.config.secrets_enabled !== false; } + private sastEnabled(): boolean { + return this.ctx.config.sast_enabled !== false; + } + + private sastOptions(): SastOptions { + const cfg = this.ctx.config; + return { + paths: this.paths(), + maxDepth: (cfg.max_depth as number | undefined) ?? SAST_DEFAULTS.maxDepth, + maxFileBytes: (cfg.sast_max_file_bytes as number | undefined) ?? SAST_DEFAULTS.maxFileBytes, + vendored: (cfg.sast_vendored as SastOptions['vendored'] | undefined) ?? SAST_DEFAULTS.vendored, + minConfidence: + (cfg.sast_min_confidence as SastOptions['minConfidence'] | undefined) ?? + SAST_DEFAULTS.minConfidence, + }; + } + private secretsOptions(): SecretsOptions { const cfg = this.ctx.config; const allow = Array.isArray(cfg.secrets_allow) ? (cfg.secrets_allow as string[]) : []; @@ -174,6 +199,7 @@ export default class CodeScannerModule implements ThreatCrushModule { const reported = new Set(this.readState(STATE_REPORTED, [])); if (result.secrets) this.reportSecrets(result.secrets, floor, maxAlerts, reported); + if (result.sast) this.reportSast(result.sast, floor, maxAlerts, reported); const deps = result.deps; if (!deps) { @@ -330,6 +356,59 @@ export default class CodeScannerModule implements ThreatCrushModule { ); } + /** + * Alert on dangerous constructs. + * + * The confidence is in the alert body, not implied by tone: a `pattern` + * finding says the construct exists, and the word "vulnerability" is + * reserved for `contextual` (PRD 0004 R13). + */ + private reportSast( + sast: SastResult, + floor: number, + maxAlerts: number, + reported: Set, + ): void { + const notable = sast.findings.filter((f) => severityRank(f.severity) >= floor); + + for (const finding of notable + .filter((f) => !reported.has(`sast:${f.file}:${f.ruleId}:${f.line}`)) + .slice(0, maxAlerts)) { + const headline = `code-scanner: ${finding.title} in ${finding.file}:${finding.line}`; + this.ctx.emit( + this.event('scan', finding.severity, headline, { + rule: finding.ruleId, + cwe: finding.cwe, + file: finding.file, + line: finding.line, + confidence: finding.confidence, + vendored: finding.vendored, + }), + ); + this.ctx.alert({ + title: headline, + severity: finding.severity, + body: [ + `${finding.file}:${finding.line} ${finding.ruleId} (${finding.cwe})`, + finding.excerpt, + '', + `confidence: ${finding.confidence}`, + finding.consequence, + ].join('\n'), + } satisfies Alert); + reported.add(`sast:${finding.file}:${finding.ruleId}:${finding.line}`); + } + + // A quiet scan full of suppressions is not a clean one. + this.ctx.logger.info( + '[%s] sast: %d file(s) scanned, %d finding(s), %d suppression(s)', + this.name, + sast.filesScanned, + sast.findings.length, + sast.suppressions.length, + ); + } + private paths(): string[] { const configured = this.ctx.config.paths; if (Array.isArray(configured) && configured.length > 0) return configured as string[]; @@ -385,4 +464,5 @@ export function summarize(result: ScanResult): string { } export * from './deps/index.js'; +export * from './sast/index.js'; export * from './secrets/index.js'; diff --git a/modules/code-scanner/src/sast/index.ts b/modules/code-scanner/src/sast/index.ts new file mode 100644 index 0000000..8b8c239 --- /dev/null +++ b/modules/code-scanner/src/sast/index.ts @@ -0,0 +1,160 @@ +/** + * Source scanning — the `sast` subsystem of `code-scanner`. + * + * Implements PRD 0004. See `rules.ts` for the design constraint that shapes + * everything here: findings carry a confidence, and a bare pattern match can + * never present as high severity. + * + * This subsystem is deliberately narrow and says so. CodeQL and Semgrep run in + * this repo's CI and do real interprocedural analysis; the value added here is + * coverage of source that never reached the repository — vendored trees, + * generated files, a hotfix applied in place on the box. + */ + +export * from './rules.js'; + +import { readdir, readFile, stat } from 'node:fs/promises'; +import { extname, join } from 'node:path'; + +import { scanSource, type SastFinding, type SuppressionRecord } from './rules.js'; + +export interface SastFileFinding extends SastFinding { + file: string; + /** Third-party or generated code: ranked down, never hidden. */ + vendored: boolean; +} + +export interface SastOptions { + paths: readonly string[]; + maxDepth: number; + maxFileBytes: number; + /** `rank_down` demotes vendored findings; `equal` leaves them. */ + vendored: 'rank_down' | 'equal'; + minConfidence: 'pattern' | 'contextual'; +} + +export interface SastResult { + findings: SastFileFinding[]; + filesScanned: number; + suppressions: (SuppressionRecord & { file: string })[]; +} + +export const SAST_DEFAULTS: SastOptions = { + paths: ['/srv', '/var/www', '/opt'], + maxDepth: 8, + maxFileBytes: 2_000_000, + vendored: 'rank_down', + minConfidence: 'pattern', +}; + +const SOURCE_EXTENSIONS = new Set(['.js', '.mjs', '.cjs', '.jsx', '.ts', '.tsx', '.mts', '.cts']); + +const SKIP_DIRS = new Set([ + 'node_modules', + '.git', + '.next', + '.turbo', + '.cache', + 'coverage', + '__pycache__', + '.venv', +]); + +/** + * Paths whose findings are ranked down. + * + * A vulnerability in vendored code still executes; it is simply less likely to + * be the operator's to fix, and `deps/` already covers third-party packages by + * version. Minified bundles are included because a finding in a 40,000-column + * line is unactionable regardless of whether it is real. + */ +const VENDORED = /(?:^|\/)(?:vendor|third_party|thirdparty|bundled|generated|dist|build|public\/assets)(?:\/|$)|\.min\.js$|\.bundle\.js$/; + +export function isVendored(path: string): boolean { + return VENDORED.test(path); +} + +const CONFIDENCE_RANK = { pattern: 0, contextual: 1 }; +const SEVERITY_RANK = { low: 0, medium: 1, high: 2, critical: 3 }; + +/** One step down the ladder, floored at low. */ +function demote(severity: SastFinding['severity']): SastFinding['severity'] { + const order: SastFinding['severity'][] = ['low', 'medium', 'high', 'critical']; + return order[Math.max(0, order.indexOf(severity) - 1)]!; +} + +async function walk(dir: string, depth: number, maxDepth: number, out: string[]): Promise { + if (depth > maxDepth) return; + + let entries; + try { + entries = await readdir(dir, { withFileTypes: true }); + } catch { + return; + } + + for (const entry of entries) { + const path = join(dir, entry.name); + if (entry.isDirectory()) { + if (SKIP_DIRS.has(entry.name)) continue; + await walk(path, depth + 1, maxDepth, out); + } else if (entry.isFile() && SOURCE_EXTENSIONS.has(extname(entry.name).toLowerCase())) { + out.push(path); + } + } +} + +/** + * Scan configured paths for dangerous constructs. + * + * Options in, result out, no `ModuleContext` — matching `deps` and `secrets`, + * so the same entry point serves the daemon and the one-shot CLI. + */ +export async function scanSast(options: SastOptions): Promise { + const findings: SastFileFinding[] = []; + const suppressions: (SuppressionRecord & { file: string })[] = []; + let filesScanned = 0; + + for (const base of options.paths) { + const files: string[] = []; + await walk(base, 0, options.maxDepth, files); + + for (const file of files) { + try { + if ((await stat(file)).size > options.maxFileBytes) continue; + } catch { + continue; + } + + let text: string; + try { + text = await readFile(file, 'utf8'); + } catch { + continue; + } + + filesScanned += 1; + const vendored = isVendored(file); + const result = scanSource(text); + + for (const record of result.suppressions) suppressions.push({ ...record, file }); + + for (const finding of result.findings) { + if (CONFIDENCE_RANK[finding.confidence] < CONFIDENCE_RANK[options.minConfidence]) continue; + findings.push({ + ...finding, + file, + vendored, + severity: + vendored && options.vendored === 'rank_down' ? demote(finding.severity) : finding.severity, + }); + } + } + } + + findings.sort( + (a, b) => SEVERITY_RANK[b.severity] - SEVERITY_RANK[a.severity] || a.file.localeCompare(b.file), + ); + + return { findings, filesScanned, suppressions }; +} diff --git a/modules/code-scanner/src/sast/rules.ts b/modules/code-scanner/src/sast/rules.ts new file mode 100644 index 0000000..5755f85 --- /dev/null +++ b/modules/code-scanner/src/sast/rules.ts @@ -0,0 +1,241 @@ +/** + * Source-pattern rules — the `sast` subsystem of `code-scanner`. + * + * Implements PRD 0004. The governing constraint is stated there and repeated + * here because it is the whole design: **a regex is not data-flow analysis**, + * and a tool that blurs the two is worse than no tool. It produces + * confident-sounding findings that waste triage time and, once disproven, + * discredit the accurate findings standing next to them. + * + * So confidence is part of the finding, not part of the prose: + * + * `pattern` the dangerous construct exists. Capped at medium severity, + * structurally — there is no code path by which a bare pattern + * match becomes a high. + * `contextual` the construct appears alongside something that looks like + * untrusted input. Still not proof, but a different claim, and + * the only kind allowed to escalate. + * + * CodeQL and Semgrep already run in this repo's CI and do the real analysis. + * This exists for what they cannot see: source on a running server that never + * passed through the repository. + */ + +export type SastSeverity = 'low' | 'medium' | 'high' | 'critical'; +export type Confidence = 'pattern' | 'contextual'; + +export interface SastRule { + id: string; + /** What the construct is. */ + title: string; + /** What happens if it is real. An operator triages on consequence. */ + consequence: string; + cwe: string; + pattern: RegExp; + /** Severity when the rule fires with `contextual` confidence. */ + severity: SastSeverity; + /** + * When set, the rule only escalates to `contextual` if this also appears on + * the line. Absent means the construct is dangerous regardless of input. + */ + needsContext?: boolean; +} + +/** + * Things that look like attacker-controlled input. + * + * Deliberately shallow — it is a heuristic for *ranking*, not a taint source + * model. A real source list would be framework-aware and interprocedural, + * which is exactly what this subsystem promises not to pretend to be. + */ +export const UNTRUSTED = /\b(?:req|request|ctx|context)\s*\.\s*(?:body|query|params|headers|cookies|url)\b|\bprocess\.argv\b|\bwindow\.location\b|\bdocument\.location\b|\blocation\.(?:search|hash|href)\b|\bsearchParams\b/; + +export const SAST_RULES: readonly SastRule[] = [ + { + id: 'js-eval-call', + title: 'dynamic code execution', + consequence: 'Any string reaching this call executes as code with the process’ privileges.', + cwe: 'CWE-95', + pattern: /\beval\s*\(|\bnew\s+Function\s*\(|\bvm\s*\.\s*runInThisContext\s*\(/, + severity: 'critical', + }, + { + id: 'js-exec-interpolation', + title: 'shell execution with an interpolated string', + consequence: 'A shell metacharacter in the interpolated value runs as the server user.', + cwe: 'CWE-78', + // Only interpolated forms. `exec('ls')` with a literal is not a finding. + pattern: /\b(?:exec|execSync|spawnSync?)\s*\(\s*(?:`[^`]*\$\{|['"][^'"]*['"]\s*\+)/, + severity: 'critical', + }, + { + id: 'js-sql-concatenation', + title: 'SQL assembled by concatenation or interpolation', + consequence: 'Input containing a quote can change the query’s meaning — classic SQL injection.', + cwe: 'CWE-89', + pattern: + /\b(?:query|execute|raw)\s*\(\s*(?:`[^`]*(?:SELECT|INSERT|UPDATE|DELETE|DROP)[^`]*\$\{|['"][^'"]*(?:SELECT|INSERT|UPDATE|DELETE|DROP)[^'"]*['"]\s*\+)/i, + severity: 'critical', + }, + { + id: 'js-unsafe-html', + title: 'unescaped HTML rendering', + consequence: 'A script tag in the value executes in the victim’s session — stored or reflected XSS.', + cwe: 'CWE-79', + pattern: + /\bdangerouslySetInnerHTML\s*=|\.innerHTML\s*=\s*(?!['"`]\s*['"`])|\bdocument\s*\.\s*write\s*\(/, + severity: 'high', + }, + { + id: 'js-tls-verification-disabled', + title: 'TLS certificate verification disabled', + consequence: + 'Every connection made this way is trivially interceptable; the encryption is decorative.', + cwe: 'CWE-295', + pattern: + /rejectUnauthorized\s*:\s*false|NODE_TLS_REJECT_UNAUTHORIZED\s*[=:]\s*['"]?0|strictSSL\s*:\s*false/, + severity: 'high', + }, + { + id: 'js-weak-hash-for-secret', + title: 'broken hash used on a credential', + consequence: 'MD5 and SHA-1 are fast and collision-prone; hashed passwords are recoverable.', + cwe: 'CWE-327', + pattern: /createHash\s*\(\s*['"](?:md5|sha1)['"]\s*\)[\s\S]{0,80}(?:password|passwd|secret|token)/i, + severity: 'high', + }, + { + id: 'js-insecure-randomness', + title: 'Math.random() used for a security value', + consequence: + 'Math.random is predictable; tokens, session ids and reset codes built from it are guessable.', + cwe: 'CWE-338', + pattern: + /(?:token|secret|password|salt|nonce|session|otp|reset|apikey|api_key)[\w]*\s*[:=][^;\n]{0,60}Math\s*\.\s*random\s*\(/i, + severity: 'high', + }, + { + id: 'js-path-traversal', + title: 'filesystem path built from a variable', + consequence: 'A `../` sequence in the value reads or writes outside the intended directory.', + cwe: 'CWE-22', + pattern: + /\b(?:readFile|readFileSync|writeFile|writeFileSync|createReadStream|createWriteStream|unlink|sendFile)\s*\(\s*(?:`[^`]*\$\{|[a-zA-Z_$][\w$]*\s*\+)/, + severity: 'medium', + needsContext: true, + }, + { + id: 'js-unsafe-yaml-load', + title: 'YAML parsed with type resolution enabled', + consequence: 'A crafted document can instantiate arbitrary types during parsing.', + cwe: 'CWE-502', + pattern: /\byaml\s*\.\s*load\s*\((?![^)]*safe)|loadAll\s*\([^)]*unsafe/i, + severity: 'high', + }, + { + id: 'js-prototype-pollution', + title: 'merge or assignment onto a prototype', + consequence: + 'An attacker-supplied `__proto__` key changes behaviour for every object in the process.', + cwe: 'CWE-1321', + pattern: /\[\s*['"]__proto__['"]\s*\]|Object\s*\.\s*assign\s*\(\s*[\w.$]*\.prototype\b/, + severity: 'high', + }, +]; + +export interface SastFinding { + ruleId: string; + title: string; + consequence: string; + cwe: string; + severity: SastSeverity; + confidence: Confidence; + line: number; + /** The source line, trimmed. Safe to show — unlike a secret. */ + excerpt: string; +} + +/** + * Cap for a bare pattern match. + * + * PRD 0004's central promise: a construct that merely *exists* never presents + * as high or critical. Enforced here rather than per-rule so no future rule can + * opt out of it by accident. + */ +export function severityFor(rule: SastRule, confidence: Confidence): SastSeverity { + if (confidence === 'contextual') return rule.severity; + const order: SastSeverity[] = ['low', 'medium', 'high', 'critical']; + return order[Math.min(order.indexOf(rule.severity), order.indexOf('medium'))]!; +} + +const SUPPRESS = /threatcrush-disable-next-line\s+([\w-]+)(?:\s+(.*))?/; + +export interface SuppressionRecord { + line: number; + ruleId: string; + reason: string; +} + +export interface SastScanResult { + findings: SastFinding[]; + /** Counted and reported: a quiet scan full of suppressions is not clean. */ + suppressions: SuppressionRecord[]; +} + +/** + * Scan a source file's text. + * + * Line-oriented on purpose. The moment this wants an AST it has become a + * different project, and the honest move is to defer to Semgrep rather than + * grow one badly. + */ +export function scanSource(text: string): SastScanResult { + const lines = text.split('\n'); + const findings: SastFinding[] = []; + const suppressions: SuppressionRecord[] = []; + + // A suppression on line N applies to line N+1. + const suppressed = new Map(); + lines.forEach((line, index) => { + const match = SUPPRESS.exec(line); + if (match?.[1]) { + suppressed.set(index + 1, match[1]); + suppressions.push({ + line: index + 1, + ruleId: match[1], + reason: match[2]?.trim() || '(no reason given)', + }); + } + }); + + lines.forEach((line, index) => { + // Comment-only lines are documentation, including this module's own rule + // descriptions. Scanning them produces findings about the scanner. + const trimmed = line.trim(); + if (trimmed.startsWith('//') || trimmed.startsWith('*') || trimmed.startsWith('#')) return; + + for (const rule of SAST_RULES) { + if (!rule.pattern.test(line)) continue; + if (suppressed.get(index) === rule.id) continue; + + const contextual = UNTRUSTED.test(line); + // A rule flagged `needsContext` describes a construct that is ordinary + // on its own — reading a file from a computed path is just software. + if (rule.needsContext && !contextual) continue; + + const confidence: Confidence = contextual ? 'contextual' : 'pattern'; + findings.push({ + ruleId: rule.id, + title: rule.title, + consequence: rule.consequence, + cwe: rule.cwe, + severity: severityFor(rule, confidence), + confidence, + line: index + 1, + excerpt: trimmed.slice(0, 200), + }); + } + }); + + return { findings, suppressions }; +} diff --git a/prd/0004-find-dangerous-code-patterns-without-pretending-to-be-a-compiler.md b/prd/0004-find-dangerous-code-patterns-without-pretending-to-be-a-compiler.md new file mode 100644 index 0000000..b018a6f --- /dev/null +++ b/prd/0004-find-dangerous-code-patterns-without-pretending-to-be-a-compiler.md @@ -0,0 +1,215 @@ +--- +openprd: "0.2" +id: "0004" +title: "Find dangerous code patterns without pretending to be a compiler" +status: Draft +authors: + - anthony@profullstack.com +created: 2026-07-28 +updated: 2026-07-28 +repo: profullstack/threatcrush +discussion: +implementation: modules/code-scanner (sast subsystem) +tags: code-scanner, sast, static-analysis, injection, taint, modules +supersedes: +superseded-by: +--- + +## Problem + +`PRD.md` lists "vulnerabilities" in `code-scanner`'s remit and +`threatcrush scan ./src` is already in the CLI. Neither is specified or built. + +**The honest framing matters more here than in any other subsystem.** Real +static analysis — the kind that finds an injection by proving a path from an +untrusted source to a dangerous sink — needs a parser, a control-flow graph and +interprocedural taint tracking per language. CodeQL and Semgrep are each many +years of work, and this repo already runs both in CI. + +So the question is not "can we build a SAST engine". It is **"what can a daemon +on a running server say about source code that is worth an operator's +attention, and can we say it without lying about our confidence?"** + +Three things make a narrow version worth building: + +**Pattern-level findings are genuinely useful and genuinely cheap.** A large +share of real-world web vulnerabilities are visible in a single expression: +`eval` on a request field, `child_process.exec` with a template literal, SQL +built by string concatenation, `res.send` of unescaped input, disabled TLS +verification, `dangerouslySetInnerHTML` fed by a variable. None of these need a +call graph to spot. + +**CI-based analysis has a coverage hole that the daemon does not.** CodeQL runs +on the repository. The daemon reads what is on the server — vendored code, +generated files, a hotfix applied in place, a plugin dropped into a directory, +a build artifact that no longer matches its source. This is the same coverage +argument that motivated PRD 0002 and PRD 0003. + +**Every scanner in this category loses trust the same way: by overclaiming.** +Reporting a regex match as though it were a proven data-flow finding trains +operators to disbelieve the tool. The `deps` subsystem's answer to this was to +distinguish "no vulnerabilities" from "could not parse"; the equivalent here is +to distinguish **"this is a dangerous construct"** from **"this is a proven +vulnerability"** — and to never claim the second from evidence that only +supports the first. + +## Goals + +- Dangerous constructs on a running server are surfaced within one scan + interval, with file, line and an explanation an operator can verify by eye. +- **Confidence is stated, not implied.** Every finding declares whether it is a + pattern match, a pattern match with a nearby untrusted source, or something + stronger. No finding is presented as more certain than its evidence. +- Precision high enough to stay switched on: a first run on a normal codebase + yields a reviewable list, not a wall of `eval` hits from vendored libraries. +- The subsystem is **honest about being narrow**. Documentation states plainly + that this is not a replacement for CodeQL/Semgrep, so nobody turns those off. + +## Non-Goals + +- **Not a replacement for CodeQL or Semgrep**, both of which already run in this + repo's CI. This complements them by running on servers rather than on + repositories. +- **Not interprocedural taint analysis.** No call graph, no cross-file data + flow, no framework-aware source/sink modelling. Claiming otherwise would be + the overclaim this PRD exists to avoid. +- **Not a linter.** Style, complexity, dead code and formatting are out of + scope; `code-scanner` is a security module and every finding must have a + security consequence. +- **Not autofix.** Rewriting source on a running server is a change nobody + asked a monitoring agent to make. +- **Not every language.** JavaScript/TypeScript first, because it is what + ThreatCrush's own users deploy; others only when the rules are real rather + than transliterated. + +## Users + +- **Solo founders and small teams** with no CI security analysis at all, for + whom a pattern-level scan is the difference between nothing and something. +- **Platform/ops engineers** auditing an estate of servers where vendored and + hand-modified code accumulates outside any repository. +- **Incident responders** asking what on this host could have been the entry + point. + +## Requirements + +### Analyse + +- R1 [P0] **File discovery** shared with the other subsystems, honouring the + same paths, skip rules and honesty reporting. +- R2 [P0] **Language detection by extension**, with JavaScript/TypeScript + (`.js`, `.mjs`, `.cjs`, `.ts`, `.tsx`, `.jsx`) as the P0 set. +- R3 [P0] **Line-oriented pattern rules** with per-rule severity, a CWE + reference, and a human explanation of the consequence — not just a rule name. +- R4 [P1] **Context gating.** A rule may require a second signal within a + configurable window: `exec` is interesting; `exec` on the same line as + `req.body` is much more so. This is the mechanism by which a pattern scanner + earns a `high` rather than a `medium`. +- R5 [P1] **Vendored and generated code is ranked down, not skipped.** A + vulnerability in `vendor/` still executes; it is simply less likely to be the + operator's to fix. `deps/` already covers third-party packages by version. +- R6 [P2] Additional languages: Python, PHP, Go, Ruby. + +### Rules (P0 set) + +- R7 [P0] **Code execution**: `eval`, `new Function`, `vm.runInThisContext`, + `child_process.exec`/`execSync` with interpolation. +- R8 [P0] **Injection**: SQL built by concatenation or template literal; + command strings assembled from variables. +- R9 [P0] **Unsafe rendering**: `dangerouslySetInnerHTML`, `innerHTML`, + `document.write` with a non-literal argument. +- R10 [P0] **Broken transport and crypto**: `rejectUnauthorized: false`, + `NODE_TLS_REJECT_UNAUTHORIZED = 0`, `md5`/`sha1` for passwords, + `Math.random()` used for tokens or ids. +- R11 [P1] **Path traversal**: `fs` calls whose argument derives from a request + field without normalisation. +- R12 [P1] **Deserialisation and prototype pollution**: unsafe YAML load, + `Object.assign` onto a prototype, merge helpers over user input. + +### Report + +- R13 [P0] **Confidence on every finding**: `pattern` (the construct exists), + `contextual` (a dangerous construct with an untrusted-looking source nearby). + Severity is a function of both rule severity and confidence — a `pattern` + finding may never exceed `medium`. +- R14 [P0] **Suppression comments**: an inline `threatcrush-disable-next-line + ` with a required reason. Suppressions are counted and reported, so a + codebase cannot quietly silence the scanner. +- R15 [P1] Dedupe per (file, rule, code fingerprint) so refactors that move a + line do not re-alert. +- R16 [P1] `threatcrush scan ./src --fail-on high` for CI use. + +## UX Notes + +Ships as the `sast` subsystem of `code-scanner`, configured under `sast_*`. + +```bash +threatcrush scan ./src # one-shot +threatcrush code-scanner sast rules # every rule, severity and CWE +threatcrush code-scanner sast explain js-eval-call +``` + +```toml +[code-scanner.sast] +enabled = true +languages = ["javascript", "typescript"] +vendored = "rank_down" +min_confidence = "pattern" # pattern | contextual +``` + +``` +[HIGH] code-scanner · sast · command injection risk + + /srv/app/routes/admin.ts:52 js-exec-interpolation (CWE-78) + exec(`convert ${req.body.file} out.png`) + + confidence: contextual — request data on the same line as a shell exec + A shell metacharacter in `file` runs as the server user. +``` + +Design constraints: + +- **Never claim proof from a pattern.** The word "vulnerability" is reserved for + `contextual` findings and above; a bare pattern match is a "risk" or a + "dangerous construct". +- **Show the line.** A SAST finding an operator cannot see is a finding they + cannot triage. Source excerpts are shown for this subsystem — unlike + `secrets`, where showing the match would leak the credential. +- **Say what happens if it is real**, not just which rule fired. + +## Success Metrics + +- **Zero findings** on a freshly scaffolded Express/Next.js app. The acceptance + test for staying switched on. +- **100% recall** on a fixture set containing one deliberate instance of every + P0 rule. +- **No finding above `medium`** carries only `pattern` confidence — asserted by + test, because this is the overclaiming failure mode and it must be structural + rather than a matter of care. +- Suppression count is reported on every scan; a scan reporting 0 findings and + 200 suppressions must not read as clean. +- 50k source files in <60s on one core. + +## Risks & Open Questions + +- **Overclaiming is the existential risk for this subsystem.** A regex is not + data-flow analysis, and a tool that blurs the two is worse than no tool: it + produces confident-sounding findings that waste triage time and, when + disproven, discredit the accurate findings alongside them. R13's confidence + levels and the metric asserting `pattern` findings stay ≤ medium are the + structural defences. **Open:** should `pattern`-only findings be off by + default, surfacing only on explicit request? +- **Vendored code will dominate raw counts.** `node_modules` is excluded by the + shared walk, but vendored directories, bundled assets and generated code are + not always distinguishable from first-party source. Ranking down (R5) helps; + misclassification will still happen. +- **Rule maintenance is unbounded.** Every framework has its own sinks. This is + precisely why Semgrep has a rule registry and a team. A small hand-maintained + set that is *honest about being small* is sustainable; ambition here is not. +- **Suppression comments will be abused.** Requiring a reason (R14) and counting + them is a mitigation, not a fix. **Open:** should a suppression expire, or + require a date? +- **Overlap with `secrets/` and `deps/`.** A hardcoded key found by a SAST rule + should be reported by `secrets`, and a vulnerable library by `deps`. Rules + here must stay out of both to avoid double-reporting the same issue in two + voices. diff --git a/prd/README.md b/prd/README.md index 1446612..438870a 100644 --- a/prd/README.md +++ b/prd/README.md @@ -16,3 +16,4 @@ these numbered PRDs cover individual changes to it. | [0001](./0001-detect-and-contain-balance-drain-attacks-on-third-party-services.md) | Detect and contain balance-drain attacks on third-party services | Draft | spend-guard, billing, fraud, sms-pumping, irsf, auto-recharge, containment, modules | | [0002](./0002-detect-vulnerable-and-malicious-dependencies-on-running-servers.md) | Detect vulnerable and malicious dependencies on running servers | Draft | code-scanner, deps, supply-chain, sbom, cve, osv, install-scripts, drift, modules | | [0003](./0003-detect-hardcoded-secrets-before-they-are-committed-or-served.md) | Detect hardcoded secrets before they are committed or served | Draft | code-scanner, secrets, credentials, entropy, redaction, modules | +| [0004](./0004-find-dangerous-code-patterns-without-pretending-to-be-a-compiler.md) | Find dangerous code patterns without pretending to be a compiler | Draft | code-scanner, sast, static-analysis, injection, taint, modules |