fix(security): cron/user injection, list JSON escaping, ctx erase UB, vmwrap jailPath (1.1.22)

CHANGELOG.md

@@ -6,6 +6,50 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ---
 
+## [1.1.22] — 2026-07-07
+
+**Security & robustness: four fixes from a fresh project-wide audit.**
+
+A second-pass audit (shell-out surfaces, the root daemon, the
+allocation/registry layer, and the remaining pure modules) surfaced
+four cheap, unambiguous defects, fixed here:
+
+- **`cron/user` injection (`lib/run.cpp`).** The crontab user from a
+  `.crate` spec was emitted, unvalidated, into a root-run file path
+  (`/var/cron/tabs/<user>`) and a `sh -c "chmod … <user>"` string. In
+  the legacy setuid build that gave an unprivileged spec author path
+  traversal (`../../etc/cron.d/pwn` → host root) and shell injection.
+  New `RunPure::validateCronUser` constrains it to a POSIX-ish username
+  (`[A-Za-z0-9_-]`, ≤32, no leading `-`) before use.
+
+- **Unescaped JSON in `crate list --json` (`lib/list_pure.cpp`).**
+  `renderJson` interpolated `name`/`hostname`/`ip`/`path`/`ports`/
+  `mounts` without escaping, while the sibling `DoctorPure::renderJson`
+  routes every string through `jsonEscape`. Those fields come from the
+  kernel's jail state, so a jail created by another tool with a `"` in
+  its hostname produced malformed/injectable JSON. Now escaped.
+
+- **Undefined behavior in `FwUsers::del` (`lib/ctx.cpp`).**
+  `pids.erase(pids.find(pid))` is UB when the pid is absent
+  (`erase(end())`) — reachable on a stale/truncated context file or a
+  double teardown. Switched to `erase(key)`, a safe no-op for a missing
+  key (matching `FwSlots::release`).
+
+- **Unvalidated `jailPath` in vm-wrap (`lib/vmwrap_pure.cpp`).**
+  `validateSpec` checked every field except `jailPath`, which is
+  emitted verbatim into `path = "…";` in the jail.conf fragment that
+  `jail -c -f` runs as root. New `validateJailPath` requires an absolute
+  path and rejects a double-quote or control byte (which would close the
+  value or inject a directive).
+
+New `run_pure_test`, `list_pure_test`, and `vmwrap_pure_test` cases
+cover each. Three higher-touch items from the same audit — the auth
+locality signal keyed on a client-supplied `REMOTE_ADDR` header
+(`daemon/auth.cpp`), the lease-file `flock`-on-renamed-inode race
+(`lib/network_lease*.cpp`), and the `socket_proxy/proxy` path confinement
+gap (`lib/run_services.cpp`) — are recorded for a follow-up as they touch
+the auth and concurrency paths and want on-hardware validation.
+
 ## [1.1.21] — 2026-07-07
 
 **Security: reject control-char / path-traversal injection in

TODO

@@ -42,6 +42,54 @@ unit suite can't provide, so they are NOT being fixed blind):
   daemon path gets on-hardware test coverage. Both need the manual
   full FreeBSD workflow (or a self-hosted runner) to validate.
 
