doos/review2.md

# doos: full review

Date: 2026-10-24 | Scope: all of `src/` (3,558 lines, read in full), `CMakeLists.txt`, `Makefile`,
`scripts/*.sh`, `tests/run.sh`, `audit/`, `valgrind/`, and every document (`README.md`, `SPEC.md`,
`docs/ARCHITECTURE.md`, `docs/RESEARCH.md`, `COMPARISON.md`, `AGENTS.md`, `CHANGELOG.md`).
Method: read every source file top to bottom (not a diff), ran the real e2e suite, the CVE audit,
and valgrind against the current binary, and tested two inputs by hand that looked suspicious while
reading. This is a second, independent pass; [`review.md`](review.md) is an earlier review whose
findings are checked against the current code, not reused.

**State at review time:** `v1.3.1`, working tree clean, `tests/run.sh` 88/88, `make audit` 18/18,
`make valgrind` 6/6 (see those tools for what they do and do not cover).

## 1. Summary

doos is a small, coherent, unusually well-specified codebase for what it is: a single C++20 binary
that builds a user-namespace container by hand (no runc, no libcontainer, no OCI anything) and
backs it with overlayfs, cgroup v2 and a seccomp denylist. The code reads like it was written by one
disciplined author following one set of rules consistently: every constant lives in `spec.hpp`, every
destructive path is capped, every external process is run from a fixed path, every error throws with
a message that says what to do about it. The project-level discipline (docs that state gaps next to
the code that closes them, a changelog per release, a `make audit`/`make valgrind` pair that produces
actual evidence instead of claims) is better than most projects an order of magnitude larger.

The flip side of "one author, one style" is that nothing here has been read by a second pair of eyes
until review.md and this review. I found one confirmed, reproducible bug (an integer overflow that
silently accepts a nonsensical `--size`) and a handful of smaller inconsistencies and gaps, listed
below. Nothing I found is a new security hole in the model doos is actually built on; the shared-kernel,
denylist-seccomp, unaudited-code risks are already disclosed in `COMPARISON.md` section 6 and
`docs/RESEARCH.md`, and I did not find reason to soften or harden that disclosure.

**Resolution (1.3.2, same day):** 2.1, 2.2 and 2.4 are fixed (see `CHANGELOG.md`); 2.3 is logged as
an accepted trade-off (gap G11 in `docs/RESEARCH.md`) rather than fixed; 2.5's CI half is resolved
by `.gitea/workflows/ci.yml` and `docs/CI.md`, and its LICENSE half by the new `LICENSE` file. The
findings below are left exactly as first written, for the record of what was found and how.

## 2. Findings

Ordered most to least severe. Each is something I read or reproduced, not a hypothetical.

### 2.1 [MEDIUM] `parseSize` silently overflows on an absurd input, instead of rejecting it

`src/util.cpp:54-71`. The function accepts up to 15 digits, then left-shifts by up to 40 bits with no
overflow check. `--size 16777217t` (16.7 million exabytes, obvious garbage) wraps around 64-bit
arithmetic to exactly `2^40`, which is `1.0T`, which is a *valid* size, so it is accepted silently:

```
$ doos get_or_create --size 16777217t sizetest2
created sizetest2
$ doos list
NAME       STATE    NET    IMAGE  SIZE  AGE
sizetest2  stopped  slirp  host   1.0T  0s
```

Reproduced against `build/doos` at the time of this review. The same shift pattern exists for `k`/`m`/`g`
too, just with smaller multipliers that need more digits to wrap; `t` wraps with an 8-digit input. Every
caller does bound-check the *result*, which is why most overflowing inputs are still rejected (the
earlier example, `16777217t`+1 lands outside range and throws): it is specifically the inputs that
wrap into the valid `[64m, 1t]` window that go through unnoticed. Fix: check `v > (kMaxSizeBytes >>
shift)` before shifting, or reject more than 4-5 digits before a `t`/`g` suffix.

### 2.2 [LOW] Two different "no PATH lookup" strategies for external helpers, not one

