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 |