From 2d8c3a8d20882c98ef854285cdfde8ac5f7b7b67 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Tue, 11 Aug 2026 04:09:46 +0000 Subject: [PATCH] fix(scan): stop letting neighbouring code decide a finding's severity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Release 0.7.1. On ralyodio/debtap the same defect reported two different severities: 103 high contextual sh-insecure-transport-flag 113 medium pattern sh-plaintext-download 120 medium pattern sh-insecure-transport-flag Eight identical insecure-fetch findings, split by nothing more than distance from line 103. A --fail-on high gate would have caught five and let three through. Two causes, both fixed. An awk field reference was being read as a shell positional parameter. Line 103 contains gawk -F '=' '{print $2}', and the shell expands nothing inside single quotes, so that $2 is two characters, not untrusted input. It escalated confidence to contextual, and the six-line guard window carried the escalation to four neighbours. Single-quoted spans are now blanked before the untrusted-input test, for shell only. Separately, these rules should never have consulted context at all. Capping to medium without visible untrusted input is right for injection — exec(cmd) becomes a vulnerability once cmd can be influenced — and wrong where the construct is itself the defect. curl -k against HTTPS is interceptable whatever surrounds it; DES is broken in every file that uses it. Those rules are marked inherent and report at confidence evidence and their declared severity, the same treatment the credential rules already get: tls-verification-disabled, sh-remote-script-execution, sh-insecure-transport-flag, sh-plaintext-download, sh-world-writable-permissions, java-broken-cipher. Not a blanket escalation — context-dependent rules still cap, pinned by a test on js-unescaped-html-sink. debtap: same 8 findings, now uniformly high/evidence. capacitor: 26 findings with every severity unchanged. 123 tests, up from 118; each new one confirmed to fail with the fix reverted. Co-Authored-By: Claude Opus 5 --- apps/cli/package.json | 2 +- packages/scan/package.json | 2 +- .../scan/src/__tests__/code-rules.test.ts | 61 +++++++++++++++++++ packages/scan/src/code-rules.ts | 56 ++++++++++++++++- 4 files changed, 117 insertions(+), 4 deletions(-) diff --git a/apps/cli/package.json b/apps/cli/package.json index f3c4f24..5ef53bb 100644 --- a/apps/cli/package.json +++ b/apps/cli/package.json @@ -1,6 +1,6 @@ { "name": "@profullstack/threatcrush", - "version": "0.7.0", + "version": "0.7.1", "description": "All-in-one security agent daemon — monitor, detect, scan, and protect servers in real-time", "bin": { "threatcrush": "./dist/index.js" diff --git a/packages/scan/package.json b/packages/scan/package.json index 04073cb..1670cec 100644 --- a/packages/scan/package.json +++ b/packages/scan/package.json @@ -1,6 +1,6 @@ { "name": "@threatcrush/scan", - "version": "0.7.0", + "version": "0.7.1", "description": "ThreatCrush scan rules and engine, shared by the CLI, web, desktop and extension.", "license": "MIT", "type": "module", diff --git a/packages/scan/src/__tests__/code-rules.test.ts b/packages/scan/src/__tests__/code-rules.test.ts index 745c4c9..7d78268 100644 --- a/packages/scan/src/__tests__/code-rules.test.ts +++ b/packages/scan/src/__tests__/code-rules.test.ts @@ -446,6 +446,67 @@ describe('shell', () => { }); }); +/** + * Severity must not depend on what happens to sit near a finding. + * + * Both halves of this came out of one file. `ralyodio/debtap` reported the + * same `curl -k` defect as `high` on some lines and `medium` on others, + * decided by distance from a `gawk '{print $1}'` one-liner: the awk field + * reference was read as a shell positional parameter, and the guard window + * spread that escalation to its neighbours. + */ +describe('severity is not decided by neighbouring code', () => { + const findings = (path: string, source: string) => scanText(path, source); + + it('does not read an awk field reference as a shell positional parameter', () => { + // The shell expands nothing inside single quotes, so `$1` here is two + // characters. A rule that is *not* inherent must stay capped at medium. + const withAwk = [ + "version=$(curl -s https://x.invalid | gawk -F '=' '{print $2}' | gawk '{print $1}')", + 'eval "$cmd"', + ].join('\n'); + const evalFinding = findings('a.sh', withAwk).find((f) => f.ruleId === 'sh-eval-expansion'); + expect(evalFinding?.confidence).toBe('pattern'); + expect(evalFinding?.severity).toBe('medium'); + }); + + it('still escalates on a real positional parameter', () => { + // The exemption is for quoted text only — it must not become a way to hide + // genuine untrusted input. + const real = ['target="$1"', 'eval "$target"'].join('\n'); + const evalFinding = findings('a.sh', real).find((f) => f.ruleId === 'sh-eval-expansion'); + expect(evalFinding?.confidence).toBe('contextual'); + expect(evalFinding?.severity).toBe('high'); + }); + + it('reports an inherent rule at its declared severity either way', () => { + // `curl -k` against HTTPS is interceptable whatever surrounds it. + const bare = findings('a.sh', 'curl -k -L https://x.invalid/a.tgz > a.tgz')[0]; + const nearInput = findings( + 'a.sh', + ['target="$1"', 'curl -k -L https://x.invalid/a.tgz > a.tgz'].join('\n'), + ).find((f) => f.ruleId === 'sh-insecure-transport-flag'); + + expect(bare?.confidence).toBe('evidence'); + expect(bare?.severity).toBe('high'); + expect(nearInput?.severity).toBe(bare?.severity); + }); + + it('applies the same treatment to a broken cipher', () => { + const finding = findings('A.java', 'Cipher c = Cipher.getInstance("DES/CBC/PKCS5Padding");')[0]; + expect(finding?.confidence).toBe('evidence'); + expect(finding?.severity).toBe('high'); + }); + + it('leaves context-dependent rules capped, so this is not a blanket escalation', () => { + // `js-unescaped-html-sink` is not inherent: a static assignment says + // nothing about attacker data, and it must still cap at medium. + const finding = findings('a.js', 'el.innerHTML = "" + name + "";')[0]; + expect(finding?.confidence).toBe('pattern'); + expect(finding?.severity).toBe('medium'); + }); +}); + describe('php', () => { it('flags SQL built by interpolation and not a prepared statement', () => { expect(ruleIds('a.php', '$r = mysqli_query($db, "SELECT * FROM users WHERE id = $id");')).toContain( diff --git a/packages/scan/src/code-rules.ts b/packages/scan/src/code-rules.ts index af7958f..9d90569 100644 --- a/packages/scan/src/code-rules.ts +++ b/packages/scan/src/code-rules.ts @@ -56,6 +56,24 @@ export interface CodeRule { * input is visible nearby. */ needsContext?: boolean; + /** + * The construct *is* the defect, so its severity does not depend on context. + * + * The default model caps a finding at medium unless untrusted input is + * visible nearby, which is right for injection: `exec(cmd)` is only a + * vulnerability once `cmd` can be influenced. It is wrong for a whole class + * of rules where nothing nearby changes the answer. `curl -k` against HTTPS + * is a machine-in-the-middle hole whether or not a positional parameter + * appears six lines above it; DES is broken in every file that uses it. + * + * Marking those rules `inherent` reports them at confidence `evidence` and + * at their declared severity, the same treatment the credential rules get + * for the same reason — a committed AWS key is a committed AWS key. + * + * Do not reach for this to make a rule look important. It is for rules whose + * finding text would be identical no matter what surrounds the line. + */ + inherent?: boolean; /** * Extra evidence that must appear in the guard window for the rule to fire. * Used where the dangerous part is the *combination* — a base64 blob is @@ -627,6 +645,7 @@ export const CODE_RULES: readonly CodeRule[] = [ }, { id: 'tls-verification-disabled', + inherent: true, title: 'TLS certificate verification disabled', consequence: 'Every connection made this way is trivially interceptable; the encryption is decorative.', @@ -737,6 +756,7 @@ export const CODE_RULES: readonly CodeRule[] = [ // of privileged work actually happens, and they run as whoever invoked them. { id: 'sh-remote-script-execution', + inherent: true, 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.', @@ -796,6 +816,7 @@ export const CODE_RULES: readonly CodeRule[] = [ }, { id: 'sh-insecure-transport-flag', + inherent: true, 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.', @@ -812,6 +833,7 @@ export const CODE_RULES: readonly CodeRule[] = [ }, { id: 'sh-plaintext-download', + inherent: true, 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.', @@ -825,6 +847,7 @@ export const CODE_RULES: readonly CodeRule[] = [ }, { id: 'sh-world-writable-permissions', + inherent: true, 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.', @@ -1012,6 +1035,7 @@ export const CODE_RULES: readonly CodeRule[] = [ }, { id: 'java-broken-cipher', + inherent: true, title: 'broken cipher or ECB mode', consequence: 'DES, RC2, RC4 and Blowfish are broken or too small to rely on. ECB encrypts identical plaintext blocks to identical ciphertext blocks, so structure in the data survives encryption and is readable straight off the ciphertext.', @@ -1215,6 +1239,25 @@ export interface RuleMatch { * Returns `null` when the rule does not apply, does not match, is guarded, or * needs context it cannot see. */ +/** + * Blank out single-quoted spans before looking for untrusted input. + * + * The shell performs no expansion inside single quotes, so a `$1` there is the + * two characters `$1` and never a positional parameter. Without this, an awk + * or sed program written inline — `gawk -F '=' '{print $2}'` — reads as + * attacker-controlled input. + * + * That was not theoretical. In `ralyodio/debtap` it escalated one `curl -k` + * line to `contextual`, and the ±6-line window carried the escalation to four + * neighbouring findings, so the same defect reported `high` on lines 103–111 + * and `medium` on 113, 120 and 128 — decided entirely by distance from an awk + * one-liner. A `--fail-on high` gate would have caught five of eight identical + * problems. + */ +function withoutSingleQuoted(text: string): string { + return text.replace(/'[^'\n]*'/g, "''"); +} + export function evaluateRule(rule: CodeRule, ctx: MatchContext): RuleMatch | null { if (rule.languages && !rule.languages.includes(ctx.language)) return null; @@ -1238,9 +1281,18 @@ export function evaluateRule(rule: CodeRule, ctx: MatchContext): RuleMatch | nul if (guard && (guard.test(line) || guard.test(context))) return null; const untrusted = untrustedPatternFor(ctx.language); - const contextual = untrusted.test(line) || untrusted.test(context); + const probeLine = ctx.language === 'shell' ? withoutSingleQuoted(line) : line; + const probeContext = ctx.language === 'shell' ? withoutSingleQuoted(context) : context; + const contextual = untrusted.test(probeLine) || untrusted.test(probeContext); if (rule.needsContext && !contextual) return null; - const confidence: Confidence = contextual ? 'contextual' : 'pattern'; + // `inherent` short-circuits the whole context question. See the field's + // documentation: for these rules the construct is the defect, so nearby + // input cannot make it worse and its absence cannot make it better. + const confidence: Confidence = rule.inherent + ? 'evidence' + : contextual + ? 'contextual' + : 'pattern'; return { rule, confidence, severity: severityFor(rule.severity, confidence) }; }