fix: regressions of the 1.1.21–1.1.25 hardening, invalid-JSON emitters, FwUsers GC, small correctness batch (1.1.27)

CHANGELOG.md

@@ -6,6 +6,82 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ---
 
+## [1.1.27] — 2026-09-14
+
+**Regressions introduced by the 1.1.21–1.1.25 hardening, found by an
+adversarial self-review, plus two invalid-JSON emitters, the `FwUsers`
+GC gap, and a small correctness batch.**
+
+Regressions of our own fixes:
+
+- **`cron/user` rejected legitimate dotted usernames — `lib/run_pure.cpp`
+  (MED).** 1.1.22's `validateCronUser` allowed `[A-Za-z0-9_-]` only, but
+  FreeBSD `pw(8)` accepts `.` in login names — a spec with
+  `user: john.doe` (common on first.last hosts) aborted `crate run`. `.`
+  is now allowed (shell-inert; `/` excluded and `.`/`..` reserved, so no
+  traversal). The 1.1.22 comment "empty is accepted unchanged (caller's
+  default handling)" was also **false** — no such handling existed (the
+  `root` default lives in the Spec struct initializer), so an explicit
+  `user: ""` reached `/var/cron/tabs/` (a directory). `run.cpp` now maps
+  empty → `root` before validating; the validator rejects empty.
+
+- **`validateStackIp` never checked the CIDR suffix — `lib/stack_pure.cpp`
+  (LOW-MED).** 1.1.25 validated only the address part, so `10.0.0.5/999`,
+  `10.0.0.5/abc` and `10.0.0.5/` all passed (the charset admits `/` and
+  hex letters). Shell-inert, but garbage for `/etc/hosts`. A `/` now
+  requires a bare decimal prefix within the family's range.
+
+- **`crate migrate` refused artifacts its own server produced —
+  `lib/migrate_pure.cpp` (LOW).** 1.1.21's `validateArtifactFile` rejected
+  any `..` substring, stricter than the daemon's `validateArtifactName`
+  and the jail-name validators, which allow `.` freely — a container
+  named `app..v2` exported fine and then failed to migrate. With `/`
+  excluded a single component cannot traverse, so the check bought
+  nothing; dropped (exact `.`/`..` stay reserved).
+
+- **Trailing-slash prefix in `datasetOwned`/`pathOwned` —
+  `lib/privops_authz_pure.cpp` (LOW, latent).** Same bug class fixed in
+  `Util::safePath` in 1.1.25: a prefix already ending in `/` made the
+  separator check look at the child's first char and reject every
+  descendant. Both now share one guarded helper.
+
+- CHANGELOG correction: the 1.1.25 entry said `validateStackName`
+  enforces "no `..`" — it reserves only the exact `.`/`..`; `a..b` is a
+  legal single component (and is asserted so by `stack_test`).
+
+Other fixes:
+
+- **Two invalid-JSON emitters.** `hub/scheduling_pure.cpp` `jsonQuote`
+  wrote `"\u" << std::hex << c` with no width/fill (byte 0x01 → `\u1`,
+  and the stream stayed in hex mode), and compared a signed `char` so
+  every UTF-8 byte ≥ 0x80 was "escaped". `daemon/routes.cpp`'s container
+  log endpoint escaped only `" \ \n \r \t`, so an ESC from a colour code
+  went out raw. Both now emit `\u00XX` for all control bytes.
+
+- **`FwUsers` had no dead-pid garbage collection — `lib/ctx.cpp`,
+  `lib/clean.cpp` (MED).** `FwSlots` always GC'd; `FwUsers` did not, and
+  `crate clean`'s comment claimed it did while only locking/unlocking. A
+  `crate run` killed without teardown (SIGKILL/OOM/panic) left its pid
+  in `ctx-fw-users`, `isEmpty()` was never true again, and the shared NAT
+  rule + `net.inet.ip.forwarding` were never restored. New
+  `FwUsers::garbageCollect` (mirrors `FwSlots`), called from `del()` so
+  the teardown's `isEmpty()` sees only live users, and explicitly from
+  `crate clean`.
+
+- **Small correctness batch.** `Util::Fs::writeFile(data, fd)` no longer
+  closes the *caller's* fd on error (callers pass their flock'd context
+  fd and close it themselves → double close). `S_ISREG`/`S_ISDIR` instead
+  of `st_mode & S_IF*` (S_IFDIR's bits are a subset of S_IFSOCK's, so a
+  UNIX socket under the chroot matched as a directory and
+  `directory_iterator` threw out of `findElfFiles`, aborting
+  `crate create`). `gmtime_r` instead of `gmtime` in the snapshot route
+  (static buffer shared across worker threads). `socat` proxy `fork()`
+  failure now errors instead of silently not starting the proxy.
+
+Tests updated/added in `run_pure_test`, `stack_test`, `migrate_pure_test`,
+`privops_authz_pure_test`. The `ctx`/`clean`/`util`/`routes` changes are
+runtime-only, compile-gated by the FreeBSD lite build.
+
 ## [1.1.26] — 2026-09-14
 
 **Correctness: seven daemon/runtime fixes from a correctness-lens audit

