From 2fbf3fb977c02c5fed5b2b8b6796902a270bc1c5 Mon Sep 17 00:00:00 2001 From: Glen Ko Date: Mon, 28 Sep 2026 21:24:43 +0000 Subject: [PATCH] fix(cli): fold per-user /home entry into the alias loop, add test seam Address review on #2309: - Fold the /home/ handling into the existing #2175 alias loop via a shared activation_alias_map() that builds the mapped path in a stack buffer and returns activation_string_copy(); raw allocator sites in activation_transaction.c drop from 80 to 78 (baseline lowered). - Comment the deliberate difference: the per-user entry may resolve to a directory owned by the current account, the whole-alias case may not. - Add cbm_activation_transaction_set_home_root_for_testing() (test seams only) and a Linux test that fails without the per-user branch. Co-authored-by: Claude Signed-off-by: Glen Ko --- scripts/memory-core-baseline.txt | 2 +- src/cli/activation_transaction.c | 122 ++++++++++++++-------------- src/cli/activation_transaction.h | 5 ++ tests/test_activation_transaction.c | 90 ++++++++++++++++++++ 4 files changed, 159 insertions(+), 60 deletions(-) diff --git a/scripts/memory-core-baseline.txt b/scripts/memory-core-baseline.txt index 138ef0f77..5bbae344a 100644 --- a/scripts/memory-core-baseline.txt +++ b/scripts/memory-core-baseline.txt @@ -14,7 +14,7 @@ internal/cbm/lsp/py_lsp.c 2 internal/cbm/macro_table.c 1 internal/cbm/service_patterns.c 2 internal/cbm/sqlite_writer.c 74 -src/cli/activation_transaction.c 80 +src/cli/activation_transaction.c 78 src/cli/agent_clients.c 73 src/cli/agent_profiles.c 7 src/cli/cli.c 170 diff --git a/src/cli/activation_transaction.c b/src/cli/activation_transaction.c index fb7b5983a..f526b2c9c 100644 --- a/src/cli/activation_transaction.c +++ b/src/cli/activation_transaction.c @@ -890,49 +890,44 @@ static bool activation_posix_acl_empty(int descriptor) { } #ifdef __linux__ -/* Managed Linux hosts often keep /home a real directory and point the per-user - * entry elsewhere (/home/alice -> /local/home/alice). Trust that entry only - * when root owns it inside a /home nobody else can write, and it resolves to a - * directory owned by root or the current user. Returns NULL when not - * applicable; the O_NOFOLLOW walk then rejects the path as before. */ -static char *activation_linux_home_entry_path(const char *directory) { - static const char home_prefix[] = "/home/"; - size_t prefix_length = sizeof(home_prefix) - 1U; - if (strncmp(directory, home_prefix, prefix_length) != 0) { - return NULL; - } - size_t name_length = strcspn(directory + prefix_length, "/"); - char entry[sizeof(home_prefix) + 256U]; - if (name_length == 0 || name_length >= 256U) { - return NULL; - } - memcpy(entry, directory, prefix_length + name_length); - entry[prefix_length + name_length] = '\0'; +/* Root of the per-user home tree and the account trusted to own it. Root and + * "/home" in production; the test seam substitutes a fixture tree and the test + * account, because the real layout needs root-owned entries to reproduce. */ +static const char *g_activation_home_root = "/home"; +static uid_t g_activation_home_owner = 0; - struct stat home_status; - struct stat entry_status; - struct stat resolved_status; +#ifdef CBM_ENABLE_TEST_SEAMS +void cbm_activation_transaction_set_home_root_for_testing(const char *home_root, + unsigned long owner_uid) { + g_activation_home_root = home_root ? home_root : "/home"; + g_activation_home_owner = home_root ? (uid_t)owner_uid : 0; +} +#endif +#endif + +#if defined(__APPLE__) || defined(__linux__) +/* Map a trusted symlink `link` (already lstat'd) onto its resolved target and + * append `rest`. The link and the target must both belong to `owner`; with + * accept_own_target the target may instead belong to the current account. + * Returns NULL when either check fails or the mapped path does not fit. */ +static char *activation_alias_map(const char *link, const struct stat *link_status, uid_t owner, + bool accept_own_target, const char *rest) { char resolved[4096]; - if (lstat("/home", &home_status) != 0 || !S_ISDIR(home_status.st_mode) || - home_status.st_uid != 0 || (home_status.st_mode & 0022) != 0 || - lstat(entry, &entry_status) != 0 || !S_ISLNK(entry_status.st_mode) || - entry_status.st_uid != 0 || !realpath(entry, resolved) || - lstat(resolved, &resolved_status) != 0 || !S_ISDIR(resolved_status.st_mode) || - (resolved_status.st_uid != 0 && resolved_status.st_uid != geteuid())) { + struct stat resolved_status; + if (link_status->st_uid != owner || !realpath(link, resolved) || + lstat(resolved, &resolved_status) != 0 || !S_ISDIR(resolved_status.st_mode)) { return NULL; } - const char *rest = directory + prefix_length + name_length; - size_t needed = strlen(resolved) + strlen(rest) + 1U; - char *mapped = malloc(needed); - if (!mapped) { + if (resolved_status.st_uid != owner && + !(accept_own_target && resolved_status.st_uid == geteuid())) { return NULL; } - int written = snprintf(mapped, needed, "%s%s", resolved, rest); - if (written <= 0 || (size_t)written >= needed) { - free(mapped); + char mapped[4096]; + int written = snprintf(mapped, sizeof(mapped), "%s%s", resolved, rest); + if (written <= 0 || (size_t)written >= sizeof(mapped)) { return NULL; } - return mapped; + return activation_string_copy(mapped); } #endif @@ -943,7 +938,7 @@ static char *activation_posix_walk_path(const char *directory) { * Resolve only these trusted system aliases; arbitrary user symlinks must * still fail the O_NOFOLLOW walk below. */ #ifdef __linux__ - static const char *const aliases[] = {"/tmp", "/var", "/home"}; + const char *const aliases[] = {"/tmp", "/var", g_activation_home_root}; #else static const char *const aliases[] = {"/tmp", "/var"}; #endif @@ -954,39 +949,48 @@ static char *activation_posix_walk_path(const char *directory) { (directory[alias_length] != '\0' && directory[alias_length] != '/')) { continue; } + uid_t owner = 0; +#ifdef __linux__ + bool is_home = alias == g_activation_home_root; + if (is_home) { + owner = g_activation_home_owner; + } +#endif struct stat alias_status; - char resolved[4096]; if (lstat(alias, &alias_status) != 0) { continue; } - if (!S_ISLNK(alias_status.st_mode)) { + if (S_ISLNK(alias_status.st_mode)) { + return activation_alias_map(alias, &alias_status, owner, false, + directory + alias_length); + } +#ifdef __linux__ + /* Managed Linux hosts often keep /home a real directory and point the + * per-user entry elsewhere (/home/alice -> /local/home/alice, #2306). + * Trust that entry only inside a root-owned /home nobody else can + * write, and only when root owns the entry itself. Unlike the whole- + * alias case above, the target may belong to the current account: + * that is the normal shape of a relocated home, and an account can + * already write its own home, so it gains nothing it did not have. */ + const char *name = directory + alias_length; + size_t name_length = *name == '/' ? strcspn(name + 1, "/") : 0; + if (!is_home || !S_ISDIR(alias_status.st_mode) || alias_status.st_uid != owner || + (alias_status.st_mode & 0022) != 0 || name_length == 0) { continue; } - if (alias_status.st_uid != 0 || !realpath(alias, resolved)) { + char entry[4096]; + size_t entry_length = alias_length + 1U + name_length; + if (entry_length >= sizeof(entry)) { return NULL; } - struct stat resolved_status; - if (lstat(resolved, &resolved_status) != 0 || !S_ISDIR(resolved_status.st_mode) || - resolved_status.st_uid != 0) { - return NULL; + memcpy(entry, directory, entry_length); + entry[entry_length] = '\0'; + struct stat entry_status; + if (lstat(entry, &entry_status) == 0 && S_ISLNK(entry_status.st_mode)) { + return activation_alias_map(entry, &entry_status, owner, true, + directory + entry_length); } - size_t needed = strlen(resolved) + strlen(directory + alias_length) + 1U; - char *mapped = malloc(needed); - if (!mapped) { - return NULL; - } - int written = snprintf(mapped, needed, "%s%s", resolved, directory + alias_length); - if (written <= 0 || (size_t)written >= needed) { - free(mapped); - return NULL; - } - return mapped; - } #endif -#ifdef __linux__ - char *home_entry = activation_linux_home_entry_path(directory); - if (home_entry) { - return home_entry; } #endif return activation_string_copy(directory); diff --git a/src/cli/activation_transaction.h b/src/cli/activation_transaction.h index c0dd89bfc..591f655fb 100644 --- a/src/cli/activation_transaction.h +++ b/src/cli/activation_transaction.h @@ -100,6 +100,11 @@ const char *cbm_activation_transaction_refusal_note(void); #ifdef CBM_ENABLE_TEST_SEAMS void cbm_activation_transaction_note_refusal_for_testing(const char *predicate, unsigned long os_error); +/* Linux only: stand home_root in for /home and owner_uid in for root when + * deciding whether /home or a /home/ entry is a trusted alias. NULL + * restores the production values. Inert elsewhere. */ +void cbm_activation_transaction_set_home_root_for_testing(const char *home_root, + unsigned long owner_uid); #endif #endif /* CBM_ACTIVATION_TRANSACTION_H */ diff --git a/tests/test_activation_transaction.c b/tests/test_activation_transaction.c index b984145f5..28f185f56 100644 --- a/tests/test_activation_transaction.c +++ b/tests/test_activation_transaction.c @@ -764,6 +764,95 @@ TEST(activation_transaction_rejects_symlink_candidate_target_and_parent) { PASS(); } +/* #2306: managed Linux hosts keep /home a real directory and point the + * per-user entry elsewhere (/home/alice -> /local/home/alice). Root owns that + * entry, so it is trusted like the #2175 /home alias; the seam stands a + * fixture tree in for /home and the test account in for root. Every status is + * captured before the seam is reset so a failure cannot leak into the + * following tests. */ +TEST(activation_transaction_follows_trusted_per_user_home_entry) { +#if defined(__linux__) && defined(CBM_ENABLE_TEST_SEAMS) + char root[ACTIVATION_TEST_PATH_CAP]; + char home[ACTIVATION_TEST_PATH_CAP]; + char entry[ACTIVATION_TEST_PATH_CAP]; + char relocated[ACTIVATION_TEST_PATH_CAP]; + char relocated_bin[ACTIVATION_TEST_PATH_CAP]; + char relocated_target[ACTIVATION_TEST_PATH_CAP]; + char target[ACTIVATION_TEST_PATH_CAP]; + ASSERT_TRUE(activation_test_fixture(root)); + ASSERT_TRUE(activation_test_path(home, root, "home")); + ASSERT_TRUE(activation_test_path(entry, home, "alice")); + ASSERT_TRUE(activation_test_path(relocated, root, "local-alice")); + ASSERT_TRUE(activation_test_path(relocated_bin, relocated, "bin")); + ASSERT_TRUE(activation_test_path(relocated_target, relocated_bin, "cbm")); + ASSERT_TRUE(activation_test_path(target, entry, "bin/cbm")); + ASSERT_TRUE(cbm_mkdir_p(home, 0755)); + ASSERT_EQ(chmod(home, 0755), 0); + ASSERT_TRUE(cbm_mkdir_p(relocated_bin, 0700)); + ASSERT_EQ(symlink(relocated, entry), 0); + unsigned long self = (unsigned long)geteuid(); + + /* Without the seam the entry is an ordinary user symlink: refused. */ + cbm_activation_transaction_t *transaction = NULL; + cbm_activation_transaction_status_t untrusted_status = cbm_activation_transaction_stage_bytes( + target, "candidate", strlen("candidate"), &transaction); + bool untrusted_created = transaction != NULL; + (void)cbm_activation_transaction_close(&transaction); + + /* The home root and the entry must belong to the trusted owner. */ + cbm_activation_transaction_set_home_root_for_testing(home, self + 1U); + cbm_activation_transaction_status_t foreign_owner_status = + cbm_activation_transaction_stage_bytes(target, "candidate", strlen("candidate"), + &transaction); + bool foreign_owner_created = transaction != NULL; + (void)cbm_activation_transaction_close(&transaction); + + /* A home root others can write could have its entry swapped: refused. */ + cbm_activation_transaction_set_home_root_for_testing(home, self); + (void)chmod(home, 0775); + cbm_activation_transaction_status_t writable_home_status = + cbm_activation_transaction_stage_bytes(target, "candidate", strlen("candidate"), + &transaction); + bool writable_home_created = transaction != NULL; + (void)cbm_activation_transaction_close(&transaction); + (void)chmod(home, 0755); + + /* Trusted root, trusted entry: staged and committed into the target. */ + cbm_activation_transaction_status_t trusted_status = cbm_activation_transaction_stage_bytes( + target, "candidate", strlen("candidate"), &transaction); + cbm_activation_transaction_status_t commit_status = + transaction ? cbm_activation_transaction_commit(transaction, NULL, NULL) + : CBM_ACTIVATION_TRANSACTION_INVALID_STATE; + cbm_activation_transaction_status_t finalize_status = + transaction ? cbm_activation_transaction_finalize(transaction) + : CBM_ACTIVATION_TRANSACTION_INVALID_STATE; + cbm_activation_transaction_status_t close_status = + cbm_activation_transaction_close(&transaction); + cbm_activation_transaction_set_home_root_for_testing(NULL, 0); + + char contents[ACTIVATION_TEST_CONTENT_CAP] = ""; + bool published = activation_test_read(relocated_target, contents); + int unlink_status = unlink(entry); + int cleanup_status = th_rmtree(root); + + ASSERT_EQ(untrusted_status, CBM_ACTIVATION_TRANSACTION_IO); + ASSERT_FALSE(untrusted_created); + ASSERT_EQ(foreign_owner_status, CBM_ACTIVATION_TRANSACTION_IO); + ASSERT_FALSE(foreign_owner_created); + ASSERT_EQ(writable_home_status, CBM_ACTIVATION_TRANSACTION_IO); + ASSERT_FALSE(writable_home_created); + ASSERT_EQ(trusted_status, CBM_ACTIVATION_TRANSACTION_OK); + ASSERT_EQ(commit_status, CBM_ACTIVATION_TRANSACTION_OK); + ASSERT_EQ(finalize_status, CBM_ACTIVATION_TRANSACTION_OK); + ASSERT_EQ(close_status, CBM_ACTIVATION_TRANSACTION_OK); + ASSERT_TRUE(published); + ASSERT_STR_EQ(contents, "candidate"); + ASSERT_EQ(unlink_status, 0); + ASSERT_EQ(cleanup_status, 0); +#endif + PASS(); +} + TEST(activation_transaction_fails_closed_if_target_directory_is_replaced) { char root[ACTIVATION_TEST_PATH_CAP]; char active[ACTIVATION_TEST_PATH_CAP]; @@ -1089,6 +1178,7 @@ SUITE(activation_transaction) { RUN_TEST(activation_transaction_admits_group_writable_ancestor); RUN_TEST(activation_transaction_rejects_windows_callback_allow_directory_ace); RUN_TEST(activation_transaction_rejects_symlink_candidate_target_and_parent); + RUN_TEST(activation_transaction_follows_trusted_per_user_home_entry); RUN_TEST(activation_transaction_fails_closed_if_target_directory_is_replaced); RUN_TEST(activation_transaction_does_not_replace_target_created_at_publish_boundary); RUN_TEST(activation_transaction_rejects_macos_mutating_extended_acl);