addyosmani / addyosmani/agent-skills

Security: SSRF, cache poisoning & symlink overwrite in opt-in lifecycle hooks (sdd-cache, simplify-ignore)

Abierto
#295 2 comentarios 0 reacciones 0 asignados Ver en GitHub
Lenguaje dominante
JavaScript
Estrellas
93.8k
Forks
10k
Merge medio
3 d 15 h
PR fusionados (30 d)
23

Descripción

## Summary

While evaluating the lifecycle hooks in `hooks/`, a multi-pass review (correctness, security, and test-coverage) surfaced several security and data-integrity issues in the **opt-in** hooks — `sdd-cache-pre.sh`, `sdd-cache-post.sh`, and `simplify-ignore.sh`.

These hooks are **not wired by default** (`hooks/hooks.json` only registers `session-start.sh`, which is fine), so this affects users who manually enable the WebFetch cache or the simplify-ignore feature per `SDD-CACHE.md` / `SIMPLIFY-IGNORE.md`. Filing publicly since there's no `SECURITY.md` / private channel. Happy to convert to a private report if you'd prefer.

`session-start.sh` reviewed clean (trusted local input, correct `jq --arg` escaping).

## Findings

| # | Severity | File | Class | Issue |
|---|----------|------|-------|-------|
| 1 | Critical | `sdd-cache-pre.sh:71-74`, `sdd-cache-post.sh:82` | SSRF | `curl -L "$URL"` with no scheme/host allowlist, following redirects, fires automatically on every WebFetch |
| 2 | High | `sdd-cache-post.sh:105-128`, `sdd-cache-pre.sh:62-106` | Cache poisoning / prompt injection | No TTL cap; any `304` serves cached body forever as authoritative "unchanged" content |
| 3 | High | `sdd-cache-post.sh:75-77`, `sdd-cache-pre.sh:50-51` | Tamperable cache | Predictable filenames (`sha256(URL)`), default perms, in-tree `.claude/sdd-cache/`; a committed/crafted cache entry is trusted on next 304 |
| 4 | High | `simplify-ignore.sh:153-154,202,209,287,291` | Symlink overwrite / TOCTOU | Backup/restore writes via `cat > "$target"` on an untrusted `file_path` with no symlink check; arbitrary-file-overwrite primitive |
| 5 | Critical (integrity) | `simplify-ignore.sh` filter/expand path | Data loss | Non-atomic in-place rewrite under `set -e`; an abort (e.g. missing `perl`, full disk) can leave the real file holding placeholders after the backup was overwritten |

### 1. SSRF via unvalidated `curl` (Critical)
`URL` comes verbatim from `.tool_input.url` and is passed to `curl ... -L` with no scheme/host validation and redirect-following enabled. A WebFetch to an attacker-influenced link (e.g. via prompt injection in a fetched page) can 302-redirect the hook's automatic out-of-band request to internal targets — cloud metadata (`169.254.169.254`), `localhost` admin ports, etc.

**Highest-value single fix:** drop `-L` (or re-validate the host on every hop), require `https://`, and reject internal/metadata hosts before any `curl`.

### 2. Durable cache poisoning (High)
`fetched_at` is stored but never enforced as a max age. Once an entry is cached, any subsequent `304 Not Modified` causes the stored body to be served to the agent verbatim and labeled "Revalidated … unchanged" — with no TTL. A malicious or compromised origin (or MITM on a non-HTTPS URL via #1) can pin injected content that persists across sessions.

**Fix:** enforce a TTL cap before serving from cache regardless of 304; optionally store + verify a content hash.

### 3. Tamperable / disclosing cache (High)
Cache files are written to `${CLAUDE_PROJECT_DIR:-$PWD}/.claude/sdd-cache/.json` with default umask and no integrity protection; the filename is `sha256(URL)`, fully predictable. A crafted entry (even one committed to a repo) is trusted and served on the next matching WebFetch + 304. Fetched bodies (possibly authenticated/internal content) are persisted in plaintext inside the working tree.

**Fix:** `chmod 700` the cache dir / `umask 077`; add `.claude/sdd-cache/` to `.gitignore` guidance; don't trust pre-existing entries without an integrity check.

### 4. Symlink/arbitrary-file overwrite (High)
`file_path` (untrusted) drives backup-and-rewrite on Read and write-back on Edit/Write/Stop via `cat > "$orig"`, which follows symlinks. There's no `[ -L ]` check and the path→file mapping is a 16-char hash with no inode re-verification, opening a session-long TOCTOU window. This is an arbitrary-overwrite primitive against any user-writable file (`~/.bashrc`, etc.).

**Fix:** `[ -L "$FILE_PATH" ] && exit 0` at entry; re-verify the target is a regular non-symlink (by inode) before every write-back; write via temp + `mv -T`.

### 5. Non-atomic rewrite under `set -e` (Critical integrity / data loss)
`simplify-ignore.sh` rewrites the user's real file in place across multiple hook events. With `set -euo pipefail`, an unguarded failure inside `filter_file` (e.g. `perl` absent on a minimal Git Bash — line 137/269 hard-depend on `perl` though only `jq`/`shasum` are declared; or a full/read-only cache dir) can abort after `: > "$dest"` and after the backup was overwritten, leaving the on-disk file full of `BLOCK_` placeholders and no clean backup.

**Fix:** declare `perl` as a dependency and check up front (or replace with pure-shell newline handling); `trap`-guard the rewrite; make the final swap atomic (`mv` temp into place).

## Additional notes
- `simplify-ignore.sh` file identity is `sha(path)|cut -c1-16` with no path normalization — the same file as `C:\…` vs `/c/…` can be filtered under two IDs, and Stop can restore stale content over edits. Normalize the path (e.g. `cygpath`/`realpath`) before hashing.
- `BLOCK_` placeholder expansion uses global substring replace with fuzzy/last-resort fallbacks; protected content that itself contains a `BLOCK_`-shaped string can be corrupted on expand. Consider a sentinel that cannot occur in source.
- The two `sdd-cache` scripts have **no tests**; the data-loss-critical expand/restore half of `simplify-ignore.sh` is also untested. Highest-value missing test: round-trip fidelity (filter → expand → original bytes recovered).

## Suggested minimum changes to make the opt-in hooks safe to enable
1. `sdd-cache-*`: require `https://`, drop `-L` (or re-validate host per hop), block internal/metadata hosts. *(fixes #1)*
2. `sdd-cache-*`: enforce a TTL cap before serving cache; `chmod 700` the cache dir; don't trust pre-existing entries. *(fixes #2, #3)*
3. `simplify-ignore.sh`: reject symlinks and re-verify a regular-file target before write-back; atomic, `trap`-guarded rewrite; declare/​check `perl`. *(fixes #4, #5)*

---
*Methodology: findings independently corroborated across three review passes. Glad to follow up with PRs for any of the above.*

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.