diff --git a/CHANGELOG.md b/CHANGELOG.md index 441d738..cd55d6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,42 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). --- +## [1.1.21] — 2026-07-07 + +**Security: reject control-char / path-traversal injection in +config free-text fields (WireGuard, IPsec, migrate).** + +The pure-module audit found three sinks where a less-trusted string was +emitted verbatim into a generated config or used as a filesystem path +without a control-character / traversal check: + +- `WireguardPure::validateConfig` validated keys, CIDRs, ports and + endpoints but never the free-text `FwMark`, `DNS`, `MTU`, and peer + `description` fields. Each is written straight into the wg-quick + `.conf`; an embedded newline escapes the line and injects its own + directive — most dangerously `PostUp = `, which wg-quick runs as + **root**. A new `hasControlChar` helper now rejects any byte `< 0x20` + or `== 0x7f` (incl. `\n`/`\r`) in those fields. + +- `IpsecPure::validateConfig` likewise never checked `leftid`, + `rightid`, or `description`, all emitted into `ipsec.conf` + (`leftid=…` / `rightid=…` / `# …`). A newline injects an arbitrary + conn option (e.g. `rightsubnet=0.0.0.0/0`, silently widening the + tunnel policy). Control bytes in those three fields are now rejected. + +- `MigratePure` gained `validateArtifactFile`: the artifact filename in + the migrate flow comes from the **source server's** `/export` JSON + (`extractFileField`) and is concatenated into `workDir + "/" + name`, + then used both as a `curl -o` target and an `unlink()` target. A + hostile or compromised source returning `"../../../etc/cron.d/pwn"` + got arbitrary file write **and** delete on the migrating host. The + filename is now validated as a single safe path component (no `/`, no + `..`, no control bytes, ≤ 255 chars) in `lib/migrate.cpp` before use. + +New `wireguard_pure_test`, `ipsec_pure_test`, and `migrate_pure_test` +cases cover each rejection (newline / traversal / slash / control byte) +and confirm clean values still validate. + ## [1.1.20] — 2026-07-05 **Fix: IPv6 lease parser didn't validate the jail-name charset.** diff --git a/cli/args.cpp b/cli/args.cpp index a662b1c..c92fb2f 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.20" << std::endl; + std::cout << "crate 1.1.21" << 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.20" << std::endl; + std::cout << "crate 1.1.21" << 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 5079d2c..12e65fe 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.20 (rootless model + per-tenant authz series 1.1.12 → +**Applies to:** 1.1.21 (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.20 means reasoning about who can reach +Reasoning about isolation on 1.1.21 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 7da4a15..8985a0b 100644 --- a/docs/trust-model.uk.md +++ b/docs/trust-model.uk.md @@ -4,7 +4,7 @@ (кілька операторів на одній машині), і контрибʼютори, які розширюють привілейовану поверхню. -**Стосується:** 1.1.20 (rootless-модель + серія per-tenant authz 1.1.12 → +**Стосується:** 1.1.21 (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.20 — це міркувати про те, хто має доступ +Міркувати про ізоляцію на 1.1.21 — це міркувати про те, хто має доступ до **privops**, а не хто може запустити `crate(1)`. --- diff --git a/lib/ipsec_pure.cpp b/lib/ipsec_pure.cpp index 1acbd30..724f106 100644 --- a/lib/ipsec_pure.cpp +++ b/lib/ipsec_pure.cpp @@ -225,6 +225,20 @@ std::string validateConfig(const std::vector &conns) { if (!c.autoStart.empty()) if (auto e = validateAuto(c.autoStart); !e.empty()) return prefixed(i, e); + // 1.1.21: leftid/rightid/description are emitted verbatim into + // ipsec.conf (as `leftid=…` / `rightid=…` / `# …`). A newline + // escapes the line and injects an arbitrary conn option (e.g. + // `rightsubnet=0.0.0.0/0`, silently widening the tunnel policy). + // validateConfig validated left/right but never these free-text + // fields. + for (auto &pr : {std::pair{"leftid", c.leftId}, + std::pair{"rightid", c.rightId}, + std::pair{"description", c.description}}) { + for (char ch : pr.second) + if (static_cast(ch) < 0x20 || + static_cast(ch) == 0x7f) + return prefixed(i, std::string(pr.first) + " contains a control character"); + } } return ""; } diff --git a/lib/migrate.cpp b/lib/migrate.cpp index 1ebe661..4f2046b 100644 --- a/lib/migrate.cpp +++ b/lib/migrate.cpp @@ -164,6 +164,10 @@ bool migrateCommand(const Args &args) { r.artifactFile = extractFileField(exportBody); if (r.artifactFile.empty()) ERR("export response did not include a 'file' field") + // 1.1.21: the filename is remote-controlled — validate before it is + // used as a `curl -o` / `unlink()` path (path-traversal guard). + if (auto e = MigratePure::validateArtifactFile(r.artifactFile); !e.empty()) + ERR("export response 'file' field rejected: " << e) // Steps 2..5. for (auto &s : MigratePure::buildRemainingSteps(r)) diff --git a/lib/migrate_pure.cpp b/lib/migrate_pure.cpp index f0cc22a..a908c52 100644 --- a/lib/migrate_pure.cpp +++ b/lib/migrate_pure.cpp @@ -177,6 +177,30 @@ std::string validateContainerName(const std::string &name) { return ""; } +// 1.1.21: the artifact filename comes from the SOURCE server's export +// JSON (`extractFileField`), i.e. a remote, less-trusted input. It is +// concatenated into `workDir + "/" + artifactFile` and used both as a +// `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. +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"; + if (name == "." || name == "..") return "artifact filename is reserved"; + for (char c : name) { + if (c == '/') + return "artifact filename must not contain '/' (path traversal)"; + if (static_cast(c) < 0x20 || + static_cast(c) == 0x7f) + return "artifact filename contains a control character"; + } + if (name.find("..") != std::string::npos) + return "artifact filename must not contain '..'"; + return ""; +} + std::string normalizeBaseUrl(const std::string &endpoint) { bool hasScheme = false; auto rest = stripScheme(endpoint, &hasScheme); diff --git a/lib/migrate_pure.h b/lib/migrate_pure.h index 9ced84d..86fa9ca 100644 --- a/lib/migrate_pure.h +++ b/lib/migrate_pure.h @@ -50,6 +50,12 @@ std::string validateBearerToken(const std::string &token); // matches what a client can put in a URL path segment. std::string validateContainerName(const std::string &name); +// 1.1.21: validate the artifact filename returned by the source +// server's /export (a remote-controlled value) before it is used as a +// filesystem path. Rejects empty, `/`, `..`, control bytes, > 255 +// chars — a single safe path component. +std::string validateArtifactFile(const std::string &name); + // --- Plan --- enum class StepKind { diff --git a/lib/wireguard_pure.cpp b/lib/wireguard_pure.cpp index c59d840..c3b620e 100644 --- a/lib/wireguard_pure.cpp +++ b/lib/wireguard_pure.cpp @@ -13,6 +13,18 @@ bool isBase64Char(char c) { || (c >= '0' && c <= '9') || c == '+' || c == '/'; } +// 1.1.21: a free-text field (DNS, FwMark, MTU, peer description) is +// emitted verbatim into the wg-quick .conf. A newline would let it +// inject its own directives — most dangerously `PostUp = `, which +// wg-quick runs as root. Reject any control byte (incl. \n/\r) and DEL. +bool hasControlChar(const std::string &s) { + for (char c : s) + if (static_cast(c) < 0x20 || + static_cast(c) == 0x7f) + return true; + return false; +} + bool isAlnum(char c) { return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9'); @@ -180,6 +192,16 @@ std::string validateConfig(const InterfaceSpec &iface, if (!iface.listenPort.empty()) if (auto e = validatePort(iface.listenPort); !e.empty()) return "interface ListenPort: " + e; + // 1.1.21: fields emitted verbatim into the .conf must not carry a + // newline/control byte (would inject wg-quick directives like PostUp, + // which runs as root). validateConfig previously skipped these. + if (hasControlChar(iface.fwmark)) + return "interface FwMark contains a control character"; + for (auto &d : iface.dns) + if (hasControlChar(d)) + return "interface DNS entry '" + d + "' contains a control character"; + if (!iface.mtu.empty() && hasControlChar(iface.mtu.front())) + return "interface MTU contains a control character"; // Peers if (peers.empty()) @@ -204,6 +226,12 @@ std::string validateConfig(const InterfaceSpec &iface, if (c < '0' || c > '9') return "peer #" + std::to_string(i + 1) + " PersistentKeepalive must be numeric"; } + // 1.1.21: the peer description is rendered as a `# ` + // comment line — a newline escapes the comment and injects a + // directive into the [Interface]/[Peer] scope. + if (hasControlChar(p.description)) + return "peer #" + std::to_string(i + 1) + + " description contains a control character"; } return ""; } diff --git a/tests/unit/ipsec_pure_test.cpp b/tests/unit/ipsec_pure_test.cpp index a6e3e0f..32dcf9b 100644 --- a/tests/unit/ipsec_pure_test.cpp +++ b/tests/unit/ipsec_pure_test.cpp @@ -164,6 +164,41 @@ ATF_TEST_CASE_BODY(config_conn_index_in_error) { ATF_REQUIRE(err.find("conn #2") != std::string::npos); } +// --- 1.1.21: injection hardening (control byte in id/description) --- + +ATF_TEST_CASE_WITHOUT_HEAD(config_leftid_newline_rejected); +ATF_TEST_CASE_BODY(config_leftid_newline_rejected) { + auto c = goodConn(); + c.leftId = "@vpn.example.com\n rightsubnet=0.0.0.0/0"; + auto err = validateConfig({c}); + ATF_REQUIRE(err.find("leftid") != std::string::npos); +} + +ATF_TEST_CASE_WITHOUT_HEAD(config_rightid_newline_rejected); +ATF_TEST_CASE_BODY(config_rightid_newline_rejected) { + auto c = goodConn(); + c.rightId = "peer\n auto=start"; + auto err = validateConfig({c}); + ATF_REQUIRE(err.find("rightid") != std::string::npos); +} + +ATF_TEST_CASE_WITHOUT_HEAD(config_description_newline_rejected); +ATF_TEST_CASE_BODY(config_description_newline_rejected) { + auto c = goodConn(); + c.description = "tunnel\nconn evil"; + auto err = validateConfig({c}); + ATF_REQUIRE(err.find("description") != std::string::npos); +} + +ATF_TEST_CASE_WITHOUT_HEAD(config_clean_id_and_description_accepted); +ATF_TEST_CASE_BODY(config_clean_id_and_description_accepted) { + auto c = goodConn(); + c.leftId = "@vpn.example.com"; + c.rightId = "C=UA, O=Example, CN=peer"; + c.description = "DC1 to DC2"; + ATF_REQUIRE_EQ(validateConfig({c}), std::string()); +} + // --- renderConf --- ATF_TEST_CASE_WITHOUT_HEAD(render_emits_setup_and_conn); @@ -244,6 +279,10 @@ ATF_INIT_TEST_CASES(tcs) { ATF_ADD_TEST_CASE(tcs, config_no_conns_rejected); ATF_ADD_TEST_CASE(tcs, config_keyexchange_invalid_rejected); ATF_ADD_TEST_CASE(tcs, config_conn_index_in_error); + ATF_ADD_TEST_CASE(tcs, config_leftid_newline_rejected); + ATF_ADD_TEST_CASE(tcs, config_rightid_newline_rejected); + ATF_ADD_TEST_CASE(tcs, config_description_newline_rejected); + ATF_ADD_TEST_CASE(tcs, config_clean_id_and_description_accepted); ATF_ADD_TEST_CASE(tcs, render_emits_setup_and_conn); ATF_ADD_TEST_CASE(tcs, render_csv_joins_subnets); ATF_ADD_TEST_CASE(tcs, render_omits_optional_fields_when_empty); diff --git a/tests/unit/migrate_pure_test.cpp b/tests/unit/migrate_pure_test.cpp index 0c24fb7..0f4be76 100644 --- a/tests/unit/migrate_pure_test.cpp +++ b/tests/unit/migrate_pure_test.cpp @@ -15,6 +15,7 @@ using MigratePure::describeStep; using MigratePure::normalizeBaseUrl; using MigratePure::redactToken; using MigratePure::validateBearerToken; +using MigratePure::validateArtifactFile; using MigratePure::validateContainerName; using MigratePure::validateEndpoint; @@ -83,6 +84,30 @@ ATF_TEST_CASE_BODY(name_invalid_rejected) { ATF_REQUIRE(!validateContainerName("foo;rm").empty()); } +// --- 1.1.21: validateArtifactFile (remote-controlled filename) --- + +ATF_TEST_CASE_WITHOUT_HEAD(artifact_file_typical_accepted); +ATF_TEST_CASE_BODY(artifact_file_typical_accepted) { + ATF_REQUIRE_EQ(validateArtifactFile("web01-20260707.crate"), std::string()); + ATF_REQUIRE_EQ(validateArtifactFile("export_1.tar.zst"), std::string()); +} + +ATF_TEST_CASE_WITHOUT_HEAD(artifact_file_traversal_rejected); +ATF_TEST_CASE_BODY(artifact_file_traversal_rejected) { + // A hostile/compromised source server returns these; they must be + // refused before they become a `curl -o` / `unlink()` path. + ATF_REQUIRE(!validateArtifactFile("").empty()); + ATF_REQUIRE(!validateArtifactFile(".").empty()); + ATF_REQUIRE(!validateArtifactFile("..").empty()); + 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 + ATF_REQUIRE(!validateArtifactFile("bad\nname").empty()); + ATF_REQUIRE(!validateArtifactFile("bad\tname").empty()); + ATF_REQUIRE(!validateArtifactFile(std::string(256, 'a')).empty()); +} + // --- normalizeBaseUrl --- ATF_TEST_CASE_WITHOUT_HEAD(normalize_adds_https_when_missing); @@ -204,6 +229,8 @@ ATF_INIT_TEST_CASES(tcs) { ATF_ADD_TEST_CASE(tcs, token_invalid_rejected); ATF_ADD_TEST_CASE(tcs, name_typical_accepted); ATF_ADD_TEST_CASE(tcs, name_invalid_rejected); + ATF_ADD_TEST_CASE(tcs, artifact_file_typical_accepted); + ATF_ADD_TEST_CASE(tcs, artifact_file_traversal_rejected); ATF_ADD_TEST_CASE(tcs, normalize_adds_https_when_missing); ATF_ADD_TEST_CASE(tcs, normalize_preserves_existing_scheme); ATF_ADD_TEST_CASE(tcs, build_export_step_shape); diff --git a/tests/unit/wireguard_pure_test.cpp b/tests/unit/wireguard_pure_test.cpp index 284298d..3fec867 100644 --- a/tests/unit/wireguard_pure_test.cpp +++ b/tests/unit/wireguard_pure_test.cpp @@ -178,6 +178,51 @@ ATF_TEST_CASE_BODY(config_peer_index_in_error) { ATF_REQUIRE(err.find("peer #2") != std::string::npos); } +// --- 1.1.21: injection hardening (control byte in free-text fields) --- + +ATF_TEST_CASE_WITHOUT_HEAD(config_iface_fwmark_newline_rejected); +ATF_TEST_CASE_BODY(config_iface_fwmark_newline_rejected) { + auto i = goodIface(); + i.fwmark = "0x1\nPostUp = touch /tmp/pwn"; + auto err = validateConfig(i, {goodPeer()}); + ATF_REQUIRE(err.find("FwMark") != std::string::npos); +} + +ATF_TEST_CASE_WITHOUT_HEAD(config_iface_dns_newline_rejected); +ATF_TEST_CASE_BODY(config_iface_dns_newline_rejected) { + auto i = goodIface(); + i.dns = {"1.1.1.1", "9.9.9.9\nPostUp = id"}; + auto err = validateConfig(i, {goodPeer()}); + ATF_REQUIRE(err.find("DNS") != std::string::npos); +} + +ATF_TEST_CASE_WITHOUT_HEAD(config_iface_mtu_newline_rejected); +ATF_TEST_CASE_BODY(config_iface_mtu_newline_rejected) { + auto i = goodIface(); + i.mtu = {"1420\nPostUp = id"}; + auto err = validateConfig(i, {goodPeer()}); + ATF_REQUIRE(err.find("MTU") != std::string::npos); +} + +ATF_TEST_CASE_WITHOUT_HEAD(config_peer_description_newline_rejected); +ATF_TEST_CASE_BODY(config_peer_description_newline_rejected) { + auto p = goodPeer(); + p.description = "edge\nPostUp = touch /tmp/pwn"; + auto err = validateConfig(goodIface(), {p}); + ATF_REQUIRE(err.find("description") != std::string::npos); +} + +ATF_TEST_CASE_WITHOUT_HEAD(config_clean_free_text_accepted); +ATF_TEST_CASE_BODY(config_clean_free_text_accepted) { + auto i = goodIface(); + i.fwmark = "0x1"; + i.dns = {"1.1.1.1"}; + i.mtu = {"1420"}; + auto p = goodPeer(); + p.description = "edge router (site B)"; + ATF_REQUIRE_EQ(validateConfig(i, {p}), std::string()); +} + // --- renderConf --- ATF_TEST_CASE_WITHOUT_HEAD(render_emits_interface_section); @@ -273,6 +318,11 @@ ATF_INIT_TEST_CASES(tcs) { ATF_ADD_TEST_CASE(tcs, config_no_peers_rejected); ATF_ADD_TEST_CASE(tcs, config_no_addresses_rejected); ATF_ADD_TEST_CASE(tcs, config_peer_index_in_error); + ATF_ADD_TEST_CASE(tcs, config_iface_fwmark_newline_rejected); + ATF_ADD_TEST_CASE(tcs, config_iface_dns_newline_rejected); + ATF_ADD_TEST_CASE(tcs, config_iface_mtu_newline_rejected); + ATF_ADD_TEST_CASE(tcs, config_peer_description_newline_rejected); + ATF_ADD_TEST_CASE(tcs, config_clean_free_text_accepted); ATF_ADD_TEST_CASE(tcs, render_emits_interface_section); ATF_ADD_TEST_CASE(tcs, render_csv_joins_addresses); ATF_ADD_TEST_CASE(tcs, render_omits_optional_listenport);