TODO

@@ -133,35 +133,30 @@ and best validated on a live FreeBSD host):
   Fix shape: register the RunAtEnd immediately after each successful
   acquisition, before the next fallible step.
 
-* (correctness, MED) lib/ctx.cpp FwUsers has no dead-pid garbage
-  collection (FwSlots does, ctx.cpp:~200) and lib/clean.cpp:~120
-  claims to GC it but only locks/unlocks. A `crate run` killed without
-  teardown (SIGKILL/OOM/panic) leaves its pid in ctx-fw-users, so
-  FwUsers::isEmpty() is never true again and the shared NAT rule 50000
-  + net.inet.ip.forwarding are never restored (run_net.cpp:~488). Add
-  kill(pid,0)-based GC mirroring FwSlots and make clean.cpp call it.
+  [FIXED in 1.1.27] lib/ctx.cpp FwUsers dead-pid garbage collection:
+  FwUsers::garbageCollect (mirrors FwSlots), called from del() so the
+  teardown's isEmpty() sees only live NAT-rule users, and explicitly
+  from `crate clean` (which previously only locked/unlocked despite its
+  comment).
 
 * (robustness, MED/LOW) daemon/privops_listener.cpp nvlist_recv and
   daemon/control_socket.cpp read() block forever with no timeout, and
   both accept loops spawn one detached thread per connection with no
   cap — an idle client pins a thread + fd; N of them exhaust the
   daemon. Add a recv timeout (SO_RCVTIMEO) and a concurrency cap.
 
-* (LOW batch) daemon/routes.cpp import tmp file is <name>.crate.tmp.<pid>
-  — constant across threads, so two concurrent imports of one name
-  interleave writes; no fsync before rename. lib/util.cpp writeFile(data,
-  fd) closes the CALLER's fd on error (ctx.cpp then double-closes).
-  execPipelineImpl: fork() failure for child i>0 throws without reaping
-  children 0..i-1 (zombies). Capture loops treat read()==-1 as EOF
-  (EINTR) and jail_query.cpp ignores waitpid failure / decodes an
-  uninitialized status. lib/util.cpp ~:631/662 test `st_mode & S_IFREG/
-  S_IFDIR` instead of S_ISREG/S_ISDIR (a socket matches S_IFDIR →
-  directory_iterator throws out of findElfFiles). daemon/routes.cpp
-  gmtime() (static buffer) in worker threads. ws_console.cpp select()
-  with fds >= FD_SETSIZE. run.cpp hcThread joinable while execInJail
-  can throw on fork EAGAIN → std::terminate skips every RunAtEnd.
-  run_services.cpp ignores fork() failure for socat. stack.cpp
-  inet_pton results unchecked for ip_range.
+* (LOW batch — what is left after 1.1.27 fixed writeFile(fd) double-
+  close, S_ISREG/S_ISDIR, gmtime_r, and the socat fork check)
+  daemon/routes.cpp import tmp file is <name>.crate.tmp.<pid> —
+  constant across threads, so two concurrent imports of one name
+  interleave writes; no fsync before rename. execPipelineImpl: fork()
+  failure for child i>0 throws without reaping children 0..i-1
+  (zombies). Capture loops treat read()==-1 as EOF (EINTR) and
+  jail_query.cpp ignores waitpid failure / decodes an uninitialized
+  status. ws_console.cpp select() with fds >= FD_SETSIZE. run.cpp
+  hcThread joinable while execInJail can throw on fork EAGAIN →
+  std::terminate skips every RunAtEnd. stack.cpp inet_pton results
+  unchecked for ip_range.
 
 === High priority — blocking production use ===
 

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.26" << std::endl;
+        std::cout << "crate 1.1.27" << 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.26" << std::endl;
+          std::cout << "crate 1.1.27" << std::endl;
           exit(0);
         default:
           err("unsupported short option '%s'", argv[a]);