+Deferred from the 2026-07 second-pass audit (its four clear, testable
+findings shipped as 1.1.22; these three remain because they touch the
+auth and concurrency paths and want on-hardware validation before
+shipping — do NOT fix blind):
+
+* (security, HIGH if TCP listener enabled) daemon/auth.cpp decides
+  connection locality — and thus full trust — from a header-map
+  lookup, `req.get_header_value("REMOTE_ADDR").empty()`, instead of
+  the connection itself. A UDS peer is treated as unauthenticated-but-
+  fully-trusted (bypasses bearer token AND pool ACL); the empty-string
+  signal is what a remote TCP client could shadow by sending its own
+  empty `REMOTE_ADDR:` header (cpp-httplib stores request headers in a
+  multimap; get_header_value returns the first match, which may be the
+  client's). Exploitability depends on the pkg-vendored cpp-httplib
+  version's handling of client-supplied reserved headers, which is not
+  in-repo. The daemon already runs SEPARATE TCP and UDS `httplib::Server`
+  instances (daemon/server.cpp), so the robust fix is to thread an
+  explicit `isUnixListener` flag into the auth path (or read the
+  non-spoofable `req.remote_addr` struct field), never a header lookup.
+  Also fix getClientId() (daemon/routes.cpp) which keys the rate-limit
+  bucket on the same header. Touches the auth path — validate against a
+  live TCP+UDS daemon before shipping.
+
+* (correctness/security, MED) lib/network_lease.cpp + network_lease6.cpp
+  take the exclusive flock on the lease FILE's inode (openLocked opens
+  effectivePath() and locks that fd), but writeAllAtomic publishes by
+  renaming a FRESH inode over the path. A second waiter that open()ed
+  the path before the rename ends up holding its lock on the stale,
+  now-unlinked inode and reads pre-update contents → two concurrent
+  `crate run` invocations by the same user can allocate the SAME IPv4/
+  IPv6 address and drop one lease row. Scoped to one tenant (per-user
+  lease file), not cross-tenant. Fix: hold the lock on a stable, never-
+  renamed lock file (e.g. `<path>.lock`), or re-open+re-stat after
+  acquiring and retry if the inode changed. Concurrency correctness is
+  not exercised by the pure unit suite — validate with a real
+  multi-process race on a FreeBSD host.
+
+* (security, MED) lib/run_services.cpp socket_proxy: the `share` loop
+  runs each socket path through Util::safePath, but the `proxy` loop
+  does not — `entry.jail` flows into `create_directories(J(jailParent))`
+  and a `UNIX-LISTEN:J(entry.jail)` socat bind, and `entry.host` into
+  `UNIX-CONNECT:`. A `..`-bearing jail path escapes the jail tree for a
+  root-owned dir/socket. The correct guard needs care: safePath("/") as
+  used by the share loop is itself weak (it canonicalizes but the prefix
+  check never rejects, and the return value is discarded), so this wants
+  an explicit `..`/traversal rejection reviewed against how J() prefixes
+  the path — not a blind copy of the share-loop call.
+
 === High priority — blocking production use ===
 
 (High-priority items above are now complete; the next batch is

cli/args.cpp

@@ -753,7 +753,7 @@ Args parseArguments(int argc, char** argv, unsigned &processed) {
         args.noColor = true;
         break;
       } else if (strEq(argv[a], "--version")) {
-        std::cout << "crate 1.1.21" << std::endl;
+        std::cout << "crate 1.1.22" << std::endl;
         exit(0);
       } else if (auto argShort = isShort(argv[a])) {
         switch (argShort) {
@@ -764,7 +764,7 @@ Args parseArguments(int argc, char** argv, unsigned &processed) {
           args.logProgress = true;
           break;
         case 'V':
-          std::cout << "crate 1.1.21" << std::endl;
+          std::cout << "crate 1.1.22" << std::endl;
           exit(0);
         default:
           err("unsupported short option '%s'", argv[a]);

docs/trust-model.md

@@ -4,7 +4,7 @@
 operators on one machine) and contributors extending the privileged
 surface.
 
-**Applies to:** 1.1.21 (rootless model + per-tenant authz series 1.1.12 →
+**Applies to:** 1.1.22 (rootless model + per-tenant authz series 1.1.12 →
 1.1.17 covering every privops verb that carries an operator-controlled
 ownership signal). For the ≤ 0.9.x setuid model and the migration, see
 [`rootless-migration.md`](rootless-migration.md).
@@ -62,7 +62,7 @@ surface; 1.0.0 removed the setuid bit (`Makefile`, comment at the
 operator and delegates privileged operations to crated(8)"*).
 
 The single-trust-domain property did **not** disappear — it relocated.
-Reasoning about isolation on 1.1.21 means reasoning about who can reach
+Reasoning about isolation on 1.1.22 means reasoning about who can reach
 **privops**, not who can run `crate(1)`.
 
 ---

docs/trust-model.uk.md

@@ -4,7 +4,7 @@
 (кілька операторів на одній машині), і контрибʼютори, які розширюють
 привілейовану поверхню.
 
-**Стосується:** 1.1.21 (rootless-модель + серія per-tenant authz 1.1.12 →
+**Стосується:** 1.1.22 (rootless-модель + серія per-tenant authz 1.1.12 →
 1.1.17 покриває кожен privops-верб з operator-controlled ownership-
 сигналом). Про ≤ 0.9.x setuid-модель і міграцію див.
 [`rootless-migration.md`](rootless-migration.md).
@@ -62,7 +62,7 @@ privops-сокета, ніколи admin-токен і ніколи Unix-сок
 privileged operations to crated(8)»*).
 
 Властивість «єдиний домен довіри» **не зникла** — вона переїхала.
-Міркувати про ізоляцію на 1.1.21 — це міркувати про те, хто має доступ
+Міркувати про ізоляцію на 1.1.22 — це міркувати про те, хто має доступ
 до **privops**, а не хто може запустити `crate(1)`.
 
 ---

lib/ctx.cpp

@@ -75,7 +75,12 @@ void FwUsers::add(pid_t pid) {
 void FwUsers::del(pid_t pid) {
   if (!inMemory)
     readIntoMemory();
-  pids.erase(pids.find(pid));
+  // 1.1.22: erase by key, not by iterator. pids.erase(pids.find(pid))
+  // is undefined behavior when pid is absent (find returns end()) —
+  // reachable on a stale/truncated context file or a double teardown.
+  // std::set::erase(key) is a safe no-op for a missing key (matches
+  // FwSlots::release which already uses erase(pid)).
+  pids.erase(int(pid));
   changed = true;
 }
 

lib/list_pure.cpp

@@ -3,25 +3,55 @@
 #include "list_pure.h"
 
 #include <algorithm>
+#include <cstdio>
 #include <iomanip>
 #include <ostream>
 #include <sstream>
 #include <string>
 
 namespace ListPure {
 
+// 1.1.22: JSON-escape string fields before interpolating them into the
+// output. name/hostname/path/ip come from the kernel's jail state (not
+// crate's own validators), so a jail created by another tool with a `"`
+// or control byte in its hostname/path would otherwise produce
+// malformed or injectable JSON. Same shape as DoctorPure::jsonEscape.
+static std::string jsonEscape(const std::string &s) {
+  std::ostringstream o;
+  for (unsigned char c : s) {
+    switch (c) {
+    case '"':  o << "\\\""; break;
+    case '\\': o << "\\\\"; break;
+    case '\n': o << "\\n";  break;
+    case '\r': o << "\\r";  break;
+    case '\t': o << "\\t";  break;
+    case '\b': o << "\\b";  break;
+    case '\f': o << "\\f";  break;
+    default:
+      if (c < 0x20) {
+        char buf[8];
+        std::snprintf(buf, sizeof(buf), "\\u%04x", (int)c);
+        o << buf;
+      } else {
+        o << (char)c;
+      }
+    }
+  }
+  return o.str();
+}
+
 void renderJson(std::ostream &out, const std::vector<Entry> &entries) {
   out << "[\n";
   for (size_t i = 0; i < entries.size(); i++) {
     auto &e = entries[i];
     out << "  {"
         << "\"jid\":" << e.jid
-        << ",\"name\":\"" << e.name << "\""
-        << ",\"hostname\":\"" << e.hostname << "\""
-        << ",\"ip\":\"" << e.ip << "\""
-        << ",\"path\":\"" << e.path << "\""
-        << ",\"ports\":\"" << e.ports << "\""
-        << ",\"mounts\":\"" << e.mounts << "\""
+        << ",\"name\":\"" << jsonEscape(e.name) << "\""
+        << ",\"hostname\":\"" << jsonEscape(e.hostname) << "\""
+        << ",\"ip\":\"" << jsonEscape(e.ip) << "\""
+        << ",\"path\":\"" << jsonEscape(e.path) << "\""
+        << ",\"ports\":\"" << jsonEscape(e.ports) << "\""
+        << ",\"mounts\":\"" << jsonEscape(e.mounts) << "\""
         << ",\"healthcheck\":" << (e.hasHealthcheck ? "true" : "false")
         << "}";
     if (i + 1 < entries.size()) out << ",";

lib/run.cpp

@@ -1800,6 +1800,11 @@ bool runCrate(const Args &args, int argc, char** argv, int &outReturnCode) {
       crontab << job.schedule << "\t" << job.command << std::endl;
     // Write crontab for the specified user (default: root)
     auto cronUser = spec.cronJobs[0].user; // use first job's user for the crontab file
+    // 1.1.22: cronUser is a spec field that flows into a root-run file
+    // path AND a `sh -c` string below — validate it as a username so it
+    // can't traverse (`../../etc/cron.d/pwn`) or inject shell metachars.
+    if (auto e = RunPure::validateCronUser(cronUser); !e.empty())
+      ERR("cron/user rejected: " << e)
     Util::Fs::writeFile(crontab.str(), J(STR("/var/cron/tabs/" << cronUser)));
     // Ensure correct ownership and permissions (cron requires 0600)
     Util::execCommand({"/usr/sbin/chroot", jailPath, "/bin/sh", "-c",

lib/run_pure.cpp

@@ -21,4 +21,18 @@ unsigned envOrDefault(const char *name, unsigned def) {
   try { return Util::toUInt(val); } catch (...) { return def; }
 }
 
+std::string validateCronUser(const std::string &user) {
+  if (user.empty()) return "";  // unchanged: caller's default handling
+  if (user.size() > 32) return "cron user is longer than 32 chars";
+  if (user.front() == '-') return "cron user must not start with '-'";
+  for (char c : user) {
+    bool ok = (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')
+           || (c >= '0' && c <= '9') || c == '_' || c == '-';
+    if (!ok)
+      return "cron user contains an invalid character "
+             "(allowed: [A-Za-z0-9_-])";
+  }
+  return "";
+}
+
 }

lib/run_pure.h

@@ -18,4 +18,13 @@ std::string argsToString(int argc, char **argv);
 // missing or unparseable value.
 unsigned envOrDefault(const char *name, unsigned def);
 
+// 1.1.22: validate a `cron/user` spec field before it is used to build
+// the crontab path (`/var/cron/tabs/<user>`) and a `sh -c "chmod ...
+// <user>"` string. Both are root-run in the setuid build, so an
+// unvalidated value gives path traversal (`../../etc/cron.d/pwn`) and
+// shell injection. Constrain to a POSIX-ish username: [A-Za-z0-9_-],
+// 1..32 chars, no leading '-'. Empty is accepted unchanged (caller's
+// existing default handling). Returns "" on success.
+std::string validateCronUser(const std::string &user);
+
 }

lib/vmwrap_pure.cpp

@@ -78,12 +78,27 @@ std::string validateRulesetNum(unsigned n) {
   return "";
 }
 
+std::string validateJailPath(const std::string &p) {
+  if (p.empty()) return "";  // empty -> defaultJailPath() ("/")
+  if (p.size() > 1024) return "jail path is longer than 1024 chars";
+  if (p.front() != '/') return "jail path must be absolute (start with '/')";
+  for (char c : p) {
+    if (c == '"')
+      return "jail path must not contain a double-quote";
+    if (static_cast<unsigned char>(c) < 0x20 ||
+        static_cast<unsigned char>(c) == 0x7f)
+      return "jail path contains a control character";
+  }
+  return "";
+}
+
 std::string validateSpec(const WrapSpec &s) {
   if (auto e = validateVmName(s.vmName);   !e.empty()) return e;
   if (auto e = validateJailName(s.jailName); !e.empty()) return e;
   if (auto e = validateDataset(s.dataset); !e.empty()) return e;
   if (auto e = validateTap(s.tap);         !e.empty()) return e;
   if (auto e = validateNmdm(s.nmdm);       !e.empty()) return e;
+  if (auto e = validateJailPath(s.jailPath); !e.empty()) return e;
   if (s.rulesetNum != 0)
     if (auto e = validateRulesetNum(s.rulesetNum); !e.empty()) return e;
   return "";

lib/vmwrap_pure.h

@@ -71,6 +71,13 @@ std::string validateNmdm(int n);
 // in WrapSpec; deriveRulesetNum() never returns 0).
 std::string validateRulesetNum(unsigned n);
 
+// 1.1.22: jail path emitted verbatim into `path = "<jailPath>";` in the
+// jail.conf fragment that `jail -c -f` runs as root. Empty is allowed
+// (defaults to "/"). A non-empty value must be an absolute path with no
+// double-quote (would close the quoted value) and no control byte
+// (a newline injects its own jail.conf directive). Length 1..1024.
+std::string validateJailPath(const std::string &p);
+
 // One-shot: walk the WrapSpec and return the first failure or "".
 std::string validateSpec(const WrapSpec &s);
 

tests/unit/list_pure_test.cpp

@@ -65,6 +65,25 @@ ATF_TEST_CASE_BODY(json_healthcheck_false)
 	ATF_REQUIRE(out.find("\"healthcheck\":false") != std::string::npos);
 }
 
+// 1.1.22: hostname/path come from the kernel's jail state, not crate's
+// validators. A `"` / control byte must be JSON-escaped, not emitted raw
+// (which would break out of the string or inject a key).
+ATF_TEST_CASE_WITHOUT_HEAD(json_escapes_quote_and_control);
+ATF_TEST_CASE_BODY(json_escapes_quote_and_control)
+{
+	auto e = mkEntry(1, "web");
+	e.hostname = "a\",\"evil\":\"x";   // tries to inject a JSON key
+	e.path = "line1\nline2\ttab";
+	auto out = renderJsonStr({e});
+	// The raw injection string must NOT appear verbatim.
+	ATF_REQUIRE(out.find("\"evil\"") == std::string::npos);
+	// The quote and control bytes are escaped.
+	ATF_REQUIRE(out.find("a\\\",\\\"evil") != std::string::npos);
+	ATF_REQUIRE(out.find("line1\\nline2\\ttab") != std::string::npos);
+	// No literal newline/tab leaked into the output.
+	ATF_REQUIRE(out.find("line1\nline2") == std::string::npos);
+}
+
 // ===================================================================
 // Table
 // ===================================================================
@@ -129,6 +148,7 @@ ATF_INIT_TEST_CASES(tcs)
 	ATF_ADD_TEST_CASE(tcs, json_one_entry);
 	ATF_ADD_TEST_CASE(tcs, json_separator_between_entries);
 	ATF_ADD_TEST_CASE(tcs, json_healthcheck_false);
+	ATF_ADD_TEST_CASE(tcs, json_escapes_quote_and_control);
 	ATF_ADD_TEST_CASE(tcs, table_empty);
 	ATF_ADD_TEST_CASE(tcs, table_header_present);
 	ATF_ADD_TEST_CASE(tcs, table_singular_plural);

tests/unit/run_pure_test.cpp

@@ -76,6 +76,33 @@ ATF_TEST_CASE_BODY(envOrDefault_overflow_returns_default)
 	::unsetenv("CRATE_TEST_ENV_BIG");
 }
 
+// --- 1.1.22: validateCronUser ---
+
+ATF_TEST_CASE_WITHOUT_HEAD(cron_user_typical_accepted);
+ATF_TEST_CASE_BODY(cron_user_typical_accepted)
+{
+	ATF_REQUIRE_EQ(RunPure::validateCronUser("root"), "");
+	ATF_REQUIRE_EQ(RunPure::validateCronUser("www-data"), "");
+	ATF_REQUIRE_EQ(RunPure::validateCronUser("user_1"), "");
+	ATF_REQUIRE_EQ(RunPure::validateCronUser(""), "");  // empty unchanged
+}
+
+ATF_TEST_CASE_WITHOUT_HEAD(cron_user_injection_rejected);
+ATF_TEST_CASE_BODY(cron_user_injection_rejected)
+{
+	// Path traversal into the host crontab dir.
+	ATF_REQUIRE(!RunPure::validateCronUser("../../etc/cron.d/pwn").empty());
+	ATF_REQUIRE(!RunPure::validateCronUser("a/b").empty());
+	ATF_REQUIRE(!RunPure::validateCronUser("..").empty());   // '.' not allowed
+	// Shell metacharacters (the value also lands in a `sh -c` string).
+	ATF_REQUIRE(!RunPure::validateCronUser("x; rm -rf /").empty());
+	ATF_REQUIRE(!RunPure::validateCronUser("x`id`").empty());
+	ATF_REQUIRE(!RunPure::validateCronUser("x$(id)").empty());
+	ATF_REQUIRE(!RunPure::validateCronUser("x y").empty());
+	ATF_REQUIRE(!RunPure::validateCronUser("-rf").empty());  // leading dash
+	ATF_REQUIRE(!RunPure::validateCronUser(std::string(33, 'a')).empty());
+}
+
 ATF_INIT_TEST_CASES(tcs)
 {
 	ATF_ADD_TEST_CASE(tcs, argsToString_empty);
@@ -86,4 +113,6 @@ ATF_INIT_TEST_CASES(tcs)
 	ATF_ADD_TEST_CASE(tcs, envOrDefault_set_garbage_returns_default);
 	ATF_ADD_TEST_CASE(tcs, envOrDefault_set_empty_returns_default);
 	ATF_ADD_TEST_CASE(tcs, envOrDefault_overflow_returns_default);
+	ATF_ADD_TEST_CASE(tcs, cron_user_typical_accepted);
+	ATF_ADD_TEST_CASE(tcs, cron_user_injection_rejected);
 }