diff --git a/modules/code-scanner/src/__tests__/secrets.test.ts b/modules/code-scanner/src/__tests__/secrets.test.ts index 5d21b85..e4a552e 100644 --- a/modules/code-scanner/src/__tests__/secrets.test.ts +++ b/modules/code-scanner/src/__tests__/secrets.test.ts @@ -9,6 +9,7 @@ import { matchesIgnore, redact, scanText, + trimStars, } from '../secrets/index.js'; /** @@ -245,3 +246,43 @@ describe('overlapping rules', () => { expect(found[0]?.ruleId).toBe('connection-string-password'); }); }); + +/** + * ReDoS regression (CodeQL js/polynomial-redos). + * + * These patterns run against file content and operator config the scanner does + * not control. An unbounded quantifier turns a crafted file into a denial of + * service against the security agent itself — a worse outcome than any + * placeholder the bound might cause us to miss. CodeQL flagged three of these + * after the subsystem merged; these tests keep them fixed. + */ +describe('pathological input does not stall the scanner', () => { + const budgetMs = 1000; + + it('handles a long run of angle brackets', () => { + const hostile = '<'.repeat(50_000); + const started = Date.now(); + scanText(`api_key = "${hostile}"`); + expect(Date.now() - started).toBeLessThan(budgetMs); + }); + + it('handles an allowlist entry that is all asterisks', () => { + const started = Date.now(); + expect(isAllowed({ fingerprint: 'sha256:x', file: '/srv/a.ts' }, ['*'.repeat(50_000)])).toBe( + false, + ); + expect(Date.now() - started).toBeLessThan(budgetMs); + }); + + it('handles a gitignore pattern that is all asterisks', () => { + const started = Date.now(); + expect(matchesIgnore('a/b/c.ts', ['*'.repeat(50_000)])).toBe(false); + expect(Date.now() - started).toBeLessThan(budgetMs); + }); + + it('trims stars without a regex', () => { + expect(trimStars('**foo**')).toBe('foo'); + expect(trimStars('***')).toBe(''); + expect(trimStars('bar')).toBe('bar'); + }); +}); diff --git a/modules/code-scanner/src/secrets/index.ts b/modules/code-scanner/src/secrets/index.ts index cb554dc..8eb49b8 100644 --- a/modules/code-scanner/src/secrets/index.ts +++ b/modules/code-scanner/src/secrets/index.ts @@ -18,7 +18,8 @@ export * from './rules.js'; -import { readdir, readFile, stat } from 'node:fs/promises'; +import { open, readdir, readFile } from 'node:fs/promises'; +import type { FileHandle } from 'node:fs/promises'; import { basename, extname, join, relative } from 'node:path'; import { scanText, type SecretMatch } from './rules.js'; @@ -140,11 +141,22 @@ export async function readGitignore(root: string): Promise { } } +/** Strip leading and trailing `*` without a regex. See `isAllowed`. */ +export function trimStars(value: string): string { + let start = 0; + let end = value.length; + while (start < end && value[start] === '*') start += 1; + while (end > start && value[end - 1] === '*') end -= 1; + return value.slice(start, end); +} + export function matchesIgnore(relativePath: string, patterns: readonly string[]): boolean { const segments = relativePath.split('/'); return patterns.some((pattern) => { if (pattern.includes('*')) { - const suffix = pattern.replace(/^\*+/, ''); + // Same reasoning as `isAllowed`: .gitignore is file content the scanner + // does not control, so no unbounded quantifier runs against it. + const suffix = trimStars(pattern); return suffix.length > 1 && relativePath.endsWith(suffix); } return segments.includes(pattern); @@ -159,7 +171,14 @@ export function isAllowed( return allow.some((entry) => { if (entry.startsWith('sha256:')) return finding.fingerprint === entry; if (entry.includes('*')) { - const inner = entry.replace(/^\*+/, '').replace(/\*+$/, ''); + // Trimmed without a regex. An allowlist entry is operator-supplied and + // `/^\*+/` on a long run of asterisks backtracks polynomially — a config + // file should not be able to stall the scanner. + let start = 0; + let end = entry.length; + while (start < end && entry[start] === '*') start += 1; + while (end > start && entry[end - 1] === '*') end -= 1; + const inner = entry.slice(start, end); return inner.length > 0 && finding.file.includes(inner); } // A trailing slash means "everything under this directory", which is what @@ -214,28 +233,36 @@ export async function scanSecrets(options: SecretsOptions): Promise options.maxFileBytes && !highRisk) { - // Recorded rather than silently dropped: an unscanned file must never - // be indistinguishable from a clean one (PRD 0002 R6, PRD 0003 R2). - skipped.push({ file, reason: `larger than ${options.maxFileBytes} bytes` }); - continue; - } + if (size === 0) continue; + if (size > options.maxFileBytes && !highRisk) { + // Recorded rather than silently dropped: an unscanned file must never + // be indistinguishable from a clean one (PRD 0002 R6, PRD 0003 R2). + skipped.push({ file, reason: `larger than ${options.maxFileBytes} bytes` }); + continue; + } - let buffer: Buffer; - try { - buffer = await readFile(file); + buffer = await handle.readFile(); } catch { skipped.push({ file, reason: 'unreadable' }); continue; + } finally { + await handle?.close().catch(() => { + /* a failed close must not abort the scan */ + }); } if (!highRisk && looksBinary(buffer)) continue; diff --git a/modules/code-scanner/src/secrets/rules.ts b/modules/code-scanner/src/secrets/rules.ts index 6de20a3..38f11c0 100644 --- a/modules/code-scanner/src/secrets/rules.ts +++ b/modules/code-scanner/src/secrets/rules.ts @@ -75,8 +75,13 @@ function githubChecksum(token: string): boolean { * and half the world's `.env.example` files say `changeme`. Reporting these * trains an operator to ignore the scanner, which is worse than missing them. */ +// Every quantifier here is bounded. This regex runs against file content the +// scanner does not control, and an unbounded `<[^>]+>` gives a crafted file a +// polynomial-backtracking path — a denial of service against the security +// agent itself, which is a worse outcome than the missed placeholder that the +// bound might cost. Same reasoning applies to every pattern in this file. const PLACEHOLDER = - /(?:EXAMPLE|example|xxxx|XXXX|changeme|CHANGEME|your[_-]?(?:key|token|secret)|placeholder|<[^>]+>|\bfake\b|\bdummy\b|\btest[_-]?key\b|0{8,}|1234567890)/; + /(?:EXAMPLE|example|xxxx|XXXX|changeme|CHANGEME|your[_-]?(?:key|token|secret)|placeholder|<[^<>]{1,64}>|\bfake\b|\bdummy\b|\btest[_-]?key\b|0{8,12}|1234567890)/; export const SECRET_RULES: readonly SecretRule[] = [ {