doos/review.md

# doos: Security Review

Date: 2026-10-23 | Reviewer: tai | Scope: `src/*.cpp/*.hpp`, `CMakeLists.txt`, `Makefile`, `apparmor/doos`, `tests/run.sh`, `docs/*`, `SPEC.md`
Build: C++17 CMake project ("doos" container/sandbox tool using user namespaces, cgroup v2, seccomp-bpf, pasta/slirp4netns networking, overlayfs).

## Summary
Rootless container runtime with generally sound isolation primitives (user ns + idmap, pivot_root, cap-drop, seccomp denylist, no-new-privs, cgroup limits). Main risks: privileged helpers trusted via PATH (`pasta`, `slirp4netns`, `newuidmap/newgidmap`), TOCTOU-prone fs setup, permissive `--hardening relaxed` and `--net host` modes, and missing supply-chain/hardening controls (no SBOM/signing, no CI fuzz/scan, trivial AppArmor profile).

## Findings (severity-ordered)
1. **[MEDIUM] PATH-based exec of network helpers**: `src/network.cpp` execs `pasta`/`slirp4netns` resolved via PATH; a poisoned PATH in attach path = code exec as the user. Mitigate with absolute paths + allowlist env scrub.
2. **[MEDIUM] Symlink-following fs staging (TOCTOU)**: `src/sandbox.cpp:133-139` mirrors host `/etc` entries with `symlink_status`/`read_symlink` then recreates; bind source handling without `O_NOFOLLOW`/fd-relative ops invites race/escape. Use `openat2(RESOLVE_NO_SYMLINKS)` or pin fds.
3. **[MEDIUM] `relaxed` mode silently drops most controls**: per `cli.cpp:121`, relaxed = no cap-drop, no seccomp, full `/etc`. Users likely won't grasp severity. Require explicit `--i-understand` flag + warning.
4. **[MEDIUM] No integrity for images/layers**: `src/image.cpp` untars with numeric-owner handling but no digest/signature verification. Tampered tar = full container compromise. Add sha256 manifest check.
5. **[LOW] Temp overlay upperdir via `mkdtemp` in shared TMPDIR**: `src/image.cpp:52` uses `temp_directory_path()` (often `/tmp`, sticky but world-visible); predictable staging + 0700 chmod after the fact. Prefer per-user state dir with `O_EXCL`.
6. **[LOW] `sscanf` parsing of cgroup stat**: `src/container.cpp:231` unchecked return; malformed file → stale values. Check return value.
7. **[LOW] AppArmor profile is a stub**: `apparmor/doos` (12 lines) grants far less than runtime needs / isn't enforced by code. Either complete or remove to avoid false assurance.
8. **[INFO] No seccomp allowlist, denylist only**: `hardening.cpp` blocks keyring/bpf/perf/io_uring/mount/kexec/module-load; reasonable, but bypass surface remains (ptrace? userfaultfd covered). Document residual attack surface.
9. **[INFO] No network egress policy**: pasta/slirp give full user-mode egress; no DNS/TLS filtering. Expected for dev tool; document.

### What is done well
- User-namespace-first, no setuid binary; `newuidmap` delegation preferred.
- `NO_NEW_PRIVS` + capability bounding set to Docker-default keep-list.
- `/proc`/`/sys` remounts read-only, masked paths (`kcore`, `keys`), minimal `/dev`.
- Cgroup v2 `main/` vs `shell/` split with freeze + rescue path is thoughtful anti-ransom design.
- `pivot_root` (not just chroot), `umask`, `0700/0640` perms on state.

## Grades (0–10, 10=excellent)
| Category | Score | Rationale |
|---|---|---|
| Isolation & sandboxing | 7.5 | userns+pivot+cap-drop+seccomp+nonewprivs solid; weakened by relaxed mode, helper PATH trust |
| Filesystem security | 6.5 | Good perms/skeleton; symlink TOCTOU + /tmp staging + no dm-verity/readonly-root |
| Privilege & identity mgmt | 7.0 | No setuid, subuid mapping correct; host-uid file visibility nuance documented but subtle |
| Network security | 6.0 | User-mode net avoids root; no egress/filtering, PATH-exec helpers, `--net host` footgun |
| Resource controls (DoS) | 8.0 | cgroup v2 mem/pids/cpu + freeze model; best-effort when undelegated (fail-open must warn) |
| Input validation & parsing | 6.5 | CLI fixed-value flags good; tar/symlink handling + sscanf without strict checks |
| Secrets & state handling | 7.0 | State under DOOS_HOME with 0700; env (HOME/PATH) trusted too broadly |
| Supply chain & build | 5.0 | No SBOM, signing, pinning, CI SAST/fuzz; CMake clean but no `-D_FORTIFY`/`-fstack-protector` audit |
| Observability & audit | 5.5 | `doos list` table ok; no security event log, no seccomp-deny telemetry |
| Defaults & secure-by-default | 6.0 | Default mode good; relaxed/host modes one flag away with weak friction |
| **Overall** | **6.5** | Solid rootless dev-sandbox; fix PATH-exec, TOCTOU, image integrity before untrusted workloads |

## Recommendations (prioritized)
1. Absolute-path helpers + minimal sanitized env for all `execvp`.
2. `openat2` / fd-pinned fs staging; fail closed on symlink races.
3. Image digest verification (sha256 manifest) before untar.
4. Gate `relaxed`/`--net host` behind explicit confirmation + log warning.
5. Harden build flags (`-Wall -Wextra -Werror -D_FORTIFY_SOURCE=3 -fstack-protector-strong -Wl,-z,RelRO,Now`), add SBOM + CI (CodeQL/Semgrep/Trivy).
6. Check `sscanf` returns; move temp staging under state dir.
7. Finish or drop AppArmor profile; add seccomp-deny + OOM audit logging.

## Verdict
Fit for trusted-dev use. **Not yet recommended** for running untrusted images/payloads until (1), (2), (3) are fixed.