fix: make the firewall self-verify fail closed and match exact rules

Two defects that compounded: the verify file was written before the
VERIFY_OK gate, and the Lima readiness probe treats its existence as
'firewall is up' — so a failed verification still produced a VM that
limactl start reported ready. In the container sandbox the same exit 1
aborted entrypoint.sh and the container died; the equivalent here is to
withhold the readiness signal, so the file is now written only after
every check passes.

The checks themselves could not be trusted either: the gateway-reject
check grepped the human-readable listing for 'owner UID match <agent
uid>', which the agent-to-Squid ACCEPT rule also contains, so it
reported the reject as present whether or not it existed. The proxy
check had the identical weakness. Both now match whole rule lines from
iptables -S, and the suite asserts those specs directly rather than
trusting the guest's self-report.

sandbox-boot.sh traps ERR and touches a failure marker the probe
watches, turning a broken boot from a silent 300s timeout into an
immediate failure that names the step.

Verified by patching one check in a copy of the live script to expect a
nonexistent rule: exit 1, the specific failure named, no verify file
written; and the probe exits non-zero in 0s with the marker present.

Author: stempler

docs/superpowers/reviews/2026-07-31-code-review-findings.md

@@ -10,8 +10,9 @@ correctness angle, then an independent adversarial verifier per candidate
 location. 35 agents; all ten findings below survived verification as
 `CONFIRMED`. Line references were re-checked against the working tree after the
 review completed.
-**Status:** findings 6, 7 and 8 are **fixed** in `7b5209b`; finding 2 is **closed
-by removal**. The rest are open. Each section below carries its own status line.
+**Status:** findings 4, 5, 6, 7 and 8 are **fixed**; finding 2 is **closed by
+removal**. Findings 1, 3, 9 and 10 are open. Each section below carries its own
+status line; `git log` has the commits.
 
 **Plus one issue the review did not surface.** `.sandbox-secrets.yaml` was read
 from the agent-writable workspace and its `source:` values executed **on the
@@ -36,8 +37,9 @@ must do differently.
 Most findings are **regressions relative to the Docker sandbox**, not novel
 bugs: places where porting a control from `entrypoint.sh` into
 `sandbox-boot.service` silently dropped a property the container version had.
-The two most serious still-open ones (1 and 4) are both of that shape. Finding 9
-is a decision rather than a defect and is marked as such.
+Findings 1 and 4 were the clearest cases — an unfiltered-egress window and a
+firewall check that stopped being fatal. Finding 9 is a decision rather than a
+defect and is marked as such.
 
 ---
 
@@ -116,7 +118,20 @@ group to use from the GID that is actually present rather than assuming the name
 ## 4. Firewall self-verification fails open instead of closed
 
 **`internal/guest/files/scripts/init-firewall.sh:251`** (verify file written) vs
-**`:254`** (`VERIFY_OK` gate) — severity: high
+**`:254`** (`VERIFY_OK` gate) — severity: high — **FIXED**
+
+*Fix as applied:* the verify file is written only after every check passes, so its
+existence is a real success signal and a failed verification withholds the
+readiness signal entirely — the equivalent of the container dying. It also gained
+a `VERIFY=ok` first line. Because the probe would otherwise only notice by timing
+out after 300 s, `sandbox-boot.sh` now traps `ERR` and touches
+`/run/sandbox-boot-failed`, which the probe watches so a broken boot fails
+immediately and names the step.
+
+*Verified empirically,* not just by reading: patching one `has_rule` check in a
+copy of the live script to expect a nonexistent rule produced `exit=1`, the
+message `the DNS-tunneling UDP drop is missing`, and no verify file. The probe
+loop with the marker present exited non-zero in 0 s rather than 300 s.
 
 The script writes `/run/firewall-verify` **before** it evaluates `VERIFY_OK` and
 exits 1. The Lima readiness probe only checks that the file *exists*. In the
@@ -130,13 +145,23 @@ success, and `code-vm` runs Claude in a VM whose firewall failed its own
 verification. Nothing host-side reads the file's *contents*; only the
 informational `status` and `firewall` commands do.
 
-*Fix direction:* write the verify file only after `VERIFY_OK` passes (and have
-the probe or `ensureRunning` treat a failed `sandbox-boot.service` as fatal).
-This restores the fail-closed property without weakening any check.
+*Note:* the fix was to withhold the readiness signal, never to relax a check.
 
 ## 5. The gateway-REJECT check is satisfied by the Squid ACCEPT rule
 
