# Qubes-Snitch Security Audit — Opus 4.8 (max effort)

Scope: Critical/High only. Focus areas: inter-VM bypass/escape, DoS (daemon crash / lock-out),
UI/prompt tampering, privilege escalation.

Threat-model note (from README-AUDIT.md, honored here): the protected client VM is untrusted;
the local `user`/root inside `sys-snitch`, dom0, qrexec policy, and manually-edited YAML are trusted.
`Restart=no` in `qubes-snitchd.service` means any daemon exit is persistent until manual restart:
a crash therefore blocks the network for *every* VM behind `sys-snitch` (fail-closed lock-out).

---

## Finding 1 — Hostname-backed decision (uppercase `A`/`R`) crashes the whole daemon on ordinary DNS variance

* **File & Function:** `qubes_snitch/queue.py: prepare_hostname_decision()` (lines 83–94), reached from
  `save_pending_decision()` (line 116); crash is finalized in
  `qubes_snitch/config.py: resolve_dest_dns()` (lines 257–276) and
  `qubes_snitch/daemon_runtime.py: handle_cli_connection()` (lines 99–104) →
  `qubes_snitch/alerts_runtime.py: fatal_security_alert()`/`fail_daemon()` (lines 120–143).
* **Severity:** High
* **Description:**
  When the user answers a hostname-backed prompt with uppercase `A` (allow-dns) or `R` (reject-dns),
  the daemon (CLI-server thread) runs `prepare_hostname_decision()`, which performs a **live, blocking
  DNS resolution** of the hostname via `config.resolve_dest_dns("prompt", hostname)` with a 3-second
  lifetime, and then requires `len(resolved) > 1` and `request["dst"] in resolved`.

  `resolve_dest_dns()` raises `SystemExit` on *any* of these ordinary conditions:
  - the lookup times out / SERVFAILs / NXDOMAINs (`except Exception: raise SystemExit(... could not resolve ...)`),
  - the name now resolves to zero IPv4 addresses (`raise SystemExit(... resolved no IPv4 addresses)`),
  and `prepare_hostname_decision()` itself raises `SystemExit` when the name now resolves to
  `<= 1` IP or no longer includes the prompted `dst`.

  That `SystemExit` is not handled as "refuse this one rule". It propagates out of
  `queue.handle_cli_connection()` into `daemon_runtime.handle_cli_connection()`, whose
  `except BaseException` treats it as `fatal_security_alert(... "CLI decision persistence failed" ...)`.
  Because this runs in the CLI-server worker thread, `fail_daemon()` calls `os._exit(1)` and the entire
  daemon dies. With `Restart=no`, `ExecStopPost` reloads the fail-closed table and the network stays
  dead for all downstream VMs until an admin manually restarts the service.

  The trigger is *normal DNS behavior*, not a malformed input:
  - The uppercase `A`/`R` choice is only offered when the hint is `A <name>` resolving to multiple IPs
    at prompt time. Between the moment the prompt is drawn and the moment the user presses the key
    (human latency, often seconds), the A-record set for that name can legitimately shrink to one IP,
    change, or transiently fail — round-robin CDNs, short TTLs, and a slow/loaded resolver all do this
    routinely (the README's own example uses `updates.signal.org`, a multi-IP CDN name).
  - The **reject** case is especially perverse: a user pressing `R` specifically to *block* a
    suspicious multi-IP destination will instead take the firewall offline for the whole system if the
    resolver is momentarily slow.
* **Attacker amplification (untrusted VM):** the `A <name>` hint originates from the daemon's own
  resolution of a domain the VM was allowed to look up, and the VM controls that domain's DNS. A
  compromised VM can (1) publish a >1-IP A set so the uppercase choices appear, then (2) flip the name
  to a single IP or SERVFAIL right as the user reacts, deterministically converting a user keypress into
  a full firewall outage. It is a user-interaction-gated but attacker-influenced denial of service, and
  it also fires by accident under normal network jitter.
* **Impact:** Total loss of forwarding for every VM behind `sys-snitch` (fail-closed lock-out) from a
  routine user action; no automatic recovery (`Restart=no`). Availability impact on the core security
  service.
* **Remediation:** Treat "cannot build a hostname-backed rule right now" as a *non-fatal* refusal of the
  current decision, exactly like an invalid CLI answer (`handle_cli_connection` already just logs and
  keeps the prompt pending for those). Do not let `resolve_dest_dns()`'s `SystemExit` reach the
  `fatal_security_alert` path. For example, catch it where the hostname decision is prepared and keep the
  prompt pending (or fall back to a single-IP `dest` rule for the concrete `dst`):

  ```python
  # queue.save_pending_decision()
  try:
      prepare_hostname_decision(request, decision)
  except SystemExit as error:
      syslog.syslog(syslog.LOG_INFO,
          f"QUBES-SNITCH keep pending after unresolvable hostname decision: {error}")
      return   # keep prompt queued; do NOT crash the daemon
  ```

  and/or give `resolve_dest_dns()` a non-fatal variant for the interactive decision path that raises a
  caught exception instead of `SystemExit`. More broadly, `daemon_runtime.handle_cli_connection`'s
  blanket `except BaseException -> fatal_security_alert` is too aggressive: distinguish "this decision is
  not persistable" (keep pending) from "persistence machinery is broken" (fail closed).

---

## Finding 2 — Any saved hostname-backed (`dest_dns`) rule bricks the daemon on the next start: `dest_dns` is resolved at load time *while the fail-closed table blocks the daemon's own DNS*

* **File & Function:** `qubes_snitch/config.py: load_rules()` (lines 400–422) →
  `resolve_dest_dns_names()` (lines 279–287) → `resolve_dest_dns()` (lines 257–276, raises `SystemExit`).
  Called from `qubes_snitch/policy_runtime.py: load_policy_without_sources()` (lines 122–126), which
  `qubes_snitch/daemon_runtime.py: main()` (line 170) runs **before** any runtime nft policy is installed
  (`refresh_sources_required()` → `load_nft()` happens later, line 173). Interacts with
  `templates/usr/lib/qubes-snitch/fail-closed.nft` (output chain `policy drop`) and the systemd
  `ExecStartPre=/usr/sbin/nft -f .../fail-closed.nft`.
* **Severity:** High (deterministic, permanent, whole-VM lock-out; borders Critical)
* **Description:**
  Startup order in `main()` is:
  1. `load_policy_without_sources()` → `config.load_rules()`, which for every rule that has `dest_dns`
     performs a **live DNS resolution** via `resolve_dest_dns_names()` /
     `dns.resolver.resolve(qname, "A", lifetime=3.0)`.
  2. only *afterwards* does `refresh_sources_required()` call `load_nft()` to install the runtime table
     whose output chain allows the daemon's DNS to the Qubes internal resolvers
     (`ip daddr { 10.139.1.1, 10.139.1.2 } udp dport 53 accept`, `nft.py:render_runtime_host_chains`).

  During step 1 the only table loaded is the **fail-closed** table (installed by the early
  `qubes-snitch-fail-closed.service` and re-installed by `qubes-snitchd.service`'s
  `ExecStartPre`). Its `chain output { type filter hook output priority filter; policy drop;
  meta nfproto ipv4 ... reject with icmpx admin-prohibited }` rejects *all* locally-originated IPv4
  traffic — including the daemon's own DNS query. README-AUDIT.md confirms this is intended:
  "fail-closed ... blocks ... sys-snitch local input/output, including DNS."

  Therefore `dns.resolver.resolve()` in `resolve_dest_dns()` cannot succeed. It raises (Timeout /
  NoNameservers / etc.), which `resolve_dest_dns()` converts to `SystemExit`, or it returns no A records
  and `resolve_dest_dns()` raises `SystemExit("... resolved no IPv4 addresses")`.
  `resolve_dest_dns_names()` re-raises that `SystemExit` through `future.result()`, so `load_rules()` —
  and thus `main()` — aborts **before the runtime policy is ever installed**.

  Net effect: if *any* rule file under `/rw/usrlocal/qubes-snitch/rules/` contains a single `dest_dns`
  entry, the daemon fails to start on **every** boot and **every** `systemctl restart`. Because
  `Restart=no` and `ExecStopPost` re-applies the fail-closed table, the VM is left permanently
  fail-closed: no forwarding for any downstream VM until an admin manually hand-edits the YAML to remove
  the `dest_dns` rule.

  This is not an exotic input — `dest_dns` rules are created by the **documented uppercase-`A`
  ("allow the whole resolved IPv4 set") prompt feature** highlighted in README.md, and the YAML example
  with `dest_dns:` is shown as a normal saved rule. The very comment on `resolve_dest_dns_names()`
  ("so daemon restart is not one DNS timeout per rule") shows restart-time resolution was anticipated,
  but the fail-closed-blocks-DNS ordering was not.
* **Impact:** Using a documented feature (or any manual `dest_dns` rule) silently arms a
  permanent, self-inflicted denial of service: the firewall VM will not come back up after a reboot or
  restart, cutting off network for every VM routed through `sys-snitch`, and recovery requires manual
  YAML surgery in the (now network-isolated) VM. An untrusted VM that can get the user to approve a
  hostname-backed rule for an attacker-controlled domain, then let that domain lapse/NXDOMAIN, makes the
  brick certain on the next restart.
* **Remediation:** Do not resolve `dest_dns` synchronously during fail-closed startup, or make load-time
  resolution failure non-fatal and deferred. Options:
  - Defer `dest_dns` resolution until *after* the runtime table (which permits resolver DNS) is
    installed — i.e., resolve in/after `refresh_sources_required()`/`load_nft`, not in
    `load_policy_without_sources()`.
  - Make a failed/empty `dest_dns` resolution at load time **non-fatal**: keep the rule but mark it
    "unresolved" (`_resolved_dests = []` handled as "matches nothing yet") and retry lazily at runtime,
    instead of `raise SystemExit`. Ensure `flow_rule_matches()` and the nft renderer tolerate an
    empty/absent `_resolved_dests` without `KeyError` or emitting an empty nft set `{ }`.
  - At minimum, allow the daemon's own outbound DNS to the Qubes internal resolvers in the fail-closed
    table (matching the runtime output chain) so startup resolution can succeed — though deferring is
    safer than widening fail-closed.

  Note: this is the load-time sibling of Finding 1 (decision-time resolution). Both come from
  `resolve_dest_dns()` using `SystemExit` as flow control for an expected, environment-dependent
  condition (DNS not answering), which is then escalated to daemon death.

---

## Finding 3 — Check-then-act race on `SOURCES_BY_IP` in the NFQUEUE callback crashes the daemon (KeyError) → fail-closed lock-out

* **File & Function:** `qubes_snitch/packet_handlers.py: add_runtime_request_fields()` (lines 41–62),
  specifically the unsynchronized reads at line 50 (`ctx.SOURCES_BY_IP[request["src"]]` with **no lock
  held**) and lines 56–57 (relocked read whose presence guarantee came from a *previously released*
  lock at lines 43–44). Concurrent writer: `qubes_snitch/sources_runtime.py:
  refresh_sources_and_nft()` (lines 40–61), which replaces `ctx.SOURCES_BY_IP` under `POLICY_LOCK` on
  every QubesDB event via the `qubesdb_source_watcher` thread.
* **Severity:** High impact (persistent lock-out), low trigger probability (narrow race).
* **Description:**
  This runs on the main NFQUEUE thread for every forwarded packet. The pattern is check-then-act across
  a released lock:
  - Line 43–44 confirm `request["src"] in ctx.SOURCES_BY_IP` under `POLICY_LOCK`, then release it.
  - For an already-known source the code falls through to line 56–57 and dereferences
    `ctx.SOURCES_BY_IP[request["src"]]`. Between the line-44 release and the line-56 re-acquire, the
    `qubesdb_source_watcher` thread can run `refresh_sources_and_nft()`, which rebinds
    `ctx.SOURCES_BY_IP` to a new map that no longer contains that IP (the source VM shut down or changed
    its NetVM). The line-57 subscript then raises `KeyError`.
  - The refresh path at line 50–51 is worse: it reads `ctx.SOURCES_BY_IP[request["src"]]` **entirely
    outside the lock**, so a concurrent rebind between line 48 and line 50 raises `KeyError` as well.

  A `KeyError` on the NFQUEUE main thread is not caught anywhere in `run_queues()`/`handle_packet()`; it
  propagates out of `NetfilterQueue.run()` and terminates `main()`. With `Restart=no` and the
  `ExecStopPost` fail-closed reload, the firewall VM is left permanently fail-closed until an admin
  restarts the service — every downstream VM loses network.
* **Impact:** Unhandled-exception crash of the core daemon from a data race in the hottest code path,
  yielding a persistent firewall lock-out. An untrusted VM behind `sys-snitch` can raise the odds
  deliberately: flood packets and then tear itself down / flip its NetVM so that an in-flight packet is
  dereferenced in the window just after its own IP is dropped from the map. It only needs to win once,
  and can retry across admin restarts. It can also fire by accident during ordinary VM
  start/stop churn.
* **Remediation:** Do the presence check and the map read under a **single** `POLICY_LOCK` acquisition,
  and never index the shared map outside the lock. For example:

  ```python
  def add_runtime_request_fields(ctx, request):
      with ctx.POLICY_LOCK:
          source = ctx.SOURCES_BY_IP.get(request["src"])
      if source is None:
          sources_runtime.refresh_sources_and_nft(ctx, force=True)
          with ctx.POLICY_LOCK:
              source = ctx.SOURCES_BY_IP.get(request["src"])
              if source is not None:
                  request["source"] = source
                  request["display_source"] = ctx.SOURCE_DISPLAY_BY_IP.get(request["src"], source)
                  ...
                  return False           # still take the log path outside the lock if desired
          if source is None:
              alerts_runtime.fatal_security_alert(ctx, ("unknown-source", request["src"]), ...)
      with ctx.POLICY_LOCK:
          source = ctx.SOURCES_BY_IP.get(request["src"])
          if source is None:             # vanished during the gap: treat as stale, drop, do not crash
              return False
          request["source"] = source
          request["display_source"] = ctx.SOURCE_DISPLAY_BY_IP.get(request["src"], source)
          policy_runtime.ensure_rule_entry(ctx, request["source"])
          add_source_label(ctx, request)
      request["host"] = None
      return True
  ```

  Use `.get(...)` (never `[...]`) on `SOURCES_BY_IP`/`SOURCE_DISPLAY_BY_IP` in this callback so a vanished
  source is handled as "drop this packet", not "crash the firewall".

---

### Reviewed and intentionally not reported

Consistent with README-AUDIT.md, the following were examined and are not reported: argv-list subprocess
usage (no shell sink), `safe_text()` control/ANSI/BiDi stripping on all display paths, ASCII gating of
DNS/PTR/config/source text, `yaml.SafeLoader`+duplicate-key loader, nft rendering (validated
dest/proto/port/action, sanitized+hashed chain names, quoted log prefixes), fail-closed base policy +
no-`bypass` NFQUEUE rules, DNS transport-before-domain ordering, wildcard apex-safety, qrexec source
inventory validation, socket/run-dir permissions, and rule-filename path-traversal (blocked by
`SOURCE_NAME_RE`). The daemon's own DNS-wire robustness is delegated to dnspython; I could not install
dnspython in this environment to fuzz `add_dns_query_fields`/`dns_reject_payload`, so no speculative
wire-parse crash is claimed (per README-AUDIT guidance) — but note that `dns_reject_payload()` in
`packet_handlers.send_dns_refused()` is only guarded by `except OSError`, so if any
`dns.message.make_response()`/`to_wire()` on an accepted-but-parseable query were to raise a
`dns.exception.DNSException` (not `OSError`), it would crash the main thread like Findings 2/3; worth a
dnspython-backed test to close out.