`spec.hpp:52-53,75-76` hardcode `/usr/bin/newuidmap`, `/usr/bin/newgidmap`, `/usr/bin/slirp4netns`,
`/usr/bin/pasta` as single absolute paths. `resolveBinary()` (`src/util.cpp:397-417`) instead checks a
state-dir override then a list of six system directories, and is used for `curl`, `tar`, `sha256sum`
and `gpg`. Both avoid a `PATH`-poisoning attack (the actual goal, and both reach it), but the first
group breaks if a distribution installs those four tools somewhere other than exactly `/usr/bin` (for
example Fedora's `/usr/sbin` for some setuid helpers, or a non-standard prefix), while the second group
would still find them. This is not a security gap, it is an inconsistency that will show up as a
confusing "helper not found" on a distro the project has not been tested on, where the real fix (run
`resolveBinary()` for all five, or document the four as intentionally pinned) is a few lines either way.

### 2.3 [LOW] Catalog YAML/JSON parsing is hand-rolled string search, not a parser

`src/image.cpp:160-173` (Alpine's `latest-releases.yaml`) and `src/image.cpp:192-204` (Debian's
`index.json`, including a base64-embedded manifest) are read with `line.find(...)` and fixed byte
offsets, not a YAML/JSON library. This keeps the dependency count at zero, which fits the project's
"fewer moving parts" rule, and it is already tested against the real Alpine and Debian endpoints in
`tests/run.sh`. The risk is narrow but real: if either publisher reformats their file (reorders keys,
changes indentation, wraps a line), the parser fails closed with a thrown error (good: `docs/RESEARCH.md`
already calls this an accepted trade-off for Debian specifically) rather than silently extracting the
wrong artifact. Worth a one-line gap entry next to the code, same as the project's own convention asks.

### 2.4 [LOW] `Sandbox::join`'s ordering is a correctness invariant that is not stated in code

`src/sandbox.cpp:521-581`. Joining a running container does six `setns()` calls in a plain (not yet
seccomp-filtered) forked process, then forks again and *only there* calls `applyLimitsAndEnv`, which
installs the seccomp filter that denies `setns` (`src/hardening.cpp:184-186`). This is correct and
I could not find a bug in it, but the correctness depends entirely on hardening being applied after
the `setns` loop and in the right (inner) fork, a constraint that lives only in the order of statements,
not in a comment or an assertion. A future edit that moves `applyLimitsAndEnv` earlier "to simplify"
would silently break every second `attach`, and the e2e suite's "second attach joins" case would need
to be the thing that catches it rather than a comment telling the next editor not to do that.

### 2.5 [LOW] No CI, no license file

`find . -iname "*.yml"` turns up nothing; there is no `.github/workflows`, so the 88-check e2e suite,
`make audit` and `make valgrind` only run when someone remembers to run them by hand. There is also no
`LICENSE` file, which for a "commercial and scientific" project (the user's own standing rule) means
the legal default is "all rights reserved" by anyone who is not the author, worth a deliberate choice
rather than an omission.

### 2.6 [INFO] Everything else I checked was either already documented or held up

I specifically went looking for bugs in the capability-drop/seccomp-install ordering relative to
`execve`, in the overlayfs mount option construction, in the `--size`/`--memory`/`--pids`/`--cpus`
range validation (other than 2.1), in the container/image name validators against path traversal, in
`Store::remove`'s delete-outside-the-store guard, and in the symlink handling added for the PATH and
TOCTOU fixes (`src/util.cpp:329-354`, `src/sandbox.cpp:130-153`). All of it matched what the comments
claim, and the specific "mkdirs follows a symlink" class of bug this project itself found via
`audit/CVE-2021-30465` is closed the right way (per-component `openat(O_NOFOLLOW)`), not patched over.

## 3. Grades

| Category | Grade | Why |
|---|---|---|
| Architecture & code organization | 8.5/10 | One class per responsibility, matches `docs/ARCHITECTURE.md` exactly; every constant in one file; consistent error style throughout 3,558 lines. Loses points only for 2.4's unstated invariant. |
| Correctness & input validation | 7/10 | Careful everywhere except one real bug (2.1). CLI parsing, name validation, bounds checks on memory/pids/cpus are all solid; size parsing is the one spot that was not. |
| Isolation design (namespaces, overlayfs, mounts) | 9/10 | TOCTOU-safe symlink handling, `O_NOFOLLOW` directory creation, correct remount-flag preservation, correct setns/seccomp ordering. This is the part of the codebase that most clearly got the most attention, and it shows. |
| Resource controls (cgroup, size cap) | 8.5/10 | Adaptive scan-duty throttling, freeze/thaw hysteresis, whole-container OOM kill, hard caps on scan size and tree depth. Honestly scoped: documented as advisory without cgroup v2 delegation rather than pretending to enforce. |
| Supply chain (image verification) | 8/10 | Real signature verification against pinned fingerprints for two of three catalog sources, sha256 for all three, URL inputs refused without a hash. Loses points for the hand-rolled parsers (2.3) and for Debian having no publisher signature (disclosed, not doos's fault, but still the weakest link). |
| Hardening (capabilities, seccomp) | 8/10 | Matches Docker's default capability mask (measured, not assumed), denylist seccomp with the known evasions (x32, clone3, nested userns) explicitly closed. A denylist is inherently weaker than an allowlist by design, which the project says itself in `COMPARISON.md`; grade reflects execution of the chosen design, not the design choice. |
| Testing & verification tooling | 8.5/10 | 88 end-to-end checks through the real path (real overlayfs, real downloads, real cgroups), plus `audit/` (18 named CVEs with working reproducers) and `valgrind/` (6 checks, suppressions individually justified) are well above what a project this size usually has. One machine, one architecture, no CI (2.5) is the honest ceiling. |
| Documentation | 9/10 | Seven cross-linked documents that state what exists today, with gaps listed next to the code that closes them; generated numbers and charts instead of hand-typed ones; a changelog that actually matches releases. Among the best-documented small C++ projects I have reviewed. |
| Build & release automation | 8.5/10 | `scripts/release.sh` bumps, builds, tests, checks docs, commits and tags in one gated command and aborts on any failure; hardened compiler/linker flags. Loses half a point for no CI to run any of it automatically. |
| CLI/UX | 7.5/10 | Clear, narrow-terminal-friendly usage text; sensible fixed-vs-changeable option model; good error messages that say what to do next. `--i-understand` as a deliberate confirmation gate for reduced sandboxing is a good pattern. |
| Project hygiene | 6/10 | No LICENSE, no CI (2.5); otherwise clean (`.gitignore` is thorough, no stray build artifacts tracked, commit messages are consistent and attributed). |

**Overall: 8/10.** A small, honest, carefully built tool with one real bug found by testing it rather
than reading it, and a short, specific list of things worth fixing. Nothing found here changes the
conclusion already in `COMPARISON.md`: fine for your own tools, not a substitute for an audited,
multi-tenant-hardened runtime.

## 4. Recommended order of fixes

1. Fix the `parseSize` overflow (2.1): cheapest fix, only confirmed bug, trivial to test.
2. Add a one-line comment above `applyLimitsAndEnv`'s two call sites in `sandbox.cpp` stating the
   ordering invariant from 2.4, so the next edit does not have to rediscover it the hard way.
3. Decide and document the helper-resolution split (2.2): either move the four pinned paths through
   `resolveBinary()`, or add one line to `SPEC.md` saying why they are intentionally narrower.
4. Add a LICENSE file (2.5); pick whichever the user already uses elsewhere.
5. CI is the only item here that is genuinely more than a few lines of work (namespaces, cgroup v2
   delegation and AppArmor all need to exist on the runner); worth a separate, explicit decision
   rather than a drive-by addition.

**Docs:** [README](README.md) · [SPEC](SPEC.md) · [ARCHITECTURE](docs/ARCHITECTURE.md) ·
[RESEARCH](docs/RESEARCH.md) · [COMPARISON](COMPARISON.md) · [AGENTS](AGENTS.md) ·
[CHANGELOG](CHANGELOG.md) · [audit](audit/README.md) · [valgrind](valgrind/README.md) ·
[review.md](review.md) (earlier security-only review)