feat(scan): scan shebang scripts, and give shell a rule set - #87
Merged
ralyodio merged 1 commit intoAug 10, 2026
Merged
Conversation
`shell` was a language the type system knew about and no rule targeted, so a repository written entirely in bash got secret detection and nothing else. Worse, language detection was extension-only: an executable named for the command it provides rather than the language it is written in was never opened at all. Both together meant `ralyodio/debtap` — 3,511 lines of bash in a file called `debtap` — scanned clean by scanning nothing, and reported success while doing it. - Read the interpreter from a `#!` line for extensionless files, sniffed from a 128-byte prefix so a checked-in blob costs one small read rather than a megabyte decoded and discarded. Unrecognised extensions are still skipped; `.png` is not a script. - Seven shell rules: remote script execution, eval on an expansion, unquoted expansion in a recursive remove, disabled certificate verification, plain-HTTP download, world-writable permissions and predictable temp paths. Each rule is built against the corrected shape as well as the vulnerable one. The eval rule matches eval's argument rather than the whole line, because `\beval\b.*\$` counts bash's dynamic-range idiom — where every expansion is arithmetic and cannot carry a command — and reported it 355 times in debtap alone. On debtap: 0 findings (nothing scanned) to 8, all genuine — `curl -k` against HTTPS and plain-HTTP fetches whose payloads build packages. Capacitor is unchanged at 13. 83 tests pass, up from 68. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * line of every installer. | ||
| */ | ||
| const UNTRUSTED_SH = | ||
| /\$\{?[1-9]\d*\b|\$[@*]|\$\{@\}|\bread\s+(?:-\S+\s+)*[A-Za-z_]\w*|\$\{?REPLY\b|\$\{?QUERY_STRING\b/; |
| languages: ['shell'], | ||
| // The pipe must be the *next* thing: `curl -o f url && sh f` is a different | ||
| // (and checkable) shape, and `curl url | jq` is not an execution at all. | ||
| pattern: /\b(?:curl|wget)\b[^|\n]*\|\s*(?:sudo\s+(?:-\S+\s+)*)?(?:\/bin\/|\/usr\/bin\/)?(?:ba|da|k|z|a)?sh\b/, |
| // actually re-parses untrusted text as source. `eval echo $x` is not | ||
| // covered; catching it without also catching the range idiom needs to know | ||
| // which expansions are arithmetic, which is parsing, not matching. | ||
| pattern: /\beval\s+(?:-\S+\s+)*(?:"\s*)?\$(?:\{?[A-Za-z_]\w*|\((?!\())/, |
| // never reaches the `$` and never matches. Only a genuinely bare expansion | ||
| // does. Restricted to recursive/forced removal: a bare `$f` in `rm $f` is | ||
| // sloppy, but it is not the shape that erases a filesystem. | ||
| pattern: /\brm\s+(?:-[a-zA-Z-]*[rRf][a-zA-Z-]*\s+)+[^"'\n]*?\$\{?[A-Za-z_]/, |
| cwe: 'CWE-732', | ||
| severity: 'medium', | ||
| languages: ['shell'], | ||
| pattern: /\bchmod\s+(?:-[a-zA-Z-]+\s+)*(?:0?777|a\+rwx|ugo\+rwx|a=rwx)\b/, |
| // Redirection or an explicit write into a literal `/tmp` path. A `$$` or | ||
| // `$RANDOM` suffix is still predictable, so it is not treated as a fix; | ||
| // `mktemp` is, and it is the guard below. | ||
| pattern: /(?:>{1,2}\s*|\b(?:tee|touch|cp|mv|install)\s+(?:-\S+\s+)*)\/tmp\/[\w.$-]+/, |
ThreatCrush Security Scan150 finding(s) HIGH/CRITICAL: 12 | MEDIUM: 102 | LOW: 36
…and 100 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
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.
Stacked on #86 — review #85 and #86 first; this PR's diff is the third commit.
The bug
Two gaps that compound into a scanner reporting success on a repository it never read.
1.
shellhad no rules. It is a language inScanLanguage, mapped from.sh/.bash/.zsh, and zero of the 32 code rules targeted it. A bash codebase got secret detection and nothing else.2. Language detection was extension-only.
languageOf()readsextname()and nothing else, so a file with no extension was skipped before it was opened.Executables are routinely named for the command they provide rather than the language they are written in —
debtap,configure,gradlew. Onralyodio/debtap, whose entire source is 3,511 lines of bash in a file calleddebtap:Pointed straight at the file, it is unambiguous:
A clean scan that scanned nothing is worse than no scan — it is a green check that says the opposite of the truth.
The fix
Shebang detection. For files with no extension, read a 128-byte prefix and take the language from the
#!line. Files with an unrecognised extension are still skipped —.pngis not a script, and sniffing every file would mean reading the whole tree. The prefix read matters: an extensionless blob costs one 128-byte read rather than a megabyte decoded as UTF-8 and thrown away.Seven shell rules, drawn from the classes that actually appear in installers and packaging scripts:
sh-remote-script-executioncurl … | bashsh-eval-expansioneval "$cmd"sh-unquoted-expansion-destructiverm -rf $DIR/…sh-insecure-transport-flagcurl -k,--no-check-certificatesh-plaintext-downloadcurl http://…sh-world-writable-permissionschmod 777sh-predictable-temp-path/tmp/name, unlessmktempis in the windowEvery rule is built against the corrected shape as well as the vulnerable one, per this file's existing contract.
rm -rf "$DIR"does not match because[^"'\n]*?cannot cross the quote;curl … | jqdoes not match because the pipe target must be a shell;chmod 0755does not match.The eval rule is matched against eval's argument, not the line
The obvious spelling,
\beval\b.*\$, is wrong. Bash's ordinary way to build a numeric range isEvery expansion there sits inside
$((…)), which the shell parses as an arithmetic expression — a;in it is a syntax error, not a second command. A line-wide search still finds$kinside the arithmetic and fires. That spelling produced 355 findings in debtap alone, all of them this one safe loop.Anchoring to the argument (
eval "$cmd",eval $cmd,eval "$(…)") keeps the shape that genuinely re-parses text as source.eval echo $xis not covered, and the comment says so: separating it from the range idiom needs to know which expansions are arithmetic, which is parsing rather than matching.Also handled: the documented shell-init idiom
eval "$(pyenv init -)"is guarded, and-kon a plain-HTTP URL no longer double-reports — there is no certificate to skip, sosh-plaintext-downloadis the finding that fits.Verification
ralyodio/debtap: 0 → 8 findings, all genuine.Lines 120 and 128 are the sharp ones: an archive and a package list fetched over HTTPS with verification disabled, written to
/var/cache/debtap/, and used to build packages. Anyone able to intercept that connection chooses what gets packaged.Intermediate tuning, all measured on debtap: 368 → 13 (eval anchored to its argument) → 8 (plain-HTTP double-report removed).
ionic-team/capacitorat5e5bb3b: unchanged at 13. No shell rule fires on it and shebang detection adds no files, so this stacks cleanly on #85 and #86.83 tests pass, up from 68. 15 added — each shell rule has a positive case and the corrected shape beside it, plus the brace-range idiom, the shell-init idiom, loopback HTTP, and a check that shell rules do not leak into a JS file that merely contains the same words.
tsc --noEmitclean.