diff --git a/.github/workflows/freebsd-build-lite.yml b/.github/workflows/freebsd-build-lite.yml index 251287a..17df49a 100644 --- a/.github/workflows/freebsd-build-lite.yml +++ b/.github/workflows/freebsd-build-lite.yml @@ -61,7 +61,54 @@ jobs: # (daemon/*.cpp #include ); ssl/crypto ship in # FreeBSD base. Added 1.1.21 so the daemon link is covered # in PR CI — see the "Link daemon binaries" step below. - sudo pkg install -y pkgconf yaml-cpp rang kyua gmake cpp-httplib + # + # Hard build deps — these MUST install or the job is dead. + sudo pkg install -y pkgconf yaml-cpp rang gmake cpp-httplib + + # ATF link libraries. The Makefile links every unit test with + # `-L/usr/local/lib -latf-c++ -latf-c`. FreeBSD 14.x base ships + # the HEADER (/usr/include/atf-c++.hpp) and kyua, but its ATF + # libraries are PRIVATE (/usr/lib/private/libprivateatf-*) and + # cannot be resolved as -latf-c++ — run 34233361523 compiled + # everything and then died at exactly that link step. So + # devel/atf (alive: 0.23 on quarterly) is a HARD dep, on its + # OWN line: bundling it with the dead kyua package made pkg + # abort the whole transaction and atf silently never installed. + sudo pkg install -y atf + + # kyua: the devel/kyua port was DELETED 2026-05-07 ("part of + # the base in all supported versions — Kyua's evolution + # happens in the base", D47473), which is what first broke + # this job (PR #228: "No packages available to install + # matching 'kyua'"). FreeBSD 14.x ships /usr/bin/kyua — verified + # present in this image. pkg is tried only for older images. + sudo pkg install -y kyua 2>/dev/null \ + || echo "kyua pkg gone (deleted 2026-05, lives in base) — using /usr/bin/kyua" + echo "--- kyua/ATF preflight ---" + command -v kyua || true + ls -la /usr/bin/kyua /usr/local/bin/kyua 2>/dev/null || true + ls -la /usr/include/atf-c++.hpp /usr/local/include/atf-c++.hpp 2>/dev/null || true + ls -la /usr/lib/libatf-c++* /usr/local/lib/libatf-c++* 2>/dev/null || true + # If neither base nor pkg has them, show what pkg DOES offer + # (a rename shows up here) and fail with a clear error. + if ! command -v kyua >/dev/null 2>&1; then + echo "pkg search for kyua/atf (to catch a package rename):" + pkg search -q kyua 2>/dev/null || true + pkg search -q atf 2>/dev/null || true + echo "::error::kyua not found in base or pkg — see search output above" + exit 1 + fi + if [ ! -f /usr/include/atf-c++.hpp ] && [ ! -f /usr/local/include/atf-c++.hpp ]; then + echo "::error::atf-c++.hpp not found in base or pkg" + exit 1 + fi + # The check that would have caught run 34233361523 up front: + # a header alone is not enough, the LINKABLE lib must exist. + ls -la /usr/local/lib/libatf-c++.so /usr/lib/libatf-c++.so 2>/dev/null || true + if [ ! -e /usr/local/lib/libatf-c++.so ] && [ ! -e /usr/lib/libatf-c++.so ]; then + echo "::error::linkable libatf-c++.so not found — devel/atf must be installed (base only has private copies)" + exit 1 + fi echo "::endgroup::" echo "::group::System info" diff --git a/.github/workflows/freebsd-build.yml b/.github/workflows/freebsd-build.yml index 9e81e9b..c4fdfdb 100644 --- a/.github/workflows/freebsd-build.yml +++ b/.github/workflows/freebsd-build.yml @@ -88,12 +88,56 @@ jobs: set -ex echo "::group::Install dependencies" - # `atf` ships /usr/local/bin/atf-sh, the interpreter for our - # functional test scripts (#!/usr/bin/env atf-sh). It is NOT - # pulled in as a kyua dependency, so an explicit install - # avoids the "Invalid header for test case list; got ''" - # broken result. - sudo pkg install -y pkgconf yaml-cpp rang kyua atf gmake cpp-httplib + # Hard build deps — these MUST install or the job is dead. + sudo pkg install -y pkgconf yaml-cpp rang gmake cpp-httplib + + # ATF: HARD dep, on its own line. It provides (a) the LINKABLE + # /usr/local/lib/libatf-c{,++}.so the Makefile's `-latf-c++ + # -latf-c` needs — base only ships PRIVATE copies + # (/usr/lib/private/libprivateatf-*) the linker cannot resolve, + # which is exactly where run 34233361523 died after compiling + # everything — and (b) atf-sh, the interpreter for our + # functional test scripts (#!/usr/bin/env atf-sh); without it + # kyua reports "Invalid header for test case list; got ''". + # devel/atf is alive (0.23, quarterly). Separate line on + # purpose: bundling it with the dead kyua package made pkg + # abort the whole transaction and atf silently never installed. + sudo pkg install -y atf + + # kyua: the devel/kyua port was DELETED 2026-05-07 ("part of + # the base in all supported versions — Kyua's evolution + # happens in the base", D47473) — the original PR #228 break + # ("No packages available to install matching 'kyua'"). + # FreeBSD 14.x ships /usr/bin/kyua. pkg tried only for older + # images; base is the real source. + sudo pkg install -y kyua 2>/dev/null \ + || echo "kyua pkg gone (deleted 2026-05, lives in base) — using /usr/bin/kyua" + echo "--- kyua/ATF preflight ---" + command -v kyua || true + command -v atf-sh || true + ls -la /usr/include/atf-c++.hpp /usr/local/include/atf-c++.hpp 2>/dev/null || true + if ! command -v kyua >/dev/null 2>&1; then + echo "pkg search for kyua/atf (to catch a package rename):" + pkg search -q kyua 2>/dev/null || true + pkg search -q atf 2>/dev/null || true + echo "::error::kyua not found in base or pkg — see search output above" + exit 1 + fi + if ! command -v atf-sh >/dev/null 2>&1; then + echo "::error::atf-sh not found in base or pkg (functional tests need it)" + exit 1 + fi + if [ ! -f /usr/include/atf-c++.hpp ] && [ ! -f /usr/local/include/atf-c++.hpp ]; then + echo "::error::atf-c++.hpp not found in base or pkg" + exit 1 + fi + # A header alone is not enough — the LINKABLE lib must exist + # (the check that would have caught run 34233361523 up front). + ls -la /usr/local/lib/libatf-c++.so /usr/lib/libatf-c++.so 2>/dev/null || true + if [ ! -e /usr/local/lib/libatf-c++.so ] && [ ! -e /usr/lib/libatf-c++.so ]; then + echo "::error::linkable libatf-c++.so not found — devel/atf must be installed (base only has private copies)" + exit 1 + fi echo "::endgroup::" echo "::group::System info" diff --git a/CHANGELOG.md b/CHANGELOG.md index 5918e90..2c3f00b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,84 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). --- +## [1.1.25] — 2026-07-11 + +**Security & robustness: five fixes from a third-pass audit of the +modules no earlier pass had covered (GUI/session, VM stack, lifecycle/ +runtime, audit/util/parsers).** + +- **Infinite loop in `getAllJails(crateOnly=true)` — `lib/jail_query.cpp` + (HIGH, DoS).** The `lastjid` cursor advance sat *after* the crateOnly + filter, so a non-crate jail hit `continue` without moving the cursor + and the next `jailparam_get` returned the very same jail forever — + 100% CPU the moment any foreign jail (bastille/pot/plain `jail(8)`) + coexisted with crate. That wedged every crateOnly caller — `crate + top/clean/doctor/info/list/console/stack` — **and the crated + control-socket jail listing**. No attacker input needed. The cursor + now advances before the filter. + +- **Command injection into a root `sh -c` via stack-file fields — + `lib/stack.cpp` (HIGH).** The container name, its static IP, and a + network's `gateway` were interpolated unescaped into `printf '…' >> + /etc/hosts` / `printf 'nameserver …' > /etc/resolv.conf` shell + fragments stored in `run:before-start-services` and executed by + `/bin/sh -c` as root. A container keyed `x';touch /tmp/pwned;'` ran + arbitrary commands as root on `crate stack up`. New + `StackPure::validateStackName` (`[A-Za-z0-9._-]`, ≤64, no leading `-`) + and `validateStackIp` (charset-gated `inet_pton`, CIDR tolerated) are + applied at parse time **and** re-applied as a sink-guard right where + each value enters the shell string. + +- **Path traversal via stack network name → root write + delete — + `lib/stack.cpp` (HIGH).** `confDir = dnsBaseDir()/dns-` + was `create_directories`'d, written (`unbound.conf`), passed to + `unbound -c`, and `remove_all`'d as root with an unvalidated YAML key. + A name of `../../../etc/cron.d` let root write a config under an + attacker-chosen path and recursively delete an attacker-chosen tree. + Closed by the same `validateStackName` (no `/`, no `..`). + +- **Predictable `/tmp` screenshot files → symlink attack — + `lib/gui.cpp` (HIGH when run as root).** + `/tmp/crate-screenshot-.{ppm,xwd}` (displayNum guessable, + allocation starts at 10) was opened via `fopen`/`xwd -out` with no + `O_EXCL`/`O_NOFOLLOW` in world-writable `/tmp`; `gui screenshot` runs + with root's EUID (the registry is root-only), so a local user could + pre-plant a symlink and have root truncate/overwrite an arbitrary + file (CWE-59). Scratch files now live in a private `mkdtemp(3)` + directory (0700, random name) removed on every exit path. + +- **`Util::safePath` over-rejected every path when the prefix ends in + `/` — `lib/util_pure.cpp` (MED, correctness).** The separator check + demanded `canonical[prefix.size()] == '/'`, but a prefix that already + ends in `/` (the root prefix `"/"` being the degenerate case) has + consumed that separator, so the index points at a filename char and + the check always failed. Only the separator is now demanded when the + prefix does not supply it. This silently made `socketProxy.share` + abort on every real socket path. + +- **`socketProxy` jail-side confinement — `lib/run_services.cpp`.** With + `safePath` fixed, the `share` loop's `safePath(sock, "/", …)` would + have passed everything (prefix `/` cannot confine, and its return was + discarded while the raw `..`-bearing path still reached `J()`), and + the `proxy` loop had no guard at all. Both now validate the + **concatenated** jail-side path stays under `jailPath` — the same + guard `run.cpp` applies to `dirsShare`. This closes the socket_proxy + item deferred in 1.1.22. (`proxy.host` is the operator's host-side + connect target and is deliberately not jail-confined.) + +New `stack_test` cases (name/IP injection + traversal rejection, clean +values accepted) and `util_security_test` cases (root prefix accepts, +trailing-slash prefix still rejects siblings). The `jail_query`, +`gui`, and `run_services` changes are runtime-only, compile-gated by +the FreeBSD build. + +Also noted, not changed: `lib/vm_spec.cpp` / `vm_run.cpp` / `vm_stack.cpp` +carry libvirt-XML injection and path-traversal sinks (`vmName`, +`vol.tag`, `disk`, `sharedBridge`) that would be HIGH if reachable — +but `createVm`/`parseVmOptions`/`generateDomainXml` have no callers +today. Recorded in `TODO` so they are hardened before that code is +ever wired up. + ## [1.1.24] — 2026-07-07 **Robustness: two low-severity fixes from the second-pass audit that diff --git a/TODO b/TODO index a2be3ae..da544ee 100644 --- a/TODO +++ b/TODO @@ -43,10 +43,11 @@ unit suite can't provide, so they are NOT being fixed blind): 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; the auth-locality finding was fixed in -1.1.23 — see below; these two remain because they touch the -concurrency/confinement paths and want on-hardware validation before -shipping — do NOT fix blind): +findings shipped as 1.1.22; auth-locality fixed in 1.1.23 and +socket_proxy confinement in 1.1.25 — see below; ONE item remains +because it is a concurrency race the pure unit suite cannot exercise +and wants a real multi-process test on a FreeBSD host — do NOT fix +blind): [FIXED in 1.1.23] daemon/auth.cpp connection-locality decided from a client-supplyable REMOTE_ADDR header. Replaced with a non-spoofable @@ -70,16 +71,34 @@ shipping — do NOT fix blind): 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. + [FIXED in 1.1.25] lib/run_services.cpp socket_proxy confinement. The + earlier note here mis-diagnosed safePath("/"): it did not "never + reject" — a trailing-slash boundary bug in util_pure.cpp made it + reject EVERY real path (so `share` was unusable), while its canonical + return value was discarded anyway (so it could not have confined the + raw ".." path that still reached J()). Both loops now validate the + concatenated jail-side path stays under jailPath, exactly as run.cpp + does for dirsShare; safePath's trailing-slash bug is fixed and + unit-tested. + +Recorded from the 2026-07 third-pass audit — LATENT, no caller today, +harden BEFORE wiring this code up: + +* (security, HIGH-if-reachable) lib/vm_spec.cpp:97-184 + generateDomainXml embeds `name`, `opts.disk`, `opts.sharedBridge`, + `vol.hostPath`, `vol.tag` unescaped into the libvirt domain XML + handed to virDomainCreateXML as root (vm_run.cpp:89, bhyve:///system). + A value containing `'/>…` injects arbitrary / + devices (e.g. mount host `/` into the guest). vm_run.cpp:250-388 + (configureVmDns / generateCloudInitFor9p) and vm_stack.cpp:88 + (registerVmInStack) likewise build root-written paths and an + /etc/hosts line from `vmName` / `vol.tag` with no traversal or + newline check. `createVm` / `parseVmOptions` / `generateDomainXml` / + `configureVmDns` / `registerVmInStack` have NO callers in the tree — + dead code — so not exploitable today. XML-escape every field (or + validate to a tight charset) and reuse StackPure::validateStackName + for vmName/vol.tag before any of this gets a caller. memoryToKiB + (vm_spec.cpp:84) also has an unchecked `val*1024*1024` overflow. === High priority — blocking production use === diff --git a/cli/args.cpp b/cli/args.cpp index bc7bddf..a2dc906 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.24" << std::endl; + std::cout << "crate 1.1.25" << 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.24" << std::endl; + std::cout << "crate 1.1.25" << std::endl; exit(0); default: err("unsupported short option '%s'", argv[a]); diff --git a/docs/trust-model.md b/docs/trust-model.md index e289e02..b9cec9d 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.24 (rootless model + per-tenant authz series 1.1.12 → +**Applies to:** 1.1.25 (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.24 means reasoning about who can reach +Reasoning about isolation on 1.1.25 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 112b684..f41fa84 100644 --- a/docs/trust-model.uk.md +++ b/docs/trust-model.uk.md @@ -4,7 +4,7 @@ (кілька операторів на одній машині), і контрибʼютори, які розширюють привілейовану поверхню. -**Стосується:** 1.1.24 (rootless-модель + серія per-tenant authz 1.1.12 → +**Стосується:** 1.1.25 (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.24 — це міркувати про те, хто має доступ +Міркувати про ізоляцію на 1.1.25 — це міркувати про те, хто має доступ до **privops**, а не хто може запустити `crate(1)`. --- diff --git a/lib/gui.cpp b/lib/gui.cpp index 214f17f..1ffa04d 100644 --- a/lib/gui.cpp +++ b/lib/gui.cpp @@ -13,6 +13,10 @@ #include #include +#include +#include +#include +#include #include #include #include @@ -337,9 +341,26 @@ static bool guiScreenshot(const Args &args) { isPnmOutput = true; } + // 1.1.25: scratch files go into a private mkdtemp(3) directory — mode + // 0700, random name — instead of the old predictable + // /tmp/crate-screenshot-.{ppm,xwd}. displayNum is small + // and guessable (allocation starts at 10) and this command runs with + // root's EUID (the GUI registry is root-only), so a local user could + // pre-plant a symlink at that name and have root truncate/overwrite an + // arbitrary file via fopen/`xwd -out` (CWE-59). Nobody else can write + // into our 0700 dir, so no link can be planted inside it. The dir and + // whatever is left in it are removed on every exit path. + char tmpDirTemplate[] = "/tmp/crate-screenshot-XXXXXX"; + if (::mkdtemp(tmpDirTemplate) == nullptr) + ERR("screenshot: cannot create scratch directory: " << std::strerror(errno)) + const std::string tmpDir = tmpDirTemplate; + RunAtEnd removeTmpDir([tmpDir]() { + std::error_code ec; + std::filesystem::remove_all(tmpDir, ec); + }); + if (X11Ops::available()) { - auto pnmTmp = isPnmOutput ? outFile : - STR("/tmp/crate-screenshot-" << e.displayNum << ".ppm"); + auto pnmTmp = isPnmOutput ? outFile : (tmpDir + "/screenshot.ppm"); if (!X11Ops::screenshot(dispStr, pnmTmp)) { // Fall through to the xwd pipeline below; libX11 is linked // but the display may not be reachable from this process @@ -369,7 +390,7 @@ static bool guiScreenshot(const Args &args) { } // Fallback: xwd + xwdtopnm + pnmtopng pipeline (pre-0.8.36 path). - auto xwdFile = STR("/tmp/crate-screenshot-" << e.displayNum << ".xwd"); + auto xwdFile = tmpDir + "/screenshot.xwd"; try { Util::execCommand( {CRATE_PATH_XWD, "-root", "-display", dispStr, "-out", xwdFile}, diff --git a/lib/jail_query.cpp b/lib/jail_query.cpp index 2b99bff..d50b4ea 100644 --- a/lib/jail_query.cpp +++ b/lib/jail_query.cpp @@ -194,14 +194,20 @@ std::vector getAllJails(bool crateOnly) { info.ip4 = ps.getString(5); info.dying = (ps.getString(6) == "true" || ps.getString(6) == "1"); + // 1.1.25: advance the lastjid cursor BEFORE the crateOnly filter. + // The advance used to sit after the filter, so a non-crate jail + // hit `continue` without moving the cursor and the next + // jailparam_get returned the very same jail forever — a 100%-CPU + // infinite loop the moment any foreign jail (bastille/pot/plain + // jail(8)) coexisted with crate. That wedged every crateOnly + // caller, including the crated control-socket jail listing. + auto nextJid = std::to_string(info.jid); + jailparam_import(&ps.params[0], nextJid.c_str()); + if (crateOnly && info.path.find(cratePrefix) != 0) continue; result.push_back(info); - - // Advance lastjid for next iteration - auto nextJid = std::to_string(info.jid); - jailparam_import(&ps.params[0], nextJid.c_str()); } return result; diff --git a/lib/run_services.cpp b/lib/run_services.cpp index b327493..8b98229 100644 --- a/lib/run_services.cpp +++ b/lib/run_services.cpp @@ -72,7 +72,16 @@ RunAtEnd setupSocketProxy(const Spec &spec, const std::string &jailPath, bool lo auto J = [&jailPath](auto subdir) { return STR(jailPath << subdir); }; for (auto &sockPath : spec.socketProxy->share) { - Util::safePath(sockPath, "/", "shared socket"); + // 1.1.25: confine the JAIL-SIDE path — validate that jailPath + + // sockPath, once canonicalized, still lives under jailPath (the + // same guard run.cpp applies to dirsShare/filesShare). The old + // safePath(sockPath, "/", …) could not confine anything: prefix "/" + // matches every absolute path, and its canonical return value was + // discarded while the raw (possibly "..") sockPath still went into + // J(). (It also over-rejected every path due to the trailing-slash + // bug fixed in util_pure.cpp this release — so the feature was + // simultaneously unusable AND unguarded.) + Util::safePath(J(sockPath), jailPath, "shared socket (jail side)"); auto parentDir = sockPath.substr(0, sockPath.rfind('/')); std::filesystem::create_directories(J(parentDir)); Util::Fs::writeFile("", J(sockPath)); @@ -83,6 +92,13 @@ RunAtEnd setupSocketProxy(const Spec &spec, const std::string &jailPath, bool lo std::vector socatPids; for (auto &entry : spec.socketProxy->proxy) { + // 1.1.25: the proxy loop had NO confinement at all — entry.jail + // reached create_directories(J(jailParent)) and a + // UNIX-LISTEN:J(entry.jail) socat bind unchecked, so a ".."-bearing + // value planted a root-owned dir/socket outside the jail tree. Same + // jail-side guard as `share`. (entry.host is the operator's own + // host-side connect target and is deliberately not jail-confined.) + Util::safePath(J(entry.jail), jailPath, "socket proxy (jail side)"); if (logProgress) std::cerr << rang::fg::gray << "starting socket proxy: " << entry.host << " <-> " << entry.jail << rang::style::reset << std::endl; auto jailParent = entry.jail.substr(0, entry.jail.rfind('/')); diff --git a/lib/stack.cpp b/lib/stack.cpp index 3faddab..c27aabc 100644 --- a/lib/stack.cpp +++ b/lib/stack.cpp @@ -78,6 +78,12 @@ static std::vector parseNetworks(const YAML::Node &top) { for (auto n : top["networks"]) { StackNetwork net; net.name = n.first.as(); + // 1.1.25: the network name becomes the `dns-` config directory + // that startStackDns create_directories/writes and stopStackDns + // remove_all's as root — reject '/' and '..' (path traversal) and + // shell/quote chars before it goes anywhere. + if (auto e = StackPure::validateStackName(net.name); !e.empty()) + ERR("networks/" << net.name << ": " << e) if (!n.second.IsMap()) ERR("networks/" << net.name << " must be a map") if (n.second["bridge"]) @@ -86,8 +92,13 @@ static std::vector parseNetworks(const YAML::Node &top) { ERR("networks/" << net.name << " requires 'bridge' field") if (n.second["subnet"]) net.subnet = n.second["subnet"].as(); - if (n.second["gateway"]) + if (n.second["gateway"]) { net.gateway = n.second["gateway"].as(); + // 1.1.25: the gateway is interpolated into a `printf 'nameserver …'` + // shell fragment run as root — must be a bare IP literal. + if (auto e = StackPure::validateStackIp(net.gateway); !e.empty()) + ERR("networks/" << net.name << "/gateway: " << e) + } if (n.second["dns"]) net.dns = n.second["dns"].as(); if (n.second["ip_range"]) @@ -531,6 +542,11 @@ static ParsedStack parseStackFile(const std::string &fname, const std::map(); + // 1.1.25: the container name is interpolated into a `printf '…' >> + // /etc/hosts` shell fragment run via `sh -c` as root — reject + // quotes/metachars/newlines before it can close the quote. + if (auto e = StackPure::validateStackName(entry.name); !e.empty()) + ERR("containers/" << entry.name << ": " << e) if (!c.second.IsMap()) ERR("containers/" << entry.name << " must be a map") @@ -867,11 +883,20 @@ bool stackCommand(const Args &args) { // Inject /etc/hosts entries for inter-container DNS (§26) if (!hostsEntries.empty()) { - // Build a shell command that appends all container mappings to /etc/hosts + // Build a shell command that appends all container mappings to /etc/hosts. + // 1.1.25: sink-guard — every name/IP is re-validated right here, + // at the point it enters a `sh -c` string, so nothing that could + // close the single quote reaches the shell no matter where the + // value originated (stack file, member spec, IP-pool allocator). std::ostringstream hostsCmd; hostsCmd << "printf '\\n# crate stack containers\\n"; - for (auto &kv : containerIPs) + for (auto &kv : containerIPs) { + if (auto err = StackPure::validateStackName(kv.first); !err.empty()) + ERR("stack hosts: container '" << kv.first << "': " << err) + if (auto err = StackPure::validateStackIp(kv.second); !err.empty()) + ERR("stack hosts: container '" << kv.first << "' address: " << err) hostsCmd << kv.second << " " << kv.first << "\\n"; + } hostsCmd << "' >> /etc/hosts"; spec.scripts["run:before-start-services"]["__crate_stack_hosts"] = hostsCmd.str(); } @@ -879,6 +904,9 @@ bool stackCommand(const Args &args) { // Inject DNS resolver pointing to stack DNS service for (auto &net : networks) { if (net.dns && !net.gateway.empty()) { + // 1.1.25: sink-guard (gateway was validated at parse time too). + if (auto err = StackPure::validateStackIp(net.gateway); !err.empty()) + ERR("stack dns: network '" << net.name << "' gateway: " << err) std::ostringstream dnsCmd; dnsCmd << "printf 'nameserver " << net.gateway << "\\n' > /etc/resolv.conf"; spec.scripts["run:before-start-services"]["__crate_stack_dns"] = dnsCmd.str(); diff --git a/lib/stack_pure.cpp b/lib/stack_pure.cpp index f4bae7e..ca6de09 100644 --- a/lib/stack_pure.cpp +++ b/lib/stack_pure.cpp @@ -68,6 +68,38 @@ std::string buildHostsEntries(const std::map &nameToIp return ss.str(); } +std::string validateStackName(const std::string &name) { + if (name.empty()) return "name is empty"; + if (name.size() > 64) return "name is longer than 64 chars"; + if (name == "." || name == "..") return "name is reserved"; + if (name.front() == '-') return "name must not start with '-'"; + for (char c : name) { + bool ok = (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') + || (c >= '0' && c <= '9') || c == '.' || c == '_' || c == '-'; + if (!ok) + return "name contains an invalid character (allowed: [A-Za-z0-9._-])"; + } + return ""; +} + +std::string validateStackIp(const std::string &ip) { + if (ip.empty()) return "address is empty"; + // Charset gate first: this is what makes the value inert inside the + // single-quoted shell fragment regardless of what inet_pton thinks. + for (char c : ip) { + bool ok = (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') + || (c >= 'A' && c <= 'F') || c == '.' || c == ':' || c == '/'; + if (!ok) + return "address contains an invalid character"; + } + auto bare = ipFromCidr(ip); + 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"; +} + // topoSort is templated and lives in stack_pure.h. } diff --git a/lib/stack_pure.h b/lib/stack_pure.h index b52e155..0bd4cd4 100644 --- a/lib/stack_pure.h +++ b/lib/stack_pure.h @@ -27,6 +27,25 @@ bool isIpv6Address(const std::string &addr); std::string ipFromCidr(const std::string &cidr); std::string buildHostsEntries(const std::map &nameToIp); +// 1.1.25: validators for stack-file fields that reach a root-run sink. +// +// validateStackName — container and network names. Both are YAML map +// keys that used to flow unvalidated into (a) a `printf '…' >> +// /etc/hosts` shell fragment run via `sh -c` as root and (b) the +// `dns-` config-directory path that is create_directories'd, +// written, and remove_all'd as root. Constrain to [A-Za-z0-9._-], +// 1..64 chars, not "."/"..", no leading '-': that excludes quotes, +// backslashes, newlines, and '/' — so the value is inert inside the +// single-quoted printf and is a single safe path component. +std::string validateStackName(const std::string &name); + +// validateStackIp — container static IPs and network gateways, which +// reach the same `sh -c` fragments. Accepts an IPv4/IPv6 literal with +// an optional /prefix; rejects any byte outside [0-9a-fA-F.:/] first, +// then requires inet_pton to accept the address part. Returns "" on +// success. +std::string validateStackIp(const std::string &ip); + // Lightweight stack-entry view used by the tests. struct StackEntry { std::string name; diff --git a/lib/util_pure.cpp b/lib/util_pure.cpp index 04d0d3a..18dad45 100644 --- a/lib/util_pure.cpp +++ b/lib/util_pure.cpp @@ -119,9 +119,19 @@ std::string safePath(const std::string &path, const std::string &requiredPrefix, // Require both: prefix match AND a path-separator immediately after. // Without the separator check, prefix "/foo" would wrongly accept the // unrelated path "/foobar/x". + // + // 1.1.25: when the prefix ALREADY ends in '/' (the root prefix "/" is + // the degenerate case), the separator has been consumed by the prefix + // itself, so canonical[prefix.size()] is the first char of a filename, + // not a separator. The old unconditional check therefore rejected + // every real path under "/" except "/" itself — which silently broke + // socketProxy.share (run_services.cpp calls safePath(sock, "/", …)). + // Only demand the separator when the prefix doesn't supply it. + bool prefixEndsWithSep = !requiredPrefix.empty() && requiredPrefix.back() == '/'; if (canonical.size() < requiredPrefix.size() || canonical.compare(0, requiredPrefix.size(), requiredPrefix) != 0 || - (canonical.size() > requiredPrefix.size() && + (!prefixEndsWithSep && + canonical.size() > requiredPrefix.size() && canonical[requiredPrefix.size()] != '/')) ERR2("path validation", "'" << what << "' path '" << path << "' resolves to '" << canonical << "' which is outside required prefix '" << requiredPrefix << "'") diff --git a/tests/unit/stack_test.cpp b/tests/unit/stack_test.cpp index 9be91c1..ba59c7c 100644 --- a/tests/unit/stack_test.cpp +++ b/tests/unit/stack_test.cpp @@ -13,6 +13,8 @@ using StackPure::ipFromCidr; using StackPure::buildHostsEntries; using StackPure::topoSort; +using StackPure::validateStackName; +using StackPure::validateStackIp; using StackEntry = StackPure::StackEntry; static int indexOf(const std::vector &v, const std::string &name) { @@ -73,6 +75,62 @@ ATF_TEST_CASE_BODY(buildHosts_sorted_by_name) "10.0.0.30 web\n"); } +// =================================================================== +// 1.1.25: validateStackName / validateStackIp — the fields that reach a +// root-run `sh -c` fragment (hosts/resolv.conf injection) and the +// `dns-` root-managed config dir. +// =================================================================== + +ATF_TEST_CASE_WITHOUT_HEAD(validateStackName_typical_accepted); +ATF_TEST_CASE_BODY(validateStackName_typical_accepted) +{ + ATF_REQUIRE_EQ(validateStackName("web"), ""); + ATF_REQUIRE_EQ(validateStackName("db-primary"), ""); + ATF_REQUIRE_EQ(validateStackName("app.internal_1"), ""); + ATF_REQUIRE_EQ(validateStackName("a..b"), ""); // one component, no '/' +} + +ATF_TEST_CASE_WITHOUT_HEAD(validateStackName_injection_rejected); +ATF_TEST_CASE_BODY(validateStackName_injection_rejected) +{ + // Closes the single-quoted printf and runs a command as root. + ATF_REQUIRE(!validateStackName("x';touch /tmp/pwned;'").empty()); + ATF_REQUIRE(!validateStackName("x\"y").empty()); + ATF_REQUIRE(!validateStackName("x`id`").empty()); + ATF_REQUIRE(!validateStackName("x$(id)").empty()); + ATF_REQUIRE(!validateStackName("x\\y").empty()); + ATF_REQUIRE(!validateStackName("x\ny").empty()); + ATF_REQUIRE(!validateStackName("x y").empty()); + // Path traversal out of the dns- config dir. + ATF_REQUIRE(!validateStackName("../../../etc/cron.d").empty()); + ATF_REQUIRE(!validateStackName("a/b").empty()); + ATF_REQUIRE(!validateStackName("..").empty()); + ATF_REQUIRE(!validateStackName(".").empty()); + ATF_REQUIRE(!validateStackName("").empty()); + ATF_REQUIRE(!validateStackName("-leading").empty()); + ATF_REQUIRE(!validateStackName(std::string(65, 'a')).empty()); +} + +ATF_TEST_CASE_WITHOUT_HEAD(validateStackIp_typical_accepted); +ATF_TEST_CASE_BODY(validateStackIp_typical_accepted) +{ + ATF_REQUIRE_EQ(validateStackIp("10.0.0.5"), ""); + ATF_REQUIRE_EQ(validateStackIp("10.0.0.5/24"), ""); // CIDR tolerated + ATF_REQUIRE_EQ(validateStackIp("fd00::1"), ""); + ATF_REQUIRE_EQ(validateStackIp("fd00::1/64"), ""); +} + +ATF_TEST_CASE_WITHOUT_HEAD(validateStackIp_injection_rejected); +ATF_TEST_CASE_BODY(validateStackIp_injection_rejected) +{ + ATF_REQUIRE(!validateStackIp("1.1.1.1'; reboot;'").empty()); + ATF_REQUIRE(!validateStackIp("1.1.1.1\n").empty()); + ATF_REQUIRE(!validateStackIp("1.1.1.1 evil").empty()); + ATF_REQUIRE(!validateStackIp("256.0.0.1").empty()); // charset ok, inet_pton fails + ATF_REQUIRE(!validateStackIp("not-an-ip").empty()); + ATF_REQUIRE(!validateStackIp("").empty()); +} + ATF_TEST_CASE_WITHOUT_HEAD(topoSort_empty); ATF_TEST_CASE_BODY(topoSort_empty) { @@ -199,4 +257,8 @@ ATF_INIT_TEST_CASES(tcs) ATF_ADD_TEST_CASE(tcs, topoSort_duplicate_name_throws); ATF_ADD_TEST_CASE(tcs, topoSort_three_node_cycle_throws); ATF_ADD_TEST_CASE(tcs, topoSort_disconnected_components); + ATF_ADD_TEST_CASE(tcs, validateStackName_typical_accepted); + ATF_ADD_TEST_CASE(tcs, validateStackName_injection_rejected); + ATF_ADD_TEST_CASE(tcs, validateStackIp_typical_accepted); + ATF_ADD_TEST_CASE(tcs, validateStackIp_injection_rejected); } diff --git a/tests/unit/util_security_test.cpp b/tests/unit/util_security_test.cpp index ba37604..02a8497 100644 --- a/tests/unit/util_security_test.cpp +++ b/tests/unit/util_security_test.cpp @@ -62,6 +62,37 @@ ATF_TEST_CASE_BODY(safePath_sibling_rejected) ATF_REQUIRE_THROW(Exception, Util::safePath(bad, dir, "spec")); } +// 1.1.25: a prefix that already ends in '/' (the root prefix "/" is the +// degenerate case) must accept ordinary absolute paths beneath it. The +// old separator check demanded canonical[prefix.size()] == '/', which +// for prefix "/" is the first filename character — so every real path +// was rejected and socketProxy.share always aborted. +ATF_TEST_CASE_WITHOUT_HEAD(safePath_root_prefix_accepts_absolute); +ATF_TEST_CASE_BODY(safePath_root_prefix_accepts_absolute) +{ + auto dir = makeTempDir("rootprefix"); + auto sock = dir + "/app.sock"; + // Prefix "/" — must NOT throw, and must return the canonical path. + auto out = Util::safePath(sock, "/", "shared socket"); + ATF_REQUIRE(!out.empty()); + ATF_REQUIRE_EQ(out.front(), '/'); + // "/" itself is still accepted. + ATF_REQUIRE_EQ(Util::safePath("/", "/", "root"), std::string("/")); +} + +ATF_TEST_CASE_WITHOUT_HEAD(safePath_trailing_slash_prefix_still_rejects_sibling); +ATF_TEST_CASE_BODY(safePath_trailing_slash_prefix_still_rejects_sibling) +{ + // With a trailing-slash prefix the sibling guard must still hold: + // prefix "/" accepts "/x" but rejects "_neighbour/y", + // because the prefix compare itself already covers the separator. + auto dir = makeTempDir("tslash"); + auto ok = Util::safePath(dir + "/x", dir + "/", "t"); + ATF_REQUIRE(ok.compare(0, dir.size(), dir) == 0); + ATF_REQUIRE_THROW(Exception, + Util::safePath(dir + "_neighbour/y", dir + "/", "t")); +} + ATF_TEST_CASE_WITHOUT_HEAD(safePath_absolute_outside_rejected); ATF_TEST_CASE_BODY(safePath_absolute_outside_rejected) { @@ -176,6 +207,8 @@ ATF_INIT_TEST_CASES(tcs) ATF_ADD_TEST_CASE(tcs, safePath_dot_segments_normalized); ATF_ADD_TEST_CASE(tcs, safePath_symlink_escape_rejected); ATF_ADD_TEST_CASE(tcs, safePath_returns_canonical); + ATF_ADD_TEST_CASE(tcs, safePath_root_prefix_accepts_absolute); + ATF_ADD_TEST_CASE(tcs, safePath_trailing_slash_prefix_still_rejects_sibling); // shellQuote ATF_ADD_TEST_CASE(tcs, shellQuote_neutralizes_command_substitution);