From bdcd14a7cb33d33f9e501db5d2b04ecc4ebc73cf Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 7 Jul 2026 19:39:25 +0000 Subject: [PATCH] fix(security): cron/user injection, list JSON escaping, ctx erase UB, vmwrap jailPath (1.1.22) Four clear, testable findings from a second-pass project-wide audit: - 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/) and a `sh -c "chmod ... "` string. In the legacy setuid build that is path traversal (../../etc/cron.d/pwn -> host root) + shell injection from an unprivileged spec author. New RunPure::validateCronUser constrains it to [A-Za-z0-9_-], <=32, no leading '-'. - 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 uses jsonEscape. Those fields come from the kernel's jail state, so a `"` in a hostname produced malformed/injectable JSON. Now escaped. - undefined behavior in FwUsers::del (lib/ctx.cpp): pids.erase(pids.find(pid)) is UB when pid is absent (erase(end())), reachable on a stale context file or double teardown. Switched to erase(key), matching FwSlots::release. - unvalidated jailPath in vm-wrap (lib/vmwrap_pure.cpp): validateSpec checked every field except jailPath, emitted verbatim into `path = "...";` in the jail.conf fragment `jail -c -f` runs as root. New validateJailPath requires an absolute path, rejects `"` and control bytes. Adds run_pure/list_pure/vmwrap_pure test cases for each. Bumps to 1.1.22; CHANGELOG + trust-model (en/uk) updated. Three higher-touch audit items (REMOTE_ADDR auth locality, lease flock-on-renamed-inode race, socket_proxy path confinement) recorded in TODO for a follow-up that wants on-hardware validation. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01X6t6tzVypHye5bDGLxzmZK --- CHANGELOG.md | 44 ++++++++++++++++++++++++++++++ TODO | 48 +++++++++++++++++++++++++++++++++ cli/args.cpp | 4 +-- docs/trust-model.md | 4 +-- docs/trust-model.uk.md | 4 +-- lib/ctx.cpp | 7 ++++- lib/list_pure.cpp | 42 ++++++++++++++++++++++++----- lib/run.cpp | 5 ++++ lib/run_pure.cpp | 14 ++++++++++ lib/run_pure.h | 9 +++++++ lib/vmwrap_pure.cpp | 15 +++++++++++ lib/vmwrap_pure.h | 7 +++++ tests/unit/list_pure_test.cpp | 20 ++++++++++++++ tests/unit/run_pure_test.cpp | 29 ++++++++++++++++++++ tests/unit/vmwrap_pure_test.cpp | 33 +++++++++++++++++++++++ 15 files changed, 272 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd55d6b..7c2a412 100644 --- a/CHANGELOG.md +++ b/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/`) and a `sh -c "chmod … "` 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 diff --git a/TODO b/TODO index 4ea1e48..ac92965 100644 --- a/TODO +++ b/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. `.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 diff --git a/cli/args.cpp b/cli/args.cpp index c92fb2f..739eeb2 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.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]); diff --git a/docs/trust-model.md b/docs/trust-model.md index 12e65fe..a1d9061 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.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)`. --- diff --git a/docs/trust-model.uk.md b/docs/trust-model.uk.md index 8985a0b..9ba8ca3 100644 --- a/docs/trust-model.uk.md +++ b/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)`. --- diff --git a/lib/ctx.cpp b/lib/ctx.cpp index 59b2360..62943c2 100644 --- a/lib/ctx.cpp +++ b/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; } diff --git a/lib/list_pure.cpp b/lib/list_pure.cpp index 8fca352..7ad0a62 100644 --- a/lib/list_pure.cpp +++ b/lib/list_pure.cpp @@ -3,6 +3,7 @@ #include "list_pure.h" #include +#include #include #include #include @@ -10,18 +11,47 @@ 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 &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 << ","; diff --git a/lib/run.cpp b/lib/run.cpp index 5815032..22640ec 100644 --- a/lib/run.cpp +++ b/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", diff --git a/lib/run_pure.cpp b/lib/run_pure.cpp index 7925049..786ba38 100644 --- a/lib/run_pure.cpp +++ b/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 ""; +} + } diff --git a/lib/run_pure.h b/lib/run_pure.h index 5ac63a7..337e3ad 100644 --- a/lib/run_pure.h +++ b/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/`) 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. +std::string validateCronUser(const std::string &user); + } diff --git a/lib/vmwrap_pure.cpp b/lib/vmwrap_pure.cpp index 14b6a17..c6051cb 100644 --- a/lib/vmwrap_pure.cpp +++ b/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(c) < 0x20 || + static_cast(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 ""; diff --git a/lib/vmwrap_pure.h b/lib/vmwrap_pure.h index e00a3ea..b0ca4ad 100644 --- a/lib/vmwrap_pure.h +++ b/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 = "";` 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); diff --git a/tests/unit/list_pure_test.cpp b/tests/unit/list_pure_test.cpp index f14587a..1e30973 100644 --- a/tests/unit/list_pure_test.cpp +++ b/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); diff --git a/tests/unit/run_pure_test.cpp b/tests/unit/run_pure_test.cpp index 03366c5..0d41443 100644 --- a/tests/unit/run_pure_test.cpp +++ b/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); } diff --git a/tests/unit/vmwrap_pure_test.cpp b/tests/unit/vmwrap_pure_test.cpp index 4e912d3..cf9ceca 100644 --- a/tests/unit/vmwrap_pure_test.cpp +++ b/tests/unit/vmwrap_pure_test.cpp @@ -114,6 +114,38 @@ ATF_TEST_CASE_BODY(spec_validation_short_circuits) { ATF_REQUIRE(!validateSpec(bad).empty()); } +// 1.1.22: jailPath is emitted verbatim into `path = "..."` in the +// jail.conf fragment run by `jail -c -f` as root. validateSpec must +// reject a quote (closes the value) or a control byte (injects a +// directive), and require an absolute path. +ATF_TEST_CASE_WITHOUT_HEAD(spec_validates_jail_path); +ATF_TEST_CASE_BODY(spec_validates_jail_path) { + WrapSpec good; + good.vmName = "alpine"; good.jailName = "alpine-cage"; + + // Empty (default "/") and a clean absolute path are accepted. + ATF_REQUIRE_EQ(validateSpec(good), std::string()); + WrapSpec ok = good; ok.jailPath = "/usr/jails/alpine-cage"; + ATF_REQUIRE_EQ(validateSpec(ok), std::string()); + + // Quote-injection: escape the quoted value and inject directives. + WrapSpec inj = good; + inj.jailPath = "/j\";\n allow.mount;\n path=\"/"; + ATF_REQUIRE(!validateSpec(inj).empty()); + + // Bare quote alone. + WrapSpec q = good; q.jailPath = "/tmp/a\"b"; + ATF_REQUIRE(!validateSpec(q).empty()); + + // Newline alone. + WrapSpec nl = good; nl.jailPath = "/tmp/a\nallow.mount"; + ATF_REQUIRE(!validateSpec(nl).empty()); + + // Non-absolute. + WrapSpec rel = good; rel.jailPath = "relative/path"; + ATF_REQUIRE(!validateSpec(rel).empty()); +} + // --- Derivations --- ATF_TEST_CASE_WITHOUT_HEAD(ruleset_derivation_in_range); @@ -284,6 +316,7 @@ ATF_INIT_TEST_CASES(tcs) { ATF_ADD_TEST_CASE(tcs, tap_and_nmdm_ranges); ATF_ADD_TEST_CASE(tcs, ruleset_num_range); ATF_ADD_TEST_CASE(tcs, spec_validation_short_circuits); + ATF_ADD_TEST_CASE(tcs, spec_validates_jail_path); ATF_ADD_TEST_CASE(tcs, ruleset_derivation_in_range); ATF_ADD_TEST_CASE(tcs, ruleset_derivation_deterministic); ATF_ADD_TEST_CASE(tcs, default_jail_path_is_root);