daemon/routes.cpp

@@ -417,11 +417,21 @@ static void handleContainerLogs(const httplib::Request &req, httplib::Response &
       std::ostringstream ss;
       ss << "{\"name\":\"" << jail->name << "\",\"log\":\"";
       for (char c : output) {
+        unsigned char uc = static_cast<unsigned char>(c);
         if (c == '"') ss << "\\\"";
         else if (c == '\\') ss << "\\\\";
         else if (c == '\n') ss << "\\n";
         else if (c == '\r') ss << "\\r";
         else if (c == '\t') ss << "\\t";
+        else if (c == '\b') ss << "\\b";
+        else if (c == '\f') ss << "\\f";
+        else if (uc < 0x20) {
+          // 1.1.27: any other control byte in a log line (ESC from
+          // colour codes, NUL, …) was emitted raw → invalid JSON.
+          char buf[8];
+          std::snprintf(buf, sizeof(buf), "\\u%04x", (int)uc);
+          ss << buf;
+        }
         else ss << c;
       }
       ss << "\"}";
@@ -723,7 +733,11 @@ static void handleCreateSnapshot(const httplib::Request &req, httplib::Response
     // Generate a timestamp-based name.
     auto t = ::time(nullptr);
     char buf[32];
-    std::strftime(buf, sizeof(buf), "auto_%Y-%m-%d_%H%M%S", ::gmtime(&t));
+    // 1.1.27: gmtime_r — gmtime() returns a static buffer shared by all
+    // threads; two concurrent snapshot POSTs garbled each other's name.
+    struct tm tmv{};
+    ::gmtime_r(&t, &tmv);
+    std::strftime(buf, sizeof(buf), "auto_%Y-%m-%d_%H%M%S", &tmv);
     snapName = buf;
   }
   auto reason = RoutesPure::validateSnapshotName(snapName);

docs/trust-model.md

@@ -4,7 +4,7 @@
 operators on one machine) and contributors extending the privileged
 surface.
 
