fix(scan): stop letting neighbouring code decide a finding's severity (v0.7.1) - #97
Merged
Conversation
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 <noreply@anthropic.com>
ThreatCrush Security Scan157 finding(s) HIGH/CRITICAL: 14 | MEDIUM: 107 | LOW: 36
…and 107 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 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 = "<b>" + name + "</b>";')[0]; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release 0.7.1. Fixes the severity inconsistency I flagged on #95.
The symptom
Eight findings on
ralyodio/debtap, all the same class — a package source fetched without a trustworthy channel — reported at two different severities:Nothing distinguishes line 113 from line 111 except distance from line 103. A
--fail-on highgate would have caught five of eight identical problems and let three through — which is worse than catching none, because it looks like it worked.Cause 1: an awk program read as untrusted input
Line 103 is:
version=$(curl -k -s https://packages.ubuntu.com | gawk -F '=' '{print $2}' | gawk '{print $1}' | ...)$2and$1there are awk field references inside single-quoted programs. The shell performs no expansion inside single quotes, so they are literally the characters$2and$1— but the shell untrusted-input pattern matched them as positional parameters. That escalated line 103 tocontextual, and the ±6-line guard window spread the escalation to 104–111.Single-quoted spans are now blanked before the untrusted-input test, for shell only.
target="$1"— a real positional parameter — still escalates, which is pinned by its own test.Cause 2: these rules should never have asked
The confidence cap exists for injection:
exec(cmd)is a construct, and only becomes a vulnerability oncecmdcan be influenced, so capping it at medium without visible untrusted input is correct and valuable.It is wrong wherever the construct is the defect.
curl -kagainst HTTPS is interceptable whatever surrounds it. DES is broken in every file that uses it. Nothing six lines away changes the finding text, so nothing six lines away should change its severity.Those rules now carry
inherent, reporting at confidenceevidenceand their declared severity — the same treatment the credential rules already get, and for the same stated reason: a committed AWS key is a committed AWS key.tls-verification-disabledsh-insecure-transport-flagsh-remote-script-executionsh-plaintext-downloadsh-world-writable-permissionsjava-broken-cipherThis is not a blanket escalation. Context-dependent rules still cap, and there is a test on
js-unescaped-html-sinkasserting exactly that, so the field cannot quietly become a way to make a rule look important.Result
Same eight findings — none added, none lost — now uniform.
Verification
high/evidence.inherentremoved → both inherent cases fail.tsc --noEmitclean.