-**`internal/guest/files/scripts/init-firewall.sh:236`** — severity: medium
+**`internal/guest/files/scripts/init-firewall.sh:236`** — severity: medium — **FIXED**
+
+*Fix as applied:* every check now matches a whole rule line from `iptables -S`
+via `grep -qxF`, so no other rule can satisfy it. The proxy-egress check had the
+identical weakness and was fixed with it. The suite additionally asserts the four
+rule specs directly against `iptables -S`, independent of the guest's
+self-report — the previous assertions only confirmed that the guest said "yes".
+
+One thing worth recording: the first version of this fix routed each check
+through a helper called in a command substitution, where `VERIFY_OK=false` would
+have been set in a subshell and discarded — the verification would have passed
+unconditionally, reintroducing finding 4 while appearing to fix finding 5. The
+checks are spelled out individually for that reason.
 
 `gw_reject` greps for any line containing `owner UID match $AGENT_UID`. The
 agent-to-Squid ACCEPT rule appended earlier (`:192`) matches that string too, so
@@ -257,14 +282,13 @@ loud warning.
 
 ## Remaining triage order
 
-Findings 6, 7 and 8 are fixed; 2 is closed by removal. What is left, in the order
-I would take it:
+Findings 4, 5, 6, 7 and 8 are fixed; 2 is closed by removal. What is left, in the
+order I would take it:
 
-1. **4** and **5** — self-verify defects; small, self-contained, and they restore
-   guarantees the suite currently only appears to check.
-2. **3** — one-line-class fix that prevents an unusable VM for a large class of
+1. **3** — one-line-class fix that prevents an unusable VM for a large class of
    hosts (any host user whose primary GID collides with a stock guest group).
-3. **1** — drop `-l` from `run_as_agent`; removes an unfiltered-egress channel.
-4. **10** — re-add the boot-time API reachability warning.
-5. **9** — decide whether CI should run the VM suite now that GitHub runners
+   Most likely of the remaining four to bite a real user.
+2. **1** — drop `-l` from `run_as_agent`; removes an unfiltered-egress channel.
+3. **10** — re-add the boot-time API reachability warning.
+4. **9** — decide whether CI should run the VM suite now that GitHub runners
    expose KVM.

internal/guest/files/lima/code-sandbox.yaml.tpl

@@ -72,15 +72,24 @@ provision:
     exec /usr/local/lib/sandbox/provision-system.sh
 
 probes:
-# init-firewall.sh writes /run/firewall-verify last. Waiting on it makes
-# `limactl start` return only once the egress firewall is actually up, so a
-# code-vm session can never land in an unfiltered VM.
+# init-firewall.sh writes /run/firewall-verify last, and only once its own
+# checks have passed. Waiting on it makes `limactl start` return solely when the
+# egress firewall is verified up, so a code-vm session can never land in an
+# unfiltered VM. sandbox-boot.sh touches the failure marker on any error, which
+# turns a broken boot into an immediate failure instead of a silent timeout.
 - mode: readiness
   description: sandbox boot sequence to finish
   script: |
     #!/bin/bash
     set -eu
-    timeout 300s bash -c 'until [ -f /run/firewall-verify ]; do sleep 2; done'
+    timeout 300s bash -c 'until [ -f /run/firewall-verify ]; do
+      if [ -f /run/sandbox-boot-failed ]; then
+        echo "the sandbox boot sequence failed; the firewall is not up" >&2
+        exit 1
+      fi
+      sleep 2
+    done'
   hint: |
-    The sandbox boot sequence did not finish. Inspect it with:
+    The sandbox boot sequence did not finish, so the egress firewall is not
+    verified and the VM is deliberately not reported ready. Inspect it with:
       limactl shell code-sandbox sudo journalctl -u sandbox-boot.service

internal/guest/files/scripts/init-firewall.sh

@@ -238,31 +238,81 @@ iptables -A OUTPUT -m limit --limit 5/min -j LOG --log-prefix "[FIREWALL-BLOCKED
 iptables -A OUTPUT -j REJECT --reject-with icmp-port-unreachable
 
 # ── Self-verify ─────────────────────────────────────────────────────────────
-# The Lima readiness probe waits for this file, so `limactl start` cannot
-# return before the firewall is up.
+# Checks match whole rule specs from `iptables -S`, not substrings of the
+# human-readable listing. The loose form was actively misleading: the
+# agent-to-Squid ACCEPT rule added above also contains
+# "owner UID match <agent uid>", so grepping for that reported the gateway
+# REJECT as present even when it was gone.
+RULES=$(iptables -S OUTPUT)
+PROXY_UID=$(id -u proxy 2> /dev/null || echo 13)
+
 VERIFY_OK=true