-**Applies to:** 1.1.26 (rootless model + per-tenant authz series 1.1.12 →
+**Applies to:** 1.1.27 (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.26 means reasoning about who can reach
+Reasoning about isolation on 1.1.27 means reasoning about who can reach
 **privops**, not who can run `crate(1)`.
 
 ---

docs/trust-model.uk.md

@@ -4,7 +4,7 @@
 (кілька операторів на одній машині), і контрибʼютори, які розширюють
 привілейовану поверхню.
 
-**Стосується:** 1.1.26 (rootless-модель + серія per-tenant authz 1.1.12 →
+**Стосується:** 1.1.27 (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.26 — це міркувати про те, хто має доступ
+Міркувати про ізоляцію на 1.1.27 — це міркувати про те, хто має доступ
 до **privops**, а не хто може запустити `crate(1)`.
 
 ---

hub/scheduling_pure.cpp

@@ -19,8 +19,17 @@ std::string jsonQuote(const std::string &s) {
   std::ostringstream os;
   os << '"';
   for (char c : s) {
+    unsigned char uc = static_cast<unsigned char>(c);
     if (c == '"' || c == '\\') os << '\\' << c;
-    else if (c < 0x20)         os << "\\u" << std::hex << (int)(unsigned char)c;
+    else if (uc < 0x20) {
+      // 1.1.27: was `"\\u" << std::hex << (int)c` — no width/fill, so
+      // byte 0x01 became `\u1` (invalid JSON) and the stream was left in
+      // hex mode for everything after it. Also compare as unsigned:
+      // signed `char < 0x20` was true for every UTF-8 byte ≥ 0x80.
+      char buf[8];
+      std::snprintf(buf, sizeof(buf), "\\u%04x", (int)uc);
+      os << buf;
+    }
     else                       os << c;
   }
   os << '"';

lib/clean.cpp

@@ -120,10 +120,15 @@ bool cleanCrates(const Args &args) {
     // FwUsers: remove dead PIDs
     try {
       auto fwUsers = Ctx::FwUsers::lock();
-      // FwUsers internally does garbage collection on dead PIDs
-      if (!dryRun)
+      // 1.1.27: the old comment claimed "FwUsers internally does garbage
+      // collection" — it did not; this block only locked and unlocked,
+      // so stale pids from SIGKILLed runs kept the shared NAT rule alive
+      // forever. Now an explicit GC (lock() reads lazily, so this is
+      // what actually loads, prunes, and marks the file dirty).
+      if (!dryRun) {
+        fwUsers->garbageCollect();
         fwUsers->unlock();
-      else {
+      } else {
         std::cout << "  [dry-run] would clean stale firewall user entries" << std::endl;
         fwUsers->unlock();
       }

lib/ctx.cpp

@@ -73,8 +73,9 @@ void FwUsers::add(pid_t pid) {
 }
 
 void FwUsers::del(pid_t pid) {
-  if (!inMemory)
-    readIntoMemory();
+  // 1.1.27: load + GC dead pids first, so the isEmpty() the caller
+  // checks right after this reflects only LIVE users of the NAT rule.
+  garbageCollect();
   // 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.
@@ -103,6 +104,22 @@ void FwUsers::readIntoMemory() {
   inMemory = true;
 }
 
+void FwUsers::garbageCollect() {
+  if (!inMemory)
+    readIntoMemory();
+  // Mirror of FwSlots::garbageCollect: a pid that no longer exists
+  // cannot be using the NAT rule, whatever the file says.
+  auto it = pids.begin();
+  while (it != pids.end()) {
+    if (::kill(*it, 0) == -1 && errno == ESRCH) {
+      it = pids.erase(it);
+      changed = true;
+    } else {
+      ++it;
+    }
+  }
+}
+
 void FwUsers::writeToFile() const {
   // form the file content
   std::ostringstream ss;

lib/ctx.h

@@ -25,6 +25,13 @@ class FwUsers {
   bool isEmpty() const;
   void add(pid_t pid);
   void del(pid_t pid);
+  // 1.1.27: drop entries whose pid no longer exists (kill(pid,0) →
+  // ESRCH). FwSlots always had this; FwUsers did not, so a `crate run`
+  // killed without teardown (SIGKILL/OOM/panic) left its pid here
+  // forever, isEmpty() was never true again, and the shared NAT rule +
+  // net.inet.ip.forwarding were never restored. Loads the file first if
+  // needed; marks the file dirty only when something was removed.
+  void garbageCollect();
 private:
   static std::string file();
   void readIntoMemory();

lib/migrate_pure.cpp

@@ -183,8 +183,17 @@ std::string validateContainerName(const std::string &name) {
 // `curl -o` target and an `unlink()` target. Without validation a
 // hostile/compromised source could return `"../../../etc/cron.d/pwn"`
 // and get arbitrary file write AND delete on the migrating host. Treat
-// it as a single path COMPONENT: a plain filename, no slash, no `..`,
-// no control bytes.
+// it as a single path COMPONENT: a plain filename — no slash, not the
+// reserved "."/"..", no control bytes.
+//
+// 1.1.27: the 1.1.21 version also rejected ANY ".." substring. That was
+// stricter than the server that produces the name (TransferPure::
+// validateArtifactName and the jail-name validators allow '.' freely,
+// only the exact "."/".." are reserved), so a container legitimately
+// named e.g. `app..v2` exported fine server-side and then `crate migrate`
+// refused its own artifact. With '/' excluded a single component cannot
+// traverse whatever dots it contains, so the substring check bought
+// nothing. Dropped.
 std::string validateArtifactFile(const std::string &name) {
   if (name.empty()) return "artifact filename is empty";
   if (name.size() > 255) return "artifact filename longer than 255 chars";
@@ -196,8 +205,6 @@ std::string validateArtifactFile(const std::string &name) {
         static_cast<unsigned char>(c) == 0x7f)
       return "artifact filename contains a control character";
   }
-  if (name.find("..") != std::string::npos)
-    return "artifact filename must not contain '..'";
   return "";
 }
 

lib/privops_authz_pure.cpp

@@ -4,27 +4,38 @@
 
 namespace PrivOpsAuthzPure {
 
+namespace {
+
+// Shared body for datasetOwned / pathOwned: `value` is the prefix root
+// itself, or a slash-anchored descendant "<prefix>/...". Slash-anchored
+// so "<prefix>extra" does not pass as "<prefix>".
+//
+// 1.1.27: when the prefix ALREADY ends in '/', that separator has been
+// consumed by the prefix itself, so value[prefix.size()] is the first
+// char of the child name, not a '/'. The old unconditional check then
+// rejected every descendant — the same trailing-slash bug fixed in
+// Util::safePath in 1.1.25. Demand the separator only when the prefix
+// does not supply it.
+bool ownedUnder(const std::string &value, const std::string &prefix) {
+  if (prefix.empty()) return true;   // no per-user split → nothing to gate
+  if (value == prefix) return true;  // the prefix root itself
+  bool prefixEndsWithSep = prefix.back() == '/';
+  return value.size() > prefix.size()
+      && value.compare(0, prefix.size(), prefix) == 0
+      && (prefixEndsWithSep || value[prefix.size()] == '/');
+}
+
+} // anon
+
 bool datasetOwned(const std::string &dataset, const std::string &zfsPrefix) {
-  if (zfsPrefix.empty())
-    return true;                 // no per-user ZFS split → nothing to gate
-  if (dataset == zfsPrefix)
-    return true;                 // the prefix root itself
-  // Descendant: "<prefix>/...". Slash-anchored so "<prefix>extra"
-  // does not pass as "<prefix>".
-  return dataset.size() > zfsPrefix.size()
-      && dataset.compare(0, zfsPrefix.size(), zfsPrefix) == 0
-      && dataset[zfsPrefix.size()] == '/';
+  return ownedUnder(dataset, zfsPrefix);
 }
 
 bool pathOwned(const std::string &path, const std::string &pathPrefix) {
-  // Identical shape to datasetOwned — kept as a separate helper so each
-  // call-site reads as "is this PATH inside the per-user path prefix"
-  // rather than reusing the dataset spelling.
-  if (pathPrefix.empty())  return true;
-  if (path == pathPrefix)  return true;
-  return path.size() > pathPrefix.size()
-      && path.compare(0, pathPrefix.size(), pathPrefix) == 0
-      && path[pathPrefix.size()] == '/';
+  // Kept as a separate helper so each call-site reads as "is this PATH
+  // inside the per-user path prefix" rather than reusing the dataset
+  // spelling.
+  return ownedUnder(path, pathPrefix);
 }
 
 OwnerLookup nullLookup() {