|
| 1 | +# Security Review: Sprint 4 (SAN-35) |
| 2 | + |
| 3 | +**Date:** 2026-03-23 |
| 4 | +**Scope:** Commits `6877fff` (feature/patch-subprocess merge), `9f532da` (SAN-1 exception narrowing), `467faa5` (SAN-27 fingerprint edge cases) |
| 5 | +**Reviewer:** Claude Code (automated security audit) |
| 6 | + |
| 7 | +--- |
| 8 | + |
| 9 | +## Executive Summary |
| 10 | + |
| 11 | +Sprint 4 introduced a subprocess interceptor for CLI governance, narrowed exception handling across 18 files, and fixed fingerprint edge cases for TypeScript/spec alignment. The audit found **1 critical vulnerability** (shell metacharacter bypass in the subprocess interceptor) that was fixed in this commit. Several HIGH/MEDIUM findings are documented for remediation in future sprints. |
| 12 | + |
| 13 | +--- |
| 14 | + |
| 15 | +## 1. Subprocess Interceptor (feature/patch-subprocess) |
| 16 | + |
| 17 | +### CRITICAL-1: Shell metacharacter bypass via `shell=True` (FIXED) |
| 18 | + |
| 19 | +**Location:** `src/sanna/interceptors/subprocess_interceptor.py`, `_resolve_command()` |
| 20 | + |
| 21 | +When `shell=True` is passed to `subprocess.run()` (or `call`, `check_call`, `check_output`, `Popen`), the kernel's `/bin/sh` interprets shell metacharacters (`;`, `|`, `&&`, `||`, `` ` ``, `$()`) for command chaining. The interceptor was parsing the command with `str.split()`, which only sees whitespace — it evaluated authority on the first command only. |
| 22 | + |
| 23 | +**Exploit example:** |
| 24 | +```python |
| 25 | +subprocess.run("echo hello; rm -rf /", shell=True) |
| 26 | +# Interceptor sees: binary="echo", argv=["hello;", "rm", "-rf", "/"] |
| 27 | +# Authority check: "echo" -> can_execute -> allowed |
| 28 | +# Shell executes: echo hello AND rm -rf / |
| 29 | +``` |
| 30 | + |
| 31 | +**Fix applied:** Added `_check_shell_chaining()` which splits command strings on shell operators (`;`, `|`, `||`, `&&`) and also extracts `$()` and backtick substitutions. Each sub-command is independently evaluated against the constitution's authority boundaries. Uses `shlex.split()` instead of `str.split()` for proper tokenization. Applied to all 6 patched entry points (`subprocess.run`, `.call`, `.check_call`, `.check_output`, `Popen`, `os.system`). |
| 32 | + |
| 33 | +### CRITICAL-2: Unpatched execution surfaces (DOCUMENTED — not fixed) |
| 34 | + |
| 35 | +**Severity:** CRITICAL (design limitation) |
| 36 | + |
| 37 | +The interceptor patches 6 entry points but the following allow unrestricted execution: |
| 38 | + |
| 39 | +| Surface | Risk | |
| 40 | +|---------|------| |
| 41 | +| `os.execl/execle/execlp/execlpe/execv/execve/execvp/execvpe` | Replace current process entirely | |
| 42 | +| `os.spawnl/spawnle/spawnlp/spawnlpe/spawnv/spawnve/spawnvp/spawnvpe` | Spawn new processes | |
| 43 | +| `os.popen()` | Pipe to/from a command | |
| 44 | +| `os.posix_spawn/posix_spawnp` | POSIX spawn | |
| 45 | +| `ctypes` → `libc.system()` / `execve()` | Direct C library calls | |
| 46 | + |
| 47 | +**Recommendation:** Future sprint should either (a) patch `os.exec*`, `os.spawn*`, and `os.popen`, or (b) document this explicitly in the interceptor's threat model as a known boundary. Option (b) is acceptable if the threat model assumes the governed agent does not have arbitrary code execution — the interceptor protects against tool-use agents, not full code execution. |
| 48 | + |
| 49 | +### CRITICAL-3: Trivial unpatch by governed code (DOCUMENTED — not fixed) |
| 50 | + |
| 51 | +**Severity:** CRITICAL (design limitation) |
| 52 | + |
| 53 | +`unpatch_subprocess()` is a public API in `__init__.py`. Governed code can: |
| 54 | +```python |
| 55 | +from sanna.interceptors import unpatch_subprocess |
| 56 | +unpatch_subprocess() # All governance removed |
| 57 | +``` |
| 58 | + |
| 59 | +Also, `_originals` dict is accessible: |
| 60 | +```python |
| 61 | +from sanna.interceptors.subprocess_interceptor import _originals |
| 62 | +_originals["subprocess.run"](["rm", "-rf", "/"]) |
| 63 | +``` |
| 64 | + |
| 65 | +**Recommendation:** This is a fundamental Python limitation (monkeypatching is always reversible). Document in threat model. If stronger isolation is needed, the gateway architecture (separate process) is the correct approach. |
| 66 | + |
| 67 | +### HIGH-1: TOCTOU between authority check and execution |
| 68 | + |
| 69 | +**Location:** All patched functions (e.g., `_patched_run()` lines 542-555) |
| 70 | + |
| 71 | +A window exists between the authority check and `_originals[...](*args, **kwargs)`. If caller passes a mutable list and another thread mutates it, the executed command could differ from what was checked. Mitigated by Python's GIL in CPython. |
| 72 | + |
| 73 | +### HIGH-2: Path traversal in binary name |
| 74 | + |
| 75 | +The interceptor uses `os.path.basename(binary)` which correctly strips path components, but an agent can create wrapper scripts on disk: |
| 76 | +```python |
| 77 | +# Write malicious script, then execute it |
| 78 | +subprocess.run(["./innocent_name.sh"]) # binary_name="innocent_name.sh" → not in deny list |
| 79 | +``` |
| 80 | + |
| 81 | +### HIGH-3: Thread-unsafe `_restore_originals` context manager |
| 82 | + |
| 83 | +`_restore_originals` temporarily restores original subprocess functions for the duration of the real call. During this window, concurrent calls from other threads execute without interception. |
| 84 | + |
| 85 | +### HIGH-4: Environment variable manipulation not governed |
| 86 | + |
| 87 | +An agent can manipulate execution through env vars: |
| 88 | +```python |
| 89 | +subprocess.run(["git", "push"], env={**os.environ, "GIT_SSH_COMMAND": "rm -rf /"}) |
| 90 | +``` |
| 91 | + |
| 92 | +### MEDIUM-1: `fnmatch` argv matching has edge cases |
| 93 | + |
| 94 | +Double spaces in arguments (`["git", "push", " --force"]`) produce `"push --force origin"` which doesn't match `"push --force*"`. |
| 95 | + |
| 96 | +### MEDIUM-2: Invalid regex patterns in invariants silently skipped |
| 97 | + |
| 98 | +`cli_authority.py` catches `re.error` and skips malformed regex patterns without warning. Constitution authors get no feedback that their rule is ineffective. |
| 99 | + |
| 100 | +--- |
| 101 | + |
| 102 | +## 2. Fingerprint Edge Cases (SAN-27) |
| 103 | + |
| 104 | +### MEDIUM-3: Empty checks hash change without CHECKS_VERSION bump |
| 105 | + |
| 106 | +**Location:** `receipt.py`, `middleware.py`, `verify.py`, `gateway/server.py` |
| 107 | + |
| 108 | +Empty checks array changed from `hash_obj([])` to `EMPTY_HASH`. This is a semantic change: any `checks_version: "6"` receipt with zero checks generated before this commit will fail verification after it. The verifier has no way to distinguish pre-fix vs post-fix receipts. |
| 109 | + |
| 110 | +**Impact:** Low in practice — the empty-checks path (`_generate_no_invariants_receipt`) is rarely exercised, and no golden receipts were affected. However, any production receipts from this path are now unverifiable. |
| 111 | + |
| 112 | +**Recommendation:** Either bump `CHECKS_VERSION` to `"7"` with a fallback, or document as known incompatibility. |
| 113 | + |
| 114 | +### INFO: All 4 fingerprint sites remain in parity |
| 115 | + |
| 116 | +Verified field-by-field: `receipt.py`, `middleware.py`, `verify.py`, and `gateway/server.py` all use the same 14-field formula with identical logic for `EMPTY_HASH`, `is not None` checks, and `constitution_approval` stripping. |
| 117 | + |
| 118 | +### INFO: No fingerprint collision attack surface |
| 119 | + |
| 120 | +The pipe-delimited formula prevents field-shifting. `EMPTY_HASH` is deterministic. The conditional 4-field vs 8-field check hashing auto-detects correctly based on `triggered_by` presence. |
| 121 | + |
| 122 | +--- |
| 123 | + |
| 124 | +## 3. Exception Narrowing (SAN-1) |
| 125 | + |
| 126 | +### MEDIUM-4: Missing `UnsupportedAlgorithm` in crypto catches |
| 127 | + |
| 128 | +**Location:** `bundle.py` (lines 514, 560, 667), `constitution.py` (line 1687), `verify.py` (line 789) |
| 129 | + |
| 130 | +`cryptography.exceptions.UnsupportedAlgorithm` inherits from `Exception`, not from `ValueError`/`TypeError`/`OSError`. The narrowed catches miss this exception. In practice, `load_public_key()` checks `isinstance` first and raises `ValueError` for non-Ed25519 keys, so this only fires for truly unsupported backend algorithms — rare but possible. |
| 131 | + |
| 132 | +**Impact:** Uncaught exception crashes verification instead of recording a failed check (fail-closed, which is safe but not graceful). |
| 133 | + |
| 134 | +### MEDIUM-5: Threaded webhook fallback misses `TypeError` |
| 135 | + |
| 136 | +**Location:** `enforcement/escalation.py` (line 372) |
| 137 | + |
| 138 | +Narrowed from `except Exception` to `except (OSError, urllib.error.URLError)`. If `payload` contains non-serializable objects, `json.dumps()` raises `TypeError` which is now uncaught. Since this runs in a daemon thread, the exception silently kills the thread. |
| 139 | + |
| 140 | +### LOW-1: MCP server crypto catches left broad (inconsistency) |
| 141 | + |
| 142 | +`mcp/server.py` (lines 772, 859) retains `except Exception` while equivalent blocks in `bundle.py`/`verify.py` were narrowed. Not a bug (outer MCP handler catches all), but inconsistent. |
| 143 | + |
| 144 | +### INFO: No sensitive information leakage detected |
| 145 | + |
| 146 | +All narrowed catches follow safe patterns: no stack traces to users, no key material in error messages, crypto errors produce generic "verification failed" messages. |
| 147 | + |
| 148 | +--- |
| 149 | + |
| 150 | +## Summary |
| 151 | + |
| 152 | +| ID | Severity | Area | Status | |
| 153 | +|----|----------|------|--------| |
| 154 | +| CRITICAL-1 | CRITICAL | Shell metacharacter bypass | **FIXED** | |
| 155 | +| CRITICAL-2 | CRITICAL | Unpatched os.exec*/spawn*/popen | Documented (design limitation) | |
| 156 | +| CRITICAL-3 | CRITICAL | Trivial unpatch | Documented (design limitation) | |
| 157 | +| HIGH-1 | HIGH | TOCTOU race | Documented | |
| 158 | +| HIGH-2 | HIGH | Path traversal via wrapper scripts | Documented | |
| 159 | +| HIGH-3 | HIGH | Thread-unsafe _restore_originals | Documented | |
| 160 | +| HIGH-4 | HIGH | Env var manipulation | Documented | |
| 161 | +| MEDIUM-1 | MEDIUM | fnmatch edge cases | Documented | |
| 162 | +| MEDIUM-2 | MEDIUM | Silent regex errors | Documented | |
| 163 | +| MEDIUM-3 | MEDIUM | Empty checks hash change | Documented | |
| 164 | +| MEDIUM-4 | MEDIUM | Missing UnsupportedAlgorithm | Documented | |
| 165 | +| MEDIUM-5 | MEDIUM | Webhook TypeError | Documented | |
| 166 | +| LOW-1 | LOW | MCP catch inconsistency | Documented | |
0 commit comments