diff --git a/CHANGELOG.md b/CHANGELOG.md index cc5891c..a9bd358 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/TODO b/TODO index e7d4f75..2fd84fa 100644 --- a/TODO +++ b/TODO @@ -133,13 +133,11 @@ 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 @@ -147,21 +145,18 @@ and best validated on a live FreeBSD host): 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 .crate.tmp. - — 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 .crate.tmp. — + 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 === diff --git a/cli/args.cpp b/cli/args.cpp index b2cac0e..5f313a6 100644 --- a/cli/args.cpp +++ b/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]); diff --git a/daemon/routes.cpp b/daemon/routes.cpp index bb4afe7..902db1d 100644 --- a/daemon/routes.cpp +++ b/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(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); diff --git a/docs/trust-model.md b/docs/trust-model.md index 8ad93f4..5777bd6 100644 --- a/docs/trust-model.md +++ b/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)`. --- diff --git a/docs/trust-model.uk.md b/docs/trust-model.uk.md index 3a26664..81b4845 100644 --- a/docs/trust-model.uk.md +++ b/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)`. --- diff --git a/hub/scheduling_pure.cpp b/hub/scheduling_pure.cpp index dfdac8f..31ea06e 100644 --- a/hub/scheduling_pure.cpp +++ b/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(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 << '"'; diff --git a/lib/clean.cpp b/lib/clean.cpp index dddb76f..2fea603 100644 --- a/lib/clean.cpp +++ b/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(); } diff --git a/lib/ctx.cpp b/lib/ctx.cpp index 62943c2..ab6ec0e 100644 --- a/lib/ctx.cpp +++ b/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; diff --git a/lib/ctx.h b/lib/ctx.h index e971fc4..71a7ce4 100644 --- a/lib/ctx.h +++ b/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(); diff --git a/lib/migrate_pure.cpp b/lib/migrate_pure.cpp index a908c52..1c159d8 100644 --- a/lib/migrate_pure.cpp +++ b/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(c) == 0x7f) return "artifact filename contains a control character"; } - if (name.find("..") != std::string::npos) - return "artifact filename must not contain '..'"; return ""; } diff --git a/lib/privops_authz_pure.cpp b/lib/privops_authz_pure.cpp index dc4549e..0e468da 100644 --- a/lib/privops_authz_pure.cpp +++ b/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 "/...". Slash-anchored +// so "extra" does not pass as "". +// +// 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: "/...". Slash-anchored so "extra" - // does not pass as "". - 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() { diff --git a/lib/run.cpp b/lib/run.cpp index 14b5b58..f562fb3 100644 --- a/lib/run.cpp +++ b/lib/run.cpp @@ -1809,6 +1809,10 @@ 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.27: an explicit `user: ""` in the spec used to reach + // `/var/cron/tabs/` (a directory) — the "root" default only lives in + // the Spec struct initializer. Apply the documented default here. + if (cronUser.empty()) cronUser = "root"; // 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. diff --git a/lib/run_pure.cpp b/lib/run_pure.cpp index 786ba38..bc1e856 100644 --- a/lib/run_pure.cpp +++ b/lib/run_pure.cpp @@ -22,15 +22,25 @@ unsigned envOrDefault(const char *name, unsigned def) { } std::string validateCronUser(const std::string &user) { - if (user.empty()) return ""; // unchanged: caller's default handling + // 1.1.27: empty is now rejected here — the 1.1.22 version waved it + // through claiming "caller's default handling", but 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. + if (user.empty()) return "cron user is empty"; if (user.size() > 32) return "cron user is longer than 32 chars"; if (user.front() == '-') return "cron user must not start with '-'"; + if (user == "." || user == "..") return "cron user is reserved"; for (char c : user) { + // 1.1.27: '.' is allowed — FreeBSD pw(8) accepts it in login names + // (`john.doe` is common on first.last hosts) and 1.1.22 wrongly + // rejected such specs. It is shell-inert and, with '/' excluded and + // "."/".." reserved above, cannot traverse. bool ok = (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') - || (c >= '0' && c <= '9') || c == '_' || c == '-'; + || (c >= '0' && c <= '9') || c == '_' || c == '-' || c == '.'; if (!ok) return "cron user contains an invalid character " - "(allowed: [A-Za-z0-9_-])"; + "(allowed: [A-Za-z0-9._-])"; } return ""; } diff --git a/lib/run_pure.h b/lib/run_pure.h index 337e3ad..f90910a 100644 --- a/lib/run_pure.h +++ b/lib/run_pure.h @@ -22,9 +22,11 @@ unsigned envOrDefault(const char *name, unsigned def); // the crontab path (`/var/cron/tabs/`) and a `sh -c "chmod ... // "` 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. +// shell injection. Constrain to a POSIX-ish username: [A-Za-z0-9._-] +// (1.1.27: '.' allowed, as FreeBSD pw(8) does), 1..32 chars, no leading +// '-', not "."/"..". Empty is REJECTED (1.1.27) — the caller maps an +// empty spec value to the documented default "root" before calling. +// Returns "" on success. std::string validateCronUser(const std::string &user); } diff --git a/lib/run_services.cpp b/lib/run_services.cpp index 8b98229..7356a1b 100644 --- a/lib/run_services.cpp +++ b/lib/run_services.cpp @@ -104,6 +104,10 @@ RunAtEnd setupSocketProxy(const Spec &spec, const std::string &jailPath, bool lo auto jailParent = entry.jail.substr(0, entry.jail.rfind('/')); std::filesystem::create_directories(J(jailParent)); pid_t pid = ::fork(); + // 1.1.27: a fork() failure was silently dropped (pid < 0 fell through + // to "not started") — the proxy simply never existed, with no error. + if (pid < 0) + ERR("fork failed for socat socket proxy '" << entry.jail << "'") if (pid == 0) { ::execl(CRATE_PATH_SOCAT, "socat", STR("UNIX-LISTEN:" << J(entry.jail) << ",fork").c_str(), diff --git a/lib/stack_pure.cpp b/lib/stack_pure.cpp index ca6de09..3a3802e 100644 --- a/lib/stack_pure.cpp +++ b/lib/stack_pure.cpp @@ -92,12 +92,30 @@ std::string validateStackIp(const std::string &ip) { if (!ok) return "address contains an invalid character"; } - auto bare = ipFromCidr(ip); + auto slash = ip.find('/'); + auto bare = (slash == std::string::npos) ? ip : ip.substr(0, slash); struct in_addr a4; struct in6_addr a6; - if (::inet_pton(AF_INET, bare.c_str(), &a4) == 1) return ""; - if (::inet_pton(AF_INET6, bare.c_str(), &a6) == 1) return ""; - return "address is neither a valid IPv4 nor IPv6 literal"; + long maxPrefix; + if (::inet_pton(AF_INET, bare.c_str(), &a4) == 1) maxPrefix = 32; + else if (::inet_pton(AF_INET6, bare.c_str(), &a6) == 1) maxPrefix = 128; + else return "address is neither a valid IPv4 nor IPv6 literal"; + // 1.1.27: the 1.1.25 version 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 gate admits '/' and hex letters). Shell-inert, but garbage + // for /etc/hosts. Require a bare decimal prefix within the family's + // range when a '/' is present. + if (slash != std::string::npos) { + auto suffix = ip.substr(slash + 1); + if (suffix.empty() || suffix.size() > 3) return "CIDR prefix is malformed"; + long p = 0; + for (char c : suffix) { + if (c < '0' || c > '9') return "CIDR prefix must be numeric"; + p = p * 10 + (c - '0'); + } + if (p > maxPrefix) return "CIDR prefix is out of range for the address family"; + } + return ""; } // topoSort is templated and lives in stack_pure.h. diff --git a/lib/util.cpp b/lib/util.cpp index 81d1c7a..91f45ea 100644 --- a/lib/util.cpp +++ b/lib/util.cpp @@ -518,13 +518,15 @@ size_t getFileSize(int fd) { } void writeFile(const std::string &data, int fd) { + // 1.1.27: do NOT close the caller's fd on error. Callers (FwUsers / + // FwSlots / GuiRegistry::unlock) pass their flock'd context fd and + // close it themselves; closing it here meant a double close — EBADF + // at best, and in the multithreaded daemon closing an fd number + // another thread had just been handed. auto res = ::write(fd, data.c_str(), data.size()); if (res == -1) { - auto err = STR("failed to write file: " << strerror(errno)); - (void)::close(fd); - ERR2("write file", err) + ERR2("write file", "failed to write file: " << strerror(errno)) } else if (res != (int)data.size()) { - (void)::close(fd); ERR2("write file", "short write in file, attempted to write " << data.size() << " bytes, actually wrote only " << res << " bytes") } } @@ -646,7 +648,7 @@ bool isXzArchive(const char *file) { if (res == -1) return false; // can't stat: can't be an XZ archive file - if (sb.st_mode & S_IFREG && sb.st_size > 0x100) { // the XZ archive file can't be too small + if (S_ISREG(sb.st_mode) && sb.st_size > 0x100) { // the XZ archive file can't be too small (1.1.27: S_ISREG, not a bitmask test) uint8_t signature[5]; // read the signature int fd = ::open(file, O_RDONLY); @@ -676,15 +678,18 @@ char isElfFileOrDir(const std::string &file) { // find if the file is a regular return 'N'; // ? what else to do after the above } - // directory? - if (sb.st_mode & S_IFDIR) + // directory? 1.1.27: S_ISDIR, not `& S_IFDIR` — S_IFDIR (0040000) is a + // subset of S_IFSOCK's bits (0140000), so a UNIX socket under the + // chroot matched as a directory and directory_iterator then threw a + // filesystem_error out of findElfFiles, aborting `crate create`. + if (S_ISDIR(sb.st_mode)) return 'D'; // object files aren't dynamic ELFs if (file.size() > 2 && file[file.size()-1] == 'o' && file[file.size()-2] == '.') return 'N'; - if (sb.st_mode & S_IFREG /*&& sb.st_mode & S_IXUSR*/ && sb.st_size > 0x80) { // this reference claims that ELF can be as small as 142 bytes: http://timelessname.com/elfbin/ + if (S_ISREG(sb.st_mode) /*&& sb.st_mode & S_IXUSR*/ && sb.st_size > 0x80) { // this reference claims that ELF can be as small as 142 bytes: http://timelessname.com/elfbin/ // x-bit is disabled above: some .so files have no exec bit, particularly /usr/lib/pam_*.so uint8_t signature[4]; // read the signature diff --git a/tests/unit/migrate_pure_test.cpp b/tests/unit/migrate_pure_test.cpp index 0f4be76..eb7dd4e 100644 --- a/tests/unit/migrate_pure_test.cpp +++ b/tests/unit/migrate_pure_test.cpp @@ -102,7 +102,11 @@ ATF_TEST_CASE_BODY(artifact_file_traversal_rejected) { ATF_REQUIRE(!validateArtifactFile("../../etc/cron.d/pwn").empty()); ATF_REQUIRE(!validateArtifactFile("sub/dir/file").empty()); ATF_REQUIRE(!validateArtifactFile("/etc/passwd").empty()); - ATF_REQUIRE(!validateArtifactFile("a..b").empty()); // any ".." substring + // 1.1.27: an embedded ".." is a legal filename character sequence — + // the server-side validators accept `app..v2`, and with '/' excluded + // a single component cannot traverse. Only the exact "."/".." are + // reserved (asserted above). + ATF_REQUIRE_EQ(validateArtifactFile("app..v2-1700000000.crate"), std::string()); ATF_REQUIRE(!validateArtifactFile("bad\nname").empty()); ATF_REQUIRE(!validateArtifactFile("bad\tname").empty()); ATF_REQUIRE(!validateArtifactFile(std::string(256, 'a')).empty()); diff --git a/tests/unit/privops_authz_pure_test.cpp b/tests/unit/privops_authz_pure_test.cpp index 809223a..e691fe0 100644 --- a/tests/unit/privops_authz_pure_test.cpp +++ b/tests/unit/privops_authz_pure_test.cpp @@ -56,6 +56,23 @@ ATF_TEST_CASE_BODY(dataset_owned_empty_prefix_allows_all) { ATF_REQUIRE(datasetOwned("", "")); } +// 1.1.27: a prefix that already ends in '/' must still accept its +// descendants (the separator is consumed by the prefix itself) and must +// still reject siblings — the trailing-slash bug class fixed in +// Util::safePath in 1.1.25. +ATF_TEST_CASE_WITHOUT_HEAD(owned_trailing_slash_prefix); +ATF_TEST_CASE_BODY(owned_trailing_slash_prefix) { + const std::string p = "zroot/crate-tenants/1000/"; + ATF_REQUIRE(datasetOwned("zroot/crate-tenants/1000/web", p)); + ATF_REQUIRE(datasetOwned("zroot/crate-tenants/1000/web/data", p)); + ATF_REQUIRE(datasetOwned(p, p)); + ATF_REQUIRE(!datasetOwned("zroot/crate-tenants/10001/web", p)); + ATF_REQUIRE(!datasetOwned("zroot/crate-tenants/1001/web", p)); + const std::string pp = "/var/run/crate/1000/"; + ATF_REQUIRE(pathOwned("/var/run/crate/1000/jail-a", pp)); + ATF_REQUIRE(!pathOwned("/var/run/crate/10001/jail-a", pp)); +} + // --- authorize: dataset verbs --- ATF_TEST_CASE_WITHOUT_HEAD(authorize_attach_zfs_own_dataset); @@ -508,6 +525,7 @@ ATF_TEST_CASE_BODY(decision_reason_non_empty) { } ATF_INIT_TEST_CASES(tcs) { + ATF_ADD_TEST_CASE(tcs, owned_trailing_slash_prefix); ATF_ADD_TEST_CASE(tcs, dataset_owned_prefix_and_descendants); ATF_ADD_TEST_CASE(tcs, dataset_owned_rejects_foreign_and_substring); ATF_ADD_TEST_CASE(tcs, dataset_owned_empty_prefix_allows_all); diff --git a/tests/unit/run_pure_test.cpp b/tests/unit/run_pure_test.cpp index 0d41443..d810c6f 100644 --- a/tests/unit/run_pure_test.cpp +++ b/tests/unit/run_pure_test.cpp @@ -84,7 +84,10 @@ 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 + // 1.1.27: '.' is legal in FreeBSD login names (pw(8)); 1.1.22 wrongly + // rejected first.last-style users. + ATF_REQUIRE_EQ(RunPure::validateCronUser("john.doe"), ""); + ATF_REQUIRE_EQ(RunPure::validateCronUser("svc.backup-2"), ""); } ATF_TEST_CASE_WITHOUT_HEAD(cron_user_injection_rejected); @@ -93,7 +96,11 @@ 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 + ATF_REQUIRE(!RunPure::validateCronUser("..").empty()); // reserved + ATF_REQUIRE(!RunPure::validateCronUser(".").empty()); // reserved + // 1.1.27: empty is rejected by the validator; run.cpp maps an empty + // spec value to the documented default "root" BEFORE validating. + ATF_REQUIRE(!RunPure::validateCronUser("").empty()); // 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()); diff --git a/tests/unit/stack_test.cpp b/tests/unit/stack_test.cpp index ba59c7c..5314305 100644 --- a/tests/unit/stack_test.cpp +++ b/tests/unit/stack_test.cpp @@ -129,6 +129,17 @@ ATF_TEST_CASE_BODY(validateStackIp_injection_rejected) ATF_REQUIRE(!validateStackIp("256.0.0.1").empty()); // charset ok, inet_pton fails ATF_REQUIRE(!validateStackIp("not-an-ip").empty()); ATF_REQUIRE(!validateStackIp("").empty()); + // 1.1.27: the CIDR suffix is validated too — 1.1.25 only checked the + // address part, so these all passed (shell-inert, but garbage for + // /etc/hosts). + ATF_REQUIRE(!validateStackIp("10.0.0.5/999").empty()); + ATF_REQUIRE(!validateStackIp("10.0.0.5/33").empty()); + ATF_REQUIRE(!validateStackIp("10.0.0.5/abc").empty()); + ATF_REQUIRE(!validateStackIp("10.0.0.5/").empty()); + ATF_REQUIRE(!validateStackIp("fd00::1/129").empty()); + ATF_REQUIRE_EQ(validateStackIp("10.0.0.5/32"), ""); + ATF_REQUIRE_EQ(validateStackIp("10.0.0.5/0"), ""); + ATF_REQUIRE_EQ(validateStackIp("fd00::1/128"), ""); } ATF_TEST_CASE_WITHOUT_HEAD(topoSort_empty);