From e184f9886486927d9008e9bd35a0f9c4e1ccd9dc Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 28 Jun 2026 23:07:20 +0000 Subject: [PATCH] fix(net): error on per-user sub-CIDR uid overflow instead of silent collision (1.1.18) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PerUserNetPure::composeIpv4/composeIpv6 mapped a uid into its sub-CIDR slot with `uid & (slotCount - 1)` — a modulo wrap. Two uids differing by a multiple of 2^slotBits got the SAME sub-CIDR, silently defeating the per-operator network separation the function provides. With the example 10.66.0.0/16 -> /24 (8 slot bits) every uid >= 256 aliased a lower one (uid 1000 and 1256 both -> 10.66.232.0/24). The existing tests deliberately ENCODED this wrap ("uid 256 wraps back to slot 0"), so it was a known design choice, not an accidental bug — but per docs/trust-model.md the per-user network split is honest- operator convenience, and silently handing two honest operators the same subnet is an isolation failure. Per the maintainer's call, convert it to a loud error. Now both functions return an error when `uid >= 2^slotBits`: "uid exceeds the -bit per-user slot space (max uid ); widen the master CIDR or shrink the sub-prefix". The error names the uid so operators can act. Safety: composition is opt-in (only when network_master_cidr_v4/v6 is set), and the authz path's config carries only zfs/path prefixes (no network CIDR), so the 1.1.12 -> 1.1.17 privops gates are unaffected. composeForUid already returns its network error with an "ipv4:"/"ipv6:" prefix; an over-tight config that appeared to work for one operator now reports the limit at crate run. Tests: per_user_net_pure_test replaces the wrap-encoding assertion with v4+v6 overflow-error cases (boundary uid OK, +1 errors, error names the uid) and re-bases typical/adjacency/non-canonical on slot spaces that fit their uids; per_user_env_pure_test widens the example v4 master to /8 (10.3.232.0/24 for uid 1000). Verified via standalone harness (kyua/atf absent in this env); both test files shim-syntax-checked. docs/trust-model.{md,uk.md}: Applies-to -> 1.1.18, subtitle series -> 1.1.17, + a sizing note in the per-user-namespacing section. CHANGELOG [1.1.18] + --version bump. https://claude.ai/code/session_01X6t6tzVypHye5bDGLxzmZK --- CHANGELOG.md | 40 +++++++++++++++++ cli/args.cpp | 4 +- docs/trust-model.md | 14 ++++-- docs/trust-model.uk.md | 14 ++++-- lib/per_user_net_pure.cpp | 26 +++++++++-- tests/unit/per_user_env_pure_test.cpp | 6 +-- tests/unit/per_user_net_pure_test.cpp | 62 +++++++++++++++++++-------- 7 files changed, 133 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68a9a24..8962e1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,46 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). --- +## [1.1.18] — 2026-06-10 + +**Fix: per-user network sub-CIDR silently aliased colliding uids.** +`PerUserNetPure::composeIpv4` / `composeIpv6` mapped a uid into its +sub-CIDR slot with `uid & (slotCount - 1)` — a modulo wrap. Two uids +that differ by a multiple of `2^slotBits` therefore received the +**same** sub-CIDR, silently defeating the per-operator network +separation the function exists to provide. With the example +`10.66.0.0/16` → `/24` config (8 slot bits) every uid ≥ 256 aliased a +lower one — e.g. uid 1000 and uid 1256 both got `10.66.232.0/24`. + +Now the functions **error** when a uid exceeds the slot space +(`uid >= 2^slotBits`) instead of handing out a colliding range: +`uid exceeds the -bit per-user slot space … widen the master +CIDR or shrink the sub-prefix`. Per `docs/trust-model.md` the per-user +network split is honest-operator convenience, not an adversarial +boundary; this turns a silent isolation failure into a loud +configuration error. + +### Operator impact + +Deployments that configured `network_master_cidr_v4` / `_v6` must size +the slot space (`subPrefix − masterPrefix` bits) to their uid range. A +`/16` master with `/24` subs only holds uids 0–255; real uids (≥ 1000) +need a wider master (e.g. `/8` → `/24` = 16 slot bits, 65 536 slots) or +a larger sub-prefix. Composition is opt-in (only when a network master +CIDR is set), and the authz path doesn't compose network CIDRs, so the +1.1.12 → 1.1.17 privops gates are unaffected. An over-tight config that +previously *appeared* to work for a single operator now reports the +limit at `crate run`. + +### Tests + +`per_user_net_pure_test` replaces the `uid 256 wraps back to slot 0` +assertion (which encoded the bug) with overflow-error cases for both +v4 and v6 (boundary uid OK, uid+1 errors, error names the uid) and +re-bases the typical/adjacency/non-canonical cases on slot spaces that +fit their uids. `per_user_env_pure_test` widens the example v4 master +to `/8`. + ## [1.1.17] — 2026-06-10 **Security: close three cross-tenant holes in the privops authz gate.** diff --git a/cli/args.cpp b/cli/args.cpp index ce0cfac..d65c9ce 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.17" << std::endl; + std::cout << "crate 1.1.18" << 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.17" << std::endl; + std::cout << "crate 1.1.18" << 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 a9ea006..306de5b 100644 --- a/docs/trust-model.md +++ b/docs/trust-model.md @@ -4,8 +4,8 @@ operators on one machine) and contributors extending the privileged surface. -**Applies to:** 1.1.17 (rootless model + per-tenant authz series 1.1.12 → -1.1.15 covering every privops verb that carries an operator-controlled +**Applies to:** 1.1.18 (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.17 means reasoning about who can reach +Reasoning about isolation on 1.1.18 means reasoning about who can reach **privops**, not who can run `crate(1)`. --- @@ -160,6 +160,14 @@ sub-CIDRs and an RCTL umbrella class from the connecting operator's uid separates **honest** operators — alice's and bob's `web` jails land in different ZFS subtrees and CIDRs. +> Sizing note (1.1.18): the network sub-CIDR maps each uid into a slot +> of `subPrefix − masterPrefix` bits. The master CIDR / sub-prefix must +> be sized for the operator uid range — a `/16` master with `/24` subs +> holds only uids 0–255, so realistic uids (≥ 1000) need a wider master +> (e.g. `/8` → `/24` = 16 slot bits). An out-of-range uid now **errors** +> at compose time rather than silently aliasing another operator's +> sub-CIDR (pre-1.1.18 it wrapped, two honest operators could collide). + It is **not** an adversarial boundary on the privops plane. The per-uid prefix is computed **client-side** (in `crate(1)`) and passed in the request; `crated` does **not** re-derive or validate the request's diff --git a/docs/trust-model.uk.md b/docs/trust-model.uk.md index 6eea512..2c5b91a 100644 --- a/docs/trust-model.uk.md +++ b/docs/trust-model.uk.md @@ -4,8 +4,8 @@ (кілька операторів на одній машині), і контрибʼютори, які розширюють привілейовану поверхню. -**Стосується:** 1.1.17 (rootless-модель + серія per-tenant authz 1.1.12 → -1.1.15 покриває кожен privops-верб з operator-controlled ownership- +**Стосується:** 1.1.18 (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.17 — це міркувати про те, хто має доступ +Міркувати про ізоляцію на 1.1.18 — це міркувати про те, хто має доступ до **privops**, а не хто може запустити `crate(1)`. --- @@ -157,6 +157,14 @@ sub-CIDR та RCTL-umbrella-клас з uid оператора, що під'єд **чесних** операторів — jail'и `web` від alice і bob осідають у різних ZFS-піддеревах і CIDR. +> Нотатка про розмір (1.1.18): мережевий sub-CIDR кладе кожен uid у slot +> з `subPrefix − masterPrefix` біт. Master-CIDR / sub-prefix треба +> розмірити під діапазон uid операторів — `/16` master з `/24` subs +> вміщає лише uid 0–255, тож реальні uid (≥ 1000) потребують ширшого +> master (напр. `/8` → `/24` = 16 slot-біт). uid поза діапазоном тепер +> **дає помилку** на етапі compose, а не тихо аліасить чужий sub-CIDR +> (до 1.1.18 був wrap, два чесні оператори могли колізнути). + Це **не** ворожостійка межа на privops-площині. Пер-uid-префікс рахується **на боці клієнта** (в `crate(1)`) і передається в запиті; `crated` **не** перевиводить і не валідує `jid` / `dataset` / `path` diff --git a/lib/per_user_net_pure.cpp b/lib/per_user_net_pure.cpp index c3897b6..07e00ee 100644 --- a/lib/per_user_net_pure.cpp +++ b/lib/per_user_net_pure.cpp @@ -189,8 +189,19 @@ Result composeIpv4(const std::string &masterCidr, : (uint32_t)(0xffffffffULL << (32 - split.prefix)); uint32_t base = addr & masterMask; - uint32_t slotCount = 1u << slotBits; - uint32_t slot = uid & (slotCount - 1); + // 1.1.18: refuse to silently wrap a uid that doesn't fit the slot + // space. Pre-1.1.18 this did `uid & (slotCount-1)`, so two uids that + // collide modulo 2^slotBits got the SAME sub-CIDR — defeating the + // per-operator network separation this function exists to provide. + // Error loudly so the operator sizes the master CIDR / sub-prefix to + // their uid range. (slotBits <= 24 here, so 1u<= (1u << slotBits)) + return {"", "uid " + std::to_string(uid) + " exceeds the " + + std::to_string(slotBits) + "-bit per-user slot space " + "(max uid " + std::to_string((1u << slotBits) - 1) + + "); widen the master IPv4 CIDR or shrink the sub-prefix"}; + + uint32_t slot = uid; uint32_t slotShift = 32 - subPrefixLen; uint32_t subBase = base | (slot << slotShift); @@ -227,8 +238,17 @@ Result composeIpv6(const std::string &masterCidr, bytes[bit / 8] &= (uint8_t)~(1u << (7 - (bit % 8))); } + // 1.1.18: same anti-collision guard as composeIpv4 — error rather + // than silently wrap a uid that exceeds the slot space. (slotBits <= + // 32; 1ULL<<32 is fine in uint64_t, and a uint32_t uid is always + // < 2^32, so a 32-bit slot space accepts every uid.) + if ((uint64_t)uid >= (1ULL << slotBits)) + return {"", "uid " + std::to_string(uid) + " exceeds the " + + std::to_string(slotBits) + "-bit per-user slot space; " + "widen the master IPv6 CIDR or shrink the sub-prefix"}; + // Set slot bits at [masterPrefix .. subPrefixLen). - uint64_t slot = (uint64_t)uid & ((1ULL << slotBits) - 1); + uint64_t slot = (uint64_t)uid; for (unsigned i = 0; i < slotBits; i++) { unsigned bitFromTop = slotBits - 1 - i; // MSB of slot first if ((slot >> bitFromTop) & 1ULL) { diff --git a/tests/unit/per_user_env_pure_test.cpp b/tests/unit/per_user_env_pure_test.cpp index 950cb07..5b2fd3c 100644 --- a/tests/unit/per_user_env_pure_test.cpp +++ b/tests/unit/per_user_env_pure_test.cpp @@ -13,7 +13,7 @@ static Config defaultCfg() { Config c; c.zfsMasterPrefix = "zroot/jails"; c.pathMasterPrefix = "/jails-tenants"; - c.networkMasterCidr4 = "10.66.0.0/16"; + c.networkMasterCidr4 = "10.0.0.0/8"; // 1.1.18: /8 so uid 1000 fits the slot space c.networkSubPrefixLen4 = 24; c.networkMasterCidr6 = "fd00:dead::/48"; c.networkSubPrefixLen6 = 64; @@ -37,7 +37,7 @@ ATF_TEST_CASE_BODY(compose_full_config) { ATF_REQUIRE_EQ(r.env.pathPrefix, std::string("/jails-tenants/1000")); ATF_REQUIRE_EQ(r.env.pathMasterPrefix, std::string("/jails-tenants")); // 1.1.17 // Network - ATF_REQUIRE_EQ(r.env.ipv4SubCidr, std::string("10.66.232.0/24")); + ATF_REQUIRE_EQ(r.env.ipv4SubCidr, std::string("10.3.232.0/24")); // /8+/24, uid 1000 ATF_REQUIRE_EQ(r.env.ipv6SubCidr, std::string("fd00:dead:0:3e8:0:0:0:0/64")); // RCTL @@ -83,7 +83,7 @@ ATF_TEST_CASE_BODY(path_prefix_with_different_uids) { ATF_TEST_CASE_WITHOUT_HEAD(compose_v4_only); ATF_TEST_CASE_BODY(compose_v4_only) { Config c; - c.networkMasterCidr4 = "10.0.0.0/16"; + c.networkMasterCidr4 = "10.0.0.0/8"; // 1.1.18: /8 so uid 1000 fits c.networkSubPrefixLen4 = 24; // v6 left empty auto r = composeForUid(c, 1000); diff --git a/tests/unit/per_user_net_pure_test.cpp b/tests/unit/per_user_net_pure_test.cpp index dc49c3e..493212d 100644 --- a/tests/unit/per_user_net_pure_test.cpp +++ b/tests/unit/per_user_net_pure_test.cpp @@ -10,43 +10,56 @@ using namespace PerUserNetPure; // --- IPv4 --- -ATF_TEST_CASE_WITHOUT_HEAD(ipv4_typical_16_to_24); -ATF_TEST_CASE_BODY(ipv4_typical_16_to_24) { - // /16 + /24 → 256 slots; uid 1000 % 256 = 232 - auto r = composeIpv4("10.66.0.0/16", 24, 1000); +ATF_TEST_CASE_WITHOUT_HEAD(ipv4_typical_8_to_24); +ATF_TEST_CASE_BODY(ipv4_typical_8_to_24) { + // /8 + /24 → 16-bit slot space (65536 slots), so a real uid like + // 1000 fits without collision. uid 1000 = 0x03E8 -> 10.3.232.0/24. + auto r = composeIpv4("10.0.0.0/8", 24, 1000); ATF_REQUIRE_EQ(r.error, std::string()); - ATF_REQUIRE_EQ(r.cidr, std::string("10.66.232.0/24")); + ATF_REQUIRE_EQ(r.cidr, std::string("10.3.232.0/24")); // uid 0 → slot 0 - r = composeIpv4("10.66.0.0/16", 24, 0); - ATF_REQUIRE_EQ(r.cidr, std::string("10.66.0.0/24")); + r = composeIpv4("10.0.0.0/8", 24, 0); + ATF_REQUIRE_EQ(r.cidr, std::string("10.0.0.0/24")); +} - // uid 256 wraps back to slot 0 - r = composeIpv4("10.66.0.0/16", 24, 256); - ATF_REQUIRE_EQ(r.cidr, std::string("10.66.0.0/24")); +ATF_TEST_CASE_WITHOUT_HEAD(ipv4_uid_overflowing_slot_space_errors); +ATF_TEST_CASE_BODY(ipv4_uid_overflowing_slot_space_errors) { + // 1.1.18: an 8-bit slot space (/16 + /24) holds uids 0..255. The + // boundary uid 255 is fine; 256 and above must ERROR rather than + // silently wrap to an already-taken slot (pre-1.1.18 they collided). + ATF_REQUIRE_EQ(composeIpv4("10.66.0.0/16", 24, 255).error, std::string()); + ATF_REQUIRE(!composeIpv4("10.66.0.0/16", 24, 256).error.empty()); + ATF_REQUIRE(!composeIpv4("10.66.0.0/16", 24, 1000).error.empty()); + // The error must mention the offending uid so operators can act. + ATF_REQUIRE(composeIpv4("10.66.0.0/16", 24, 256).error.find("256") + != std::string::npos); } ATF_TEST_CASE_WITHOUT_HEAD(ipv4_isolation_alice_vs_bob); ATF_TEST_CASE_BODY(ipv4_isolation_alice_vs_bob) { // Adjacent uids must land in adjacent (non-overlapping) sub-CIDRs. - auto alice = composeIpv4("10.66.0.0/16", 24, 1000); - auto bob = composeIpv4("10.66.0.0/16", 24, 1001); + // Use a /8 master so realistic uids fit the slot space. + auto alice = composeIpv4("10.0.0.0/8", 24, 1000); + auto bob = composeIpv4("10.0.0.0/8", 24, 1001); ATF_REQUIRE_EQ(alice.error, std::string()); ATF_REQUIRE_EQ(bob.error, std::string()); ATF_REQUIRE(alice.cidr != bob.cidr); - // Same /16 root - ATF_REQUIRE(alice.cidr.substr(0, 6) == std::string("10.66.")); - ATF_REQUIRE(bob.cidr.substr(0, 6) == std::string("10.66.")); + // Same /8 root + ATF_REQUIRE(alice.cidr.substr(0, 3) == std::string("10.")); + ATF_REQUIRE(bob.cidr.substr(0, 3) == std::string("10.")); } ATF_TEST_CASE_WITHOUT_HEAD(ipv4_master_low_bits_are_ignored); ATF_TEST_CASE_BODY(ipv4_master_low_bits_are_ignored) { // Operator wrote "10.66.5.7/16" — non-canonical. Compose must // mask off the low 16 bits and produce the same result as with - // "10.66.0.0/16". - auto canonical = composeIpv4("10.66.0.0/16", 24, 1000); - auto sloppy = composeIpv4("10.66.5.7/16", 24, 1000); + // "10.66.0.0/16". (uid 5 fits the 8-bit slot space.) + auto canonical = composeIpv4("10.66.0.0/16", 24, 5); + auto sloppy = composeIpv4("10.66.5.7/16", 24, 5); + ATF_REQUIRE_EQ(canonical.error, std::string()); ATF_REQUIRE_EQ(canonical.cidr, sloppy.cidr); + ATF_REQUIRE_EQ(canonical.cidr, std::string("10.66.5.0/24")); } ATF_TEST_CASE_WITHOUT_HEAD(ipv4_28_subnet); @@ -92,6 +105,15 @@ ATF_TEST_CASE_BODY(ipv6_typical_48_to_64) { ATF_REQUIRE_EQ(r.cidr, std::string("fd00:dead:0:0:0:0:0:0/64")); } +ATF_TEST_CASE_WITHOUT_HEAD(ipv6_uid_overflowing_slot_space_errors); +ATF_TEST_CASE_BODY(ipv6_uid_overflowing_slot_space_errors) { + // 1.1.18: an 8-bit slot space (/64 + /72) holds uids 0..255. + ATF_REQUIRE_EQ(composeIpv6("fd00::/64", 72, 255).error, std::string()); + ATF_REQUIRE(!composeIpv6("fd00::/64", 72, 256).error.empty()); + // A 16-bit slot space (/48 + /64) comfortably holds uid 1000. + ATF_REQUIRE_EQ(composeIpv6("fd00:dead::/48", 64, 1000).error, std::string()); +} + ATF_TEST_CASE_WITHOUT_HEAD(ipv6_isolation_alice_vs_bob); ATF_TEST_CASE_BODY(ipv6_isolation_alice_vs_bob) { auto alice = composeIpv6("fd00:dead::/48", 64, 1000); @@ -126,13 +148,15 @@ ATF_TEST_CASE_BODY(ipv6_rejects_sub_prefix_relations) { } ATF_INIT_TEST_CASES(tcs) { - ATF_ADD_TEST_CASE(tcs, ipv4_typical_16_to_24); + ATF_ADD_TEST_CASE(tcs, ipv4_typical_8_to_24); + ATF_ADD_TEST_CASE(tcs, ipv4_uid_overflowing_slot_space_errors); ATF_ADD_TEST_CASE(tcs, ipv4_isolation_alice_vs_bob); ATF_ADD_TEST_CASE(tcs, ipv4_master_low_bits_are_ignored); ATF_ADD_TEST_CASE(tcs, ipv4_28_subnet); ATF_ADD_TEST_CASE(tcs, ipv4_rejects_bad_master); ATF_ADD_TEST_CASE(tcs, ipv4_rejects_sub_prefix_relations); ATF_ADD_TEST_CASE(tcs, ipv6_typical_48_to_64); + ATF_ADD_TEST_CASE(tcs, ipv6_uid_overflowing_slot_space_errors); ATF_ADD_TEST_CASE(tcs, ipv6_isolation_alice_vs_bob); ATF_ADD_TEST_CASE(tcs, ipv6_master_low_bits_are_ignored); ATF_ADD_TEST_CASE(tcs, ipv6_rejects_bad_master);