Merge pull request #2195 from DeusData/fix/userns-smoke-spawn

fix(daemon,test,ci): make the #1830 userns guard actually run, and delete the cache behind it

.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

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

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
 

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);

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;