addyosmani / addyosmani/agent-skills

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

未關閉
#295 2 則留言 0 個 reaction 已指派 0 人 在 GitHub 檢視
主要語言
JavaScript
星號
93.8k
分支
10k
平均合併
3 天 15 小時
30 天內合併 PR
23

描述

## 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.*

貢獻指南

開啟貢獻指南

評估

這個 Issue 還沒有評估資料。

把新 issue 寄到你的電子郵件信箱

精選適合新手參與的 GitHub issue 摘要。