-OUTPUT_POLICY=$(iptables -L OUTPUT -n | head -1 | grep -o "DROP" || echo "NOT_DROP")
-[ "$OUTPUT_POLICY" = "DROP" ] || VERIFY_OK=false
+
+# has_rule matches a complete rule line, so no other rule can satisfy a check.
+# Each check is spelled out rather than routed through a helper that returns its
+# result: a helper called in a command substitution runs in a subshell, where
+# setting VERIFY_OK=false would be silently discarded and the verification would
+# always pass.
+has_rule() { printf '%s\n' "$RULES" | grep -qxF -- "$1"; }
+
+OUTPUT_POLICY=NOT_DROP
+if has_rule "-P OUTPUT DROP"; then
+    OUTPUT_POLICY=DROP
+else
+    echo "[firewall] ERROR: the OUTPUT policy is not DROP" >&2
+    VERIFY_OK=false
+fi
 
 udp_drop=no
-iptables -L OUTPUT -n | grep -qE "DROP[[:space:]]+17|DROP.*udp" && udp_drop=yes
-[ "$udp_drop" = yes ] || VERIFY_OK=false
+if has_rule "-A OUTPUT -p udp -j DROP"; then
+    udp_drop=yes
+else
+    echo "[firewall] ERROR: the DNS-tunneling UDP drop is missing" >&2
+    VERIFY_OK=false
+fi
 
-PROXY_UID=$(id -u proxy 2> /dev/null || echo 13)
 proxy_rule=no
-iptables -L OUTPUT -n -v | grep -q "owner UID match $PROXY_UID" && proxy_rule=yes
-[ "$proxy_rule" = yes ] || VERIFY_OK=false
+if has_rule "-A OUTPUT -m owner --uid-owner $PROXY_UID -j ACCEPT"; then
+    proxy_rule=yes
+else
+    echo "[firewall] ERROR: Squid's own egress rule (uid $PROXY_UID) is missing" >&2
+    VERIFY_OK=false
+fi
 
-gw_reject=no
+gw_reject=skipped
 if [ -n "$GATEWAY" ]; then
-    iptables -L OUTPUT -n -v | grep -q "owner UID match $AGENT_UID" && gw_reject=yes
-    [ "$gw_reject" = yes ] || VERIFY_OK=false
+    gw_reject=no
+    if has_rule "-A OUTPUT -d ${GATEWAY}/32 -m owner --uid-owner ${AGENT_UID} -j REJECT --reject-with icmp-port-unreachable"; then
+        gw_reject=yes
+    else
+        echo "[firewall] ERROR: the agent-to-host-gateway reject ($GATEWAY) is missing" >&2
+        VERIFY_OK=false
+    fi
 fi
 
 squid_running=no
-(echo > /dev/tcp/localhost/3128) 2> /dev/null && squid_running=yes
+if (echo > /dev/tcp/localhost/3128) 2> /dev/null; then
+    squid_running=yes
+else
+    echo "[firewall] ERROR: Squid is not listening on 3128" >&2
+    VERIFY_OK=false
+fi
+
+if [ "$VERIFY_OK" != true ]; then
+    # Deliberately leave $VERIFY_FILE absent. The Lima readiness probe treats
+    # its existence as "the firewall is up", so writing it before this gate —
+    # as an earlier version did — let `limactl start` succeed and hand the
+    # agent a VM whose firewall had failed its own checks. In the container
+    # sandbox this same exit aborted entrypoint.sh and the container died; the
+    # equivalent here is to withhold the readiness signal.
+    echo "[firewall] ERROR: verification failed; refusing to report the firewall as ready." >&2
+    exit 1
+fi
 
+# Written only once every check above has passed, so its presence is a real
+# success signal rather than a record of whatever was found.
 {
+    echo "VERIFY=ok"
     echo "OUTPUT_POLICY=$OUTPUT_POLICY"
     echo "UDP_DROP=$udp_drop"
     echo "PROXY_UID_RULE=$proxy_rule"
@@ -273,8 +323,4 @@ squid_running=no
 } > "$VERIFY_FILE"
 chmod 0444 "$VERIFY_FILE"
 
-if [ "$VERIFY_OK" != true ]; then
-    echo "[firewall] ERROR: verification failed; rules are incorrect."
-    exit 1
-fi
 echo "[firewall] Active. DEFAULT DENY + Squid allowlist on :3128 (mode=$MODE)"

internal/guest/files/scripts/sandbox-boot.sh

@@ -12,6 +12,20 @@
 ###############################################################################
 set -euo pipefail
 
