From fd1a1e229bac0a02135405a44b9965e10659e3d0 Mon Sep 17 00:00:00 2001 From: Mickael Farina Date: Sun, 17 May 2026 18:50:08 +0200 Subject: [PATCH] docs(audits): record PR-1D merge commit hash in D-5/D-14/D-16 closures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-1D (#47) merged to main as squash commit fd2b460. Update the closure footnotes for D-5, D-14, and D-16 in docs/audits/PHASE-1-SECURITY.md plus the D-5 row in docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md, replacing the branch-name placeholders with PR number + commit hash. Mirrors the citation style applied to: D-1 PR-1A #42 → 48ec5d5 D-2/3 PR-1B #43 → ff16664 D-4 PR-1C #45 → 0065d90 D-5 PR-1D #47 → fd2b460 (this commit) After this lands, Wave 1 is fully closed with complete citation trails. All five CRITICAL skill-loading + write-path findings (D-1, D-2, D-3, D-4, D-5) plus the two bonus mediums (D-14, D-16) carry merge commit hashes in their audit-doc footnotes. Co-Authored-By: Claude Opus 4.7 --- docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md | 2 +- docs/audits/PHASE-1-SECURITY.md | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md b/docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md index bdf937a..03a5e29 100644 --- a/docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md +++ b/docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md @@ -147,7 +147,7 @@ If you do all 8: you have layered defenses against D-1 / D-2 / D-3 / D-4 / D-7 u | **D-2** | Security | `/api/forge` fetches arbitrary URL → LLM → writes skill, no review gate | **W1 — CLOSED ([#43](https://github.com/AVADSA25/codec/pull/43), `ff16664`)** | | **D-3** | Security | `/api/save_skill` writes directly to skills/ with only substring check | **W1 — CLOSED ([#43](https://github.com/AVADSA25/codec/pull/43), `ff16664`)** | | **D-4** | Security | `file_write` skill (MCP-exposed) can write to `~/.codec/skills/` | **W1 — CLOSED ([#45](https://github.com/AVADSA25/codec/pull/45), `0065d90`)** | -| **D-5** | Security | `permission_gate` accepts path-traversal via `fnmatch` (no realpath) | **W1 — CLOSED (PR-1D)** | +| **D-5** | Security | `permission_gate` accepts path-traversal via `fnmatch` (no realpath) | **W1 — CLOSED ([#47](https://github.com/AVADSA25/codec/pull/47), `fd2b460`)** | | C-1 | Reliability | `codec.py` daemon ignores SIGINT/SIGTERM; leaks sox + tkinter on every restart | W4 | | C-2 | Reliability | `~/.codec/pwa_response.json` race conditions + no correlation_id | W4 | | C-3 | Reliability | `notifications.json` has 3 writers with 3 different write semantics | W4 | diff --git a/docs/audits/PHASE-1-SECURITY.md b/docs/audits/PHASE-1-SECURITY.md index 5ab939d..dc3c551 100644 --- a/docs/audits/PHASE-1-SECURITY.md +++ b/docs/audits/PHASE-1-SECURITY.md @@ -123,7 +123,7 @@ The skill is `SKILL_MCP_EXPOSE = True` and NOT in `_HTTP_BLOCKED` (`codec_config **Location:** `codec_agent_runner.py:95-142` (`permission_gate`) **CWE / OWASP:** CWE-22 Path Traversal / CWE-23 Relative Path Traversal / Agentic A06 Excessive Agency -> **Closed by PR-1D** (branch `fix/pr1d-permission-gate-realpath`). `permission_gate` was refactored: (1) reject `..` segments outright before any matching, (2) `os.path.realpath` both sides of every comparison, (3) `fnmatch` replaced with `action_real.startswith(grant_real + os.sep)`. New helper `_path_allowed` consolidates the logic for both `touches_path` and `reads_path`. Rejections emit `permission_gate_blocked` audit events (source=`codec-agent-runner`, outcome=`error`, level=`warning`, extra={`requested_path`, `resolved_path`, `reason`, `agent_id`}) before raising `PermissionViolation`. 5 new tests in `tests/test_agent_runner.py` cover dotdot rejection (write + read paths), symlink-outside-grant rejection, symlink-within-grant acceptance, and audit emission. All 7 pre-existing permission_gate tests still pass. +> **Closed by PR-1D** ([#47](https://github.com/AVADSA25/codec/pull/47), merged as `fd2b460`). `permission_gate` was refactored: (1) reject `..` segments outright before any matching, (2) `os.path.realpath` both sides of every comparison, (3) `fnmatch` replaced with `action_real.startswith(grant_real + os.sep)`. New helper `_path_allowed` consolidates the logic for both `touches_path` and `reads_path`. Rejections emit `permission_gate_blocked` audit events (source=`codec-agent-runner`, outcome=`error`, level=`warning`, extra={`requested_path`, `resolved_path`, `reason`, `agent_id`}) before raising `PermissionViolation`. 5 new tests in `tests/test_agent_runner.py` cover dotdot rejection (write + read paths), symlink-outside-grant rejection, symlink-within-grant acceptance, and audit emission. All 7 pre-existing permission_gate tests still pass. **Description:** The Step 9 permission gate checks `action.path` via `fnmatch.fnmatch(os.path.expanduser(action.path), os.path.expanduser(p))`. `expanduser` resolves `~`, but does NOT resolve `..` or symlinks. `fnmatch` is glob-style and does NOT treat `..` specially. So: - Plan grant: `~/Documents/**` - Action path: `~/Documents/../../etc/passwd` @@ -271,7 +271,7 @@ The skill is `SKILL_MCP_EXPOSE = True` and not in `_HTTP_BLOCKED`, so an attacke **Location:** `codec_agent_plan.py:754-759` **CWE / OWASP:** CWE-20 Improper Input Validation / Agentic A06 Excessive Agency -> **Closed by PR-1D** (branch `fix/pr1d-permission-gate-realpath`). `_PATH_BLOCKLIST_SUBSTRINGS` extended with 8 new entries: `/.codec/skills`, `/.codec/plugins`, `/.codec/oauth_state.json`, `/.codec/audit.log`, `/.codec/agents`, `/.codec/agent_global_grants.json`, `/.codec/config.json`, `/.codec/memory.db`. The LLM-drafted plan auto-extract path now drops any user-typed path landing in these directories — chain to D-1 RCE via auto-granted plan write_paths is closed. 7 parametrized tests in `tests/test_agent_plan.py` cover each new entry. +> **Closed by PR-1D** ([#47](https://github.com/AVADSA25/codec/pull/47), merged as `fd2b460`). `_PATH_BLOCKLIST_SUBSTRINGS` extended with 8 new entries: `/.codec/skills`, `/.codec/plugins`, `/.codec/oauth_state.json`, `/.codec/audit.log`, `/.codec/agents`, `/.codec/agent_global_grants.json`, `/.codec/config.json`, `/.codec/memory.db`. The LLM-drafted plan auto-extract path now drops any user-typed path landing in these directories — chain to D-1 RCE via auto-granted plan write_paths is closed. 7 parametrized tests in `tests/test_agent_plan.py` cover each new entry. **Description:** When the user types a path in their project description (e.g. "save outputs to ~/Documents/foo"), `extract_user_paths` auto-grants the path in the manifest. The blocklist excludes `/.ssh`, `/.aws`, `/Library/Keychains`, `/etc/`, `/var/`, etc., AND `/.codec/secrets`, `/.codec/auth`. But it does NOT exclude `/.codec/skills`, `/.codec/plugins`, `/.codec/oauth_state.json`, or the repo's `skills/` directory. An LLM-drafted plan based on a user's vague description that mentions `~/.codec/skills/util/` would auto-grant write to that path → next agent run can drop a skill → D-1. **Exploit chain:** User says "build me a skill that does X, save it to ~/.codec/skills/x_skill.py". Plan auto-grants `~/.codec/skills/x_skill.py/**`. Agent writes payload. Restart → RCE. **Impact:** Bypass of the skill review gate via the auto-extract path. @@ -306,7 +306,7 @@ Bonus: `auth_pin_hash` uses unsalted SHA-256. A 4-6-digit PIN is rainbow-table'd **Location:** `codec_agent_plan.py:791` **CWE / OWASP:** CWE-20 Improper Input Validation -> **Closed by PR-1D** (branch `fix/pr1d-permission-gate-realpath`). Replaced `any(b in raw for b in _PATH_BLOCKLIST_SUBSTRINGS)` with `_is_path_blocklisted(raw)` which calls `_path_segments_match` on each entry. `_path_segments_match` checks whether the blocklist pattern's `/`-separated segments appear as a CONSECUTIVE SUBSEQUENCE of the path's segments (path is `expanduser` + `normpath`-ed first to collapse `..` and `.`). So `~/Documents/notes_ssh/foo.md` no longer matches `/.ssh` as a false positive (the segment is `notes_ssh`, not `.ssh`), but `~/.ssh/config` still does. Three regression-coverage tests in `tests/test_agent_plan.py` (segment-aware false-positive negative, still-blocks-real-ssh, legitimate-user-paths-pass-through). +> **Closed by PR-1D** ([#47](https://github.com/AVADSA25/codec/pull/47), merged as `fd2b460`). Replaced `any(b in raw for b in _PATH_BLOCKLIST_SUBSTRINGS)` with `_is_path_blocklisted(raw)` which calls `_path_segments_match` on each entry. `_path_segments_match` checks whether the blocklist pattern's `/`-separated segments appear as a CONSECUTIVE SUBSEQUENCE of the path's segments (path is `expanduser` + `normpath`-ed first to collapse `..` and `.`). So `~/Documents/notes_ssh/foo.md` no longer matches `/.ssh` as a false positive (the segment is `notes_ssh`, not `.ssh`), but `~/.ssh/config` still does. Three regression-coverage tests in `tests/test_agent_plan.py` (segment-aware false-positive negative, still-blocks-real-ssh, legitimate-user-paths-pass-through). **Description:** `if any(b in raw for b in _PATH_BLOCKLIST_SUBSTRINGS): continue` — the substring `/.ssh` is checked against the raw path. But variations like `~/Documents/.ssh-backup/` would match (contains `/.ssh`), correctly blocking. However, `~/Documents/notes_ssh/` does NOT contain `/.ssh` (no leading dot+s before), so it passes — which is correct, but it shows the rule is purely substring. More worryingly, `~/Documents/passthrough/` doesn't contain any blocklist substring even though `passthrough` semantically signals "this is sensitive" (just an example — but the more important miss is `/.codec/skills` from D-14). **Impact:** Sub-finding of D-14. False sense of security from a fragile substring filter. **Recommended fix:** Convert to a regex-anchored or path-segment-aware check. Use `Path(raw).parts` and check for any forbidden segment.