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 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 · SPEC · ARCHITECTURE · RESEARCH · COMPARISON · AGENTS · CHANGELOG · audit · valgrind · review.md (earlier security-only review)