+FAIL_MARKER=/run/sandbox-boot-failed
+rm -f "$FAIL_MARKER"
+
+# init-firewall.sh withholds /run/firewall-verify when its own checks fail, and
+# the Lima readiness probe waits for that file — so a failure already blocks the
+# VM from being reported ready. Without this marker the probe would only find
+# out by timing out, several minutes later, with nothing to point at. Touching
+# it lets the probe fail immediately and name the step that broke.
+on_failure() {
+    echo "[boot] FAILED at line $1 — the sandbox is not safe to use" >&2
+    : > "$FAIL_MARKER"
+}
+trap 'on_failure $LINENO' ERR
+
 echo "[boot] Sandbox boot sequence starting"
 
 /usr/local/lib/sandbox/update-agent-clis.sh

internal/lima/testdata/golden-template.yaml

@@ -84,15 +84,24 @@ provision:
     exec /usr/local/lib/sandbox/provision-system.sh
 
 probes:
-# init-firewall.sh writes /run/firewall-verify last. Waiting on it makes
-# `limactl start` return only once the egress firewall is actually up, so a
-# code-vm session can never land in an unfiltered VM.
+# init-firewall.sh writes /run/firewall-verify last, and only once its own
+# checks have passed. Waiting on it makes `limactl start` return solely when the
+# egress firewall is verified up, so a code-vm session can never land in an
+# unfiltered VM. sandbox-boot.sh touches the failure marker on any error, which
+# turns a broken boot into an immediate failure instead of a silent timeout.
 - mode: readiness
   description: sandbox boot sequence to finish
   script: |
     #!/bin/bash
     set -eu
-    timeout 300s bash -c 'until [ -f /run/firewall-verify ]; do sleep 2; done'
+    timeout 300s bash -c 'until [ -f /run/firewall-verify ]; do
+      if [ -f /run/sandbox-boot-failed ]; then
+        echo "the sandbox boot sequence failed; the firewall is not up" >&2
+        exit 1
+      fi
+      sleep 2
+    done'
   hint: |
-    The sandbox boot sequence did not finish. Inspect it with:
+    The sandbox boot sequence did not finish, so the egress firewall is not
+    verified and the VM is deliberately not reported ready. Inspect it with:
       limactl shell code-sandbox sudo journalctl -u sandbox-boot.service

test-vm-sandbox.sh

@@ -148,7 +148,7 @@ echo ""
 echo "── Firewall ──────────────────────────────────────────────────────"
 
 VERIFY=$(adm cat /run/firewall-verify)
-for kv in "OUTPUT_POLICY=DROP" "UDP_DROP=yes" "PROXY_UID_RULE=yes" \
+for kv in "VERIFY=ok" "OUTPUT_POLICY=DROP" "UDP_DROP=yes" "PROXY_UID_RULE=yes" \
     "AGENT_GATEWAY_REJECT=yes" "SQUID_RUNNING=yes"; do
     if echo "$VERIFY" | grep -qx "$kv"; then
         pass "firewall self-verify: $kv"
@@ -157,6 +157,33 @@ for kv in "OUTPUT_POLICY=DROP" "UDP_DROP=yes" "PROXY_UID_RULE=yes" \
     fi
 done
 
+# The assertions above trust the guest's own report. These check the rules
+# directly, because a self-check can be wrong in the direction that matters:
+# the gateway-reject check used to match any rule mentioning the agent UID, so
+# the agent-to-Squid ACCEPT rule satisfied it and the report said "yes" whether
+# or not the reject existed.
+RULES=$(adm iptables -S OUTPUT)
+# shellcheck disable=SC2016 # awk program, expanded inside the guest
+GUEST_GW=$(adm sh -c 'ip route show default | awk "{print \$3; exit}"')
+GUEST_AGENT_UID=$(adm id -u "$AGENT_USER")
+GUEST_PROXY_UID=$(adm id -u proxy)
+
+for spec in \
+    "-P OUTPUT DROP" \
+    "-A OUTPUT -p udp -j DROP" \
+    "-A OUTPUT -m owner --uid-owner $GUEST_PROXY_UID -j ACCEPT" \
+    "-A OUTPUT -d ${GUEST_GW}/32 -m owner --uid-owner ${GUEST_AGENT_UID} -j REJECT --reject-with icmp-port-unreachable"; do
+    if echo "$RULES" | grep -qxF -- "$spec"; then
+        pass "iptables rule present: $spec"
+    else
+        fail "iptables rule present: $spec"
+    fi
+done
+
+# A failed boot must leave no readiness signal and no stale failure marker.
+assert_fails "no boot-failure marker on a healthy VM" \
+    adm test -e /run/sandbox-boot-failed
+
 # No -f here: the API root returns 404, and for HTTPS a Squid denial breaks
 # the CONNECT tunnel itself, so any completed HTTP exchange proves reachability.
 assert_ok "allowlisted domain reachable through the proxy" \