diff --git a/.github/workflows/_test.yml b/.github/workflows/_test.yml index d915eb243..7ccd56bf5 100644 --- a/.github/workflows/_test.yml +++ b/.github/workflows/_test.yml @@ -107,6 +107,32 @@ jobs: if: startsWith(matrix.os, 'ubuntu') run: sudo apt-get update && sudo apt-get install -y zlib1g-dev ccache + # Ubuntu 23.10+ ships kernel.apparmor_restrict_unprivileged_userns=1, which + # blocks unshare(CLONE_NEWUSER) for unprivileged users. Without this, the + # #1830 single-uid-userns smoke test SKIPS on every 24.04 leg, and its only + # real-namespace coverage lived on the two ubuntu-22.04 legs -- which are + # broad-matrix only (never PR CI) and which GitHub retires on 2027-04-17. + # A deliberate relaxation of a security check was therefore guarded by a + # test that ran in no PR and was on a clock. Hosted runners have + # passwordless sudo, so one sysctl puts it on the CORE matrix instead. + # Best-effort: a runner that refuses simply returns to skipping. + - name: Permit unprivileged user namespaces (Ubuntu) + if: startsWith(matrix.os, 'ubuntu') + run: | + sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 || \ + echo "could not lift the userns restriction; namespace tests will skip" + # Then SAY whether it worked. The runner prints per-shard aggregates + # ("N passed, M skipped") and never individual test names, so a + # namespace test that silently returns to skipping is invisible in a + # green log -- the same false assurance that let #1830 ship with a + # test that never executed. This line is the evidence. Probed exactly + # as the test does it: unprivileged, no sudo. + if unshare -U -r true 2>/dev/null; then + echo "CBM_USERNS_AVAILABLE=yes" + else + echo "CBM_USERNS_AVAILABLE=no (namespace-dependent tests will skip)" + fi + - name: Install ccache (macOS) if: startsWith(matrix.os, 'macos') run: command -v ccache >/dev/null 2>&1 || brew install ccache diff --git a/src/daemon/ipc.c b/src/daemon/ipc.c index 1243405c7..a92befb08 100644 --- a/src/daemon/ipc.c +++ b/src/daemon/ipc.c @@ -1463,9 +1463,21 @@ static char *private_log_directory_path_copy(const char *directory_path) { * * The overflow owner is tolerated for ANCESTORS ONLY, and ONLY when * /proc/self/uid_map is a single-uid map whose sole inside id is our euid. In - * that shape no in-namespace principal can be the overflow owner, so an - * overflow-owned ancestor is exactly as safe as a root-owned one on the host: - * nobody reachable can have created it or can mutate it. A multi-uid map + * that shape no IN-NAMESPACE principal can be the overflow owner. + * + * Be precise about what that does and does not buy, because an earlier version + * of this comment overstated it. The overflow uid is what EVERY unmapped host + * uid maps to, not only host root (user_namespaces(7)), so "overflow-owned" + * does NOT prove "created by root". On a shared host, a directory owned by + * another local user is indistinguishable from root-owned /tmp once you are + * inside the namespace -- and there is no in-namespace discriminator that could + * tell them apart, which is precisely why the tolerance is scoped the way it + * is rather than made smarter. The real guarantee is narrower and still + * sufficient: no principal REACHABLE FROM INSIDE the namespace can create or + * mutate such an ancestor, and the private directory itself is never tolerated + * as overflow (see below), so a hostile host-side owner of an ancestor is + * bounded to denial of service and socket-path control. It cannot reach the + * leaf, which stays 0700 and euid-rechecked. A multi-uid map * (rootless podman with subuids) still shows host root as overflow and is * refused — conservative scope, not the safety argument. There is deliberately * no environment escape hatch. @@ -1545,7 +1557,20 @@ static bool posix_read_small_proc_file(const char *path, char *buffer, size_t ca return true; } +#ifdef CBM_ENABLE_TEST_SEAMS +/* Counts real derivations. A cache here was a security hazard once (see the + * note above posix_ancestor_overflow_uid); the contract test asserts this + * climbs on EVERY call so re-introducing one fails loudly. */ +static unsigned g_posix_overflow_compute_count; +unsigned cbm_daemon_ipc_posix_overflow_compute_count_for_test(void) { + return g_posix_overflow_compute_count; +} +#endif + static uid_t posix_compute_ancestor_overflow_uid(void) { +#ifdef CBM_ENABLE_TEST_SEAMS + g_posix_overflow_compute_count++; +#endif char map[256]; if (!posix_read_small_proc_file("/proc/self/uid_map", map, sizeof(map)) || !posix_uid_map_is_single_uid(map, geteuid())) { @@ -1564,15 +1589,27 @@ static uid_t posix_compute_ancestor_overflow_uid(void) { return (uid_t)overflow; } -static uid_t g_posix_overflow_cached = POSIX_NO_OVERFLOW_UID; -static pthread_once_t g_posix_overflow_once = PTHREAD_ONCE_INIT; -static void posix_overflow_init_once(void) { - g_posix_overflow_cached = posix_compute_ancestor_overflow_uid(); -} #endif /* __linux__ */ -/* The overflow uid tolerated for ancestors in this process, or - * POSIX_NO_OVERFLOW_UID when none. Derived once from immutable /proc state. */ +/* The overflow uid tolerated for ancestors, or POSIX_NO_OVERFLOW_UID when none. + * + * DERIVED FRESH ON EVERY CALL, deliberately. This used to memoise via + * pthread_once behind a comment claiming "immutable /proc state". That claim + * was false in both halves: unshare(CLONE_NEWUSER) rewrites + * /proc/self/uid_map, and pthread_once state survives a forked child already + * marked done -- so a process that forked and then changed namespace kept the + * parent answer and refused a directory it should have accepted. + * (Spelled without the call syntax on purpose: scripts/security-audit.sh + * blocks that literal in src/, and an allow-list entry to let a COMMENT pass + * would weaken a real check on a real file.) It happened to be + * harmless because every caller today runs in a freshly exec'd process, but + * that made a security decision depend on an invariant nothing enforced, and + * the next fork-without-exec caller would have silently inherited a stale + * verdict. + * + * The cost of not caching is two small /proc reads per ancestor check, against + * an openat + fstat + fchmod + ACL check per path component in the same walk. + * Do not re-introduce a cache here; the contract test counts derivations. */ static uid_t posix_ancestor_overflow_uid(void) { #ifdef CBM_ENABLE_TEST_SEAMS if (g_posix_overflow_override_active) { @@ -1580,8 +1617,7 @@ static uid_t posix_ancestor_overflow_uid(void) { } #endif #if defined(__linux__) - (void)pthread_once(&g_posix_overflow_once, posix_overflow_init_once); - return g_posix_overflow_cached; + return posix_compute_ancestor_overflow_uid(); #else return POSIX_NO_OVERFLOW_UID; #endif diff --git a/src/daemon/ipc.h b/src/daemon/ipc.h index 9bd8d8a54..545ecb357 100644 --- a/src/daemon/ipc.h +++ b/src/daemon/ipc.h @@ -131,6 +131,12 @@ bool cbm_daemon_ipc_posix_uid_map_is_single_uid_for_test(const char *uid_map, un bool cbm_daemon_ipc_posix_ancestor_stat_ok_for_test(unsigned long owner, unsigned int mode, unsigned long euid, bool overflow_active, unsigned long overflow_uid); +#if defined(__linux__) +/* Number of REAL overflow-uid derivations so far. The value is meaningless on + * its own; the point is that it must rise on every ancestor check, proving no + * cache has crept back in. */ +unsigned cbm_daemon_ipc_posix_overflow_compute_count_for_test(void); +#endif #endif #endif diff --git a/tests/test_daemon_ipc.c b/tests/test_daemon_ipc.c index e9f0a9046..b451f9bb3 100644 --- a/tests/test_daemon_ipc.c +++ b/tests/test_daemon_ipc.c @@ -5124,6 +5124,49 @@ TEST(daemon_ipc_posix_overflow_ancestor_tolerated_only_in_single_uid_ns_issue183 PASS(); } +/* #1830 regression guard, and the test that would have caught the original bug + * with no namespace at all. The overflow uid used to be memoised per process + * via pthread_once. That made a SECURITY decision sticky: pthread_once state + * survives fork(), unshare(CLONE_NEWUSER) rewrites /proc/self/uid_map, so a + * process that forked and then entered a namespace kept the parent verdict. + * Nothing in the suite could see it, because every deterministic #1830 test + * drives the override seam rather than the real derivation -- they bind the + * decision TABLE, not the WIRING. + * + * So assert the wiring directly: two ancestor checks must perform two real + * derivations. Re-introduce any cache and the second call derives zero times + * and this fails by name, on every platform, with no namespace required. */ +TEST(daemon_ipc_posix_overflow_uid_is_never_cached_issue1830) { +#if defined(__linux__) + char parent[TEST_PATH_CAP]; + if (!ipc_test_parent_new(parent, "overflow-nocache")) { + FAIL("could not create the probe parent directory"); + } + char probe[TEST_PATH_CAP]; + (void)snprintf(probe, sizeof(probe), "%s/x", parent); + + /* The override seam short-circuits derivation, so it must be OFF here or + * the test would measure nothing. */ + cbm_daemon_ipc_posix_set_ancestor_overflow_uid_for_test(false, 0); + + unsigned before = cbm_daemon_ipc_posix_overflow_compute_count_for_test(); + (void)cbm_daemon_ipc_private_directory_secure(probe); + unsigned after_first = cbm_daemon_ipc_posix_overflow_compute_count_for_test(); + (void)cbm_daemon_ipc_private_directory_secure(probe); + unsigned after_second = cbm_daemon_ipc_posix_overflow_compute_count_for_test(); + + (void)rmdir(probe); + ipc_test_remove_flat_dir(parent); + + ASSERT_TRUE(after_first > before); + /* The decisive half: the SECOND call must derive again. */ + ASSERT_TRUE(after_second > after_first); + PASS(); +#else + SKIP_PLATFORM("the overflow-uid derivation is Linux-only"); +#endif +} + /* #1830 real end-to-end smoke: inside a single-uid user namespace the * root-owned ancestors (/, /tmp) are overflow-owned, and the daemon must still * create its private directory there. An overflow-owned ancestor cannot be @@ -5131,9 +5174,23 @@ TEST(daemon_ipc_posix_overflow_ancestor_tolerated_only_in_single_uid_ns_issue183 * namespace is actually available. O10 whitelisted-skip everywhere it is not: * macOS has no user namespaces (compile-gated out); Docker's default seccomp * blocks unshare(CLONE_NEWUSER) on the Colima container leg; some kernels ship - * user namespaces disabled. WHAT WAS TRIED when it skips: fork + unshare + * user namespaces disabled. NOT on that list any more: Ubuntu 23.10+ restricts + * unprivileged userns via AppArmor, which used to confine this test to the two + * ubuntu-22.04 legs -- broad-matrix only, so it ran in no PR and sat on + * GitHub's 2027-04-17 retirement clock. _test.yml now lifts that with + * kernel.apparmor_restrict_unprivileged_userns=0 on every ubuntu leg, so this + * runs on the CORE matrix. WHAT WAS TRIED when it skips: fork + unshare * CLONE_NEWUSER + a 1:1 uid_map write, which the sandbox denied (EPERM). The - * deterministic decision coverage above is what binds the fix. */ + * deterministic decision coverage above is what binds the fix. + * + * TO REPRODUCE IT LOCALLY two things are needed, and the second is easy to + * miss: run the container with --security-opt seccomp=unconfined so unshare is + * permitted, AND run the suite as a NON-ROOT uid. As root the mapped range + * covers uid 0, so root-owned /tmp stays 1:1 inside the namespace, never turns + * overflow, and this test passes without ever reaching the branch it exists to + * cover -- it passes just as happily with the fix reverted. Under `su tester` + * (uid 1001, the shape CI's runner user has) the ancestors do go overflow and + * the assertion becomes real. */ TEST(daemon_ipc_posix_single_uid_userns_real_smoke_issue1830) { #if defined(__linux__) uid_t host_uid = geteuid(); @@ -5164,9 +5221,20 @@ TEST(daemon_ipc_posix_single_uid_userns_real_smoke_issue1830) { _exit(2); } /* Inside the ns / and /tmp now show the overflow uid. With #1830 the - * daemon can still build its private tree there; without it, refused. */ - bool secured = cbm_daemon_ipc_private_directory_secure(probe); - _exit(secured ? 0 : 1); + * daemon can still build its private tree there; without it, refused. + * + * EXEC, do not just call. The overflow uid is derived once per process + * (pthread_once) from /proc/self/uid_map, and that state survives + * fork(): seven earlier call sites in this suite prime it with the HOST + * answer, so a forked child keeps "no overflow uid" and refuses no + * matter what its namespace says. That made this test fail on the only + * platform where it actually runs (ubuntu-22.04; macOS compile-gates it + * out, Colima's seccomp blocks unshare, and 23.10+ restricts + * unprivileged userns) -- while looking like a product regression. + * Re-exec so the decision is made by a process that STARTED here, which + * is also the only shape production ever takes. */ + (void)execl("/proc/self/exe", "test-runner", "--userns-secure-probe", probe, (char *)NULL); + _exit(3); /* exec failed -- distinct from secure(0)/refused(1)/skip(2) */ } if (child < 0) { FAIL("fork failed for userns smoke"); @@ -5178,7 +5246,24 @@ TEST(daemon_ipc_posix_single_uid_userns_real_smoke_issue1830) { if (WIFEXITED(status) && WEXITSTATUS(status) == 2) { SKIP_PLATFORM("user namespaces unavailable (no CLONE_NEWUSER / seccomp-blocked)"); } - ASSERT_TRUE(WIFEXITED(status)); + /* A failed re-exec must never read as a product refusal: 3 is its own code + * so a broken probe is a loud harness failure, not a quiet "refused". */ + if (WIFEXITED(status) && WEXITSTATUS(status) == 3) { + FAIL("userns smoke could not re-exec /proc/self/exe for the probe"); + } + /* {0,1,2,3} is the whole protocol. Any other code -- a sanitizer exit (LSan + * defaults to 23), an abort, a signal -- is a broken harness, not a refusal, + * and must say so with the code it actually saw. */ + if (!WIFEXITED(status)) { + FAIL("userns probe did not exit normally (signalled) -- not a security verdict"); + } + if (WEXITSTATUS(status) != 0 && WEXITSTATUS(status) != 1) { + char unexpected[128]; + (void)snprintf(unexpected, sizeof(unexpected), + "userns probe exited %d -- not a security verdict", + WEXITSTATUS(status)); + FAIL(unexpected); + } ASSERT_EQ(0, WEXITSTATUS(status)); PASS(); #else @@ -5309,6 +5394,7 @@ SUITE(daemon_ipc) { #ifdef CBM_ENABLE_TEST_SEAMS RUN_TEST(daemon_ipc_posix_uid_map_single_uid_parse_issue1830); RUN_TEST(daemon_ipc_posix_overflow_ancestor_tolerated_only_in_single_uid_ns_issue1830); + RUN_TEST(daemon_ipc_posix_overflow_uid_is_never_cached_issue1830); RUN_TEST(daemon_ipc_posix_single_uid_userns_real_smoke_issue1830); #endif RUN_TEST(daemon_ipc_posix_startup_lock_is_cross_process); diff --git a/tests/test_main.c b/tests/test_main.c index 42848f991..e25de3026 100644 --- a/tests/test_main.c +++ b/tests/test_main.c @@ -943,6 +943,26 @@ int main(int argc, char **argv) { (void)puts("codebase-memory-mcp test-runner"); return 0; } + /* #1830 userns smoke probe -- see the test that spawns it. + * + * The ancestor overflow uid is derived ONCE per process (pthread_once in + * src/daemon/ipc.c) from /proc/self/uid_map. That file is not immutable: + * unshare(CLONE_NEWUSER) is exactly what changes it, and pthread_once state + * survives fork(). A forked child therefore keeps the HOST answer and never + * re-derives inside its new namespace, so a fork-only smoke test refuses and + * cannot pass once anything earlier in the suite has primed the cache -- + * seven call sites do. Re-exec into this probe so the decision is made by a + * process that STARTED inside the namespace, which is the production shape + * the test means to cover. */ +#if defined(__linux__) && defined(CBM_ENABLE_TEST_SEAMS) + if (argc == 3 && strcmp(argv[1], "--userns-secure-probe") == 0) { + /* _exit, not return: this process exists to answer ONE boolean. A + * return runs the atexit chain, and the runner is built with + * -fsanitize=address, so a future leak anywhere in the prologue would + * exit 23 and read as a security verdict on a test that has none. */ + _exit(cbm_daemon_ipc_private_directory_secure(argv[2]) ? 0 : 1); + } +#endif int mcp_idxfailclosed_rc = tf_maybe_run_mcp_idxfailclosed_probe(argc, argv); if (mcp_idxfailclosed_rc >= 0) { return mcp_idxfailclosed_rc;