From ff9bd13c6f176e78851bf298ffcfdffd181ac09a Mon Sep 17 00:00:00 2001 From: Kjell Tore Guttormsen Date: Sat, 20 Jun 2026 09:14:11 +0200 Subject: [PATCH] docs: add full-depth plugin review (2026-06-20) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Grade B− — exceptionally disciplined design, but one CRITICAL zero-interaction RCE (F-1) plus two HIGH shell/path sinks. PUSH PARKED pending responsible-disclosure decision. Part of the marketplace-wide review (config-audit v5.4.0 + llm-security + structure + version). Read-only; this file is the only artifact. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ter3E2JSi1Khgmuf2kady8 --- docs/review-2026-06-20.md | 48 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 docs/review-2026-06-20.md diff --git a/docs/review-2026-06-20.md b/docs/review-2026-06-20.md new file mode 100644 index 0000000..547c739 --- /dev/null +++ b/docs/review-2026-06-20.md @@ -0,0 +1,48 @@ +# Plugin review — llm-security v7.7.2 (2026-06-20) + +> Full-depth review (part of the marketplace-wide sweep; pilot was okr). Tooling: config-audit +> v5.4.0 scanners (from source) + llm-security posture assessor + structure/version checks. +> Read-only; this file is the only artifact. + +## Verdict + +**Grade B− — exceptionally disciplined design undermined by one true zero-interaction RCE in a +default scanner.** At the design level this is one of the most security-conscious plugins in the +marketplace (untrusted targets treated as data behind an evidence-package boundary, least-privilege +agents, backup/dry-run-gated mutations, inert stdin-isolated fixtures, no eval/unsafe-deser). But +the scanner code does not hold itself to the standard it enforces on others: three shell/path sinks +trust untrusted strings, one of them a **critical** RCE. + +> ⚠️ **F-1 is a CRITICAL, zero-interaction RCE, reproduced end-to-end. Treat as fix-before-anything.** + +## Results by dimension + +| Dimension | Result | +|-----------|--------| +| config-audit posture | **A** (Conflicts B 75, Feature Coverage D 57) | +| config-audit plugin-health | **0 findings** | +| llm-security posture (self-meta) | **B−** — see findings. Meta-note: the reviewer distinguished deliberate research fixtures/blocklists from real defects and disproved 4 false positives (no prototype-pollution, no billion-laughs, no ReDoS, fixtures inert). | +| structure / hygiene | README ✓, CHANGELOG ✓, CLAUDE.md ✓, LICENSE ✓ | +| version consistency | **OK** (gate) | + +## Findings + +| ID | Severity | Location | Finding | +|----|----------|----------|---------| +| F-1 | **CRITICAL** | `scanners/git-forensics.mjs:59-66` (sinks :211,:223,:231,:564) | `git()` runs `execSync(\`git ${cmd}\`)` (shell string) with attacker-controlled filenames from the scanned repo interpolated in. `gitScan` is in the **default** SCANNERS array, and `/security scan ` clones a user-supplied repo then scans it. A hostile repo with a file named `commands/$(touch INJECTED).md` runs **arbitrary code on the analyst's machine with no install and no confirmation** (forensics runs outside the clone sandbox). Reproduced end-to-end. **Fix:** convert `git()` to `spawnSync('git', [...argArray], {cwd})` — every call site already has discrete tokens. | +| F-2 | High | `scanners/auto-cleaner.mjs:776,878-881` | `resolve(targetPath, f.file)` with no containment check; `f.file` from findings JSON (untrusted repo filenames, or an attacker-chosen `--findings` file). `file: "../../../.claude/settings.json"` writes outside the scanned tree. **Fix:** assert `absPath === targetPath || absPath.startsWith(targetPath + sep)` before write. | +| F-3 | High | `hooks/scripts/pre-install-supply-chain.mjs:254` → `supply-chain-data.mjs:221-227` | `execSafe(\`npm view ${spec} --json\`)` is `execSync` (shell). A spec like `foo;touch /tmp/X` survives parsing and reaches the shell, firing on **PreToolUse(Bash) before** the user's npm install — so it executes pre-confirmation even if the user then denies. **Fix:** `spawnSync('npm', ['view', spec, '--json'])` or validate spec `^[@/A-Za-z0-9._-]+$`. | +| F-4 | Low | `mcp-live-inspect.mjs:296` | Spawns the scanned target's declared MCP command (array-arg, no metachar injection). By-design for `/security mcp-inspect`, but scanning an untrusted target launches attacker-declared processes. Suggest a confirm before spawning servers from a non-home target. | +| F-5/F-6 | Low (hygiene) | repo root | Two stray committed artifacts: `--json` (0-byte redirect husk) and `.orphaned_at`. Inert; `git rm`. | + +**Cleared (so they aren't re-flagged):** malicious fixtures (stdin-isolated to subprocess, never +shell-executed), zip-extract (zip-slip + bomb guards), git-clone (hooksPath=/dev/null, +protocol.file.allow=never, array args, sandbox), vsix-fetch (HTTPS host allowlist), dep-auditor +(fixed strings), all 9 hooks (fail-open, advisory-only). Agents least-privilege. + +## Priority + +The three shell/path sinks (F-1/F-2/F-3) share one root cause — trusting untrusted strings at a +subprocess/filesystem sink — and all have small, localized `execSync→spawnSync(array)` / containment +fixes. Fixing them lifts the plugin from B− to a solid A−. **F-1 should be fixed before the next +`/security scan` of any untrusted repo URL.**