diff --git a/src/daemon/ipc.c b/src/daemon/ipc.c index 5b0a76f80..92a36b744 100644 --- a/src/daemon/ipc.c +++ b/src/daemon/ipc.c @@ -3312,6 +3312,7 @@ typedef BOOL(WINAPI *is_well_known_sid_fn)(PSID, WELL_KNOWN_SID_TYPE); typedef BOOL(WINAPI *is_valid_acl_fn)(PACL); typedef BOOL(WINAPI *initialize_acl_fn)(PACL, DWORD, DWORD); typedef BOOL(WINAPI *add_access_allowed_ace_fn)(PACL, DWORD, DWORD, PSID); +typedef BOOL(WINAPI *add_access_allowed_ace_ex_fn)(PACL, DWORD, DWORD, DWORD, PSID); typedef BOOL(WINAPI *initialize_security_descriptor_fn)(PSECURITY_DESCRIPTOR, DWORD); typedef BOOL(WINAPI *set_security_descriptor_dacl_fn)(PSECURITY_DESCRIPTOR, BOOL, PACL, BOOL); typedef BOOL(WINAPI *set_security_descriptor_owner_fn)(PSECURITY_DESCRIPTOR, PSID, BOOL); @@ -3339,6 +3340,7 @@ typedef struct { is_valid_acl_fn is_valid_acl; initialize_acl_fn initialize_acl; add_access_allowed_ace_fn add_access_allowed_ace; + add_access_allowed_ace_ex_fn add_access_allowed_ace_ex; initialize_security_descriptor_fn initialize_security_descriptor; set_security_descriptor_dacl_fn set_security_descriptor_dacl; set_security_descriptor_owner_fn set_security_descriptor_owner; @@ -3352,6 +3354,9 @@ typedef struct { PACL acl; PSECURITY_DESCRIPTOR descriptor; SECURITY_ATTRIBUTES attributes; + PACL directory_acl; + PSECURITY_DESCRIPTOR directory_descriptor; + SECURITY_ATTRIBUTES directory_attributes; } win_security_t; typedef struct win_generation_address { @@ -3560,6 +3565,8 @@ static void win_security_destroy(win_security_t *security) { } free(security->descriptor); free(security->acl); + free(security->directory_descriptor); + free(security->directory_acl); free(security->user_sid); if (security->advapi) { (void)FreeLibrary(security->advapi); @@ -3614,6 +3621,8 @@ static bool win_security_init(win_security_t *security) { RESOLVE_ADVAPI_MEMBER(security, initialize_acl, initialize_acl_fn, "InitializeAcl"); RESOLVE_ADVAPI_MEMBER(security, add_access_allowed_ace, add_access_allowed_ace_fn, "AddAccessAllowedAce"); + RESOLVE_ADVAPI_MEMBER(security, add_access_allowed_ace_ex, add_access_allowed_ace_ex_fn, + "AddAccessAllowedAceEx"); RESOLVE_ADVAPI_MEMBER(security, initialize_security_descriptor, initialize_security_descriptor_fn, "InitializeSecurityDescriptor"); RESOLVE_ADVAPI_MEMBER(security, set_security_descriptor_dacl, set_security_descriptor_dacl_fn, @@ -3679,6 +3688,37 @@ static bool win_security_init(win_security_t *security) { security->attributes.nLength = sizeof(security->attributes); security->attributes.lpSecurityDescriptor = security->descriptor; security->attributes.bInheritHandle = FALSE; + + /* Containers need a second, inheritable ACL. AddAccessAllowedAce above + * cannot express inheritance flags, and a flagless ACE applied to a + * directory together with PROTECTED_DACL_SECURITY_INFORMATION yields + * D:PAI(A;;FA;;;): the protection severs the inherited ACEs while + * the new one propagates nothing, so every child is created with an empty + * DACL. Files and kernel objects are leaves and keep the flagless ACL. + * + * The rights here must be specific (FILE_ALL_ACCESS) rather than + * GENERIC_ALL. Windows splits an inheritable generic-rights ACE into an + * effective mapped ACE plus an INHERIT_ONLY one carrying the generic bits, + * and the owner-only DACL validators require exactly one ACE. */ + security->directory_acl = malloc(acl_size); + security->directory_descriptor = malloc(SECURITY_DESCRIPTOR_MIN_LENGTH); + if (!security->directory_acl || !security->directory_descriptor || + !security->initialize_acl(security->directory_acl, (DWORD)acl_size, ACL_REVISION) || + !security->add_access_allowed_ace_ex(security->directory_acl, ACL_REVISION, + CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE, + FILE_ALL_ACCESS, security->user_sid) || + !security->initialize_security_descriptor(security->directory_descriptor, + SECURITY_DESCRIPTOR_REVISION) || + !security->set_security_descriptor_dacl(security->directory_descriptor, TRUE, + security->directory_acl, FALSE) || + !security->set_security_descriptor_owner(security->directory_descriptor, security->user_sid, + FALSE)) { + win_security_destroy(security); + return false; + } + security->directory_attributes.nLength = sizeof(security->directory_attributes); + security->directory_attributes.lpSecurityDescriptor = security->directory_descriptor; + security->directory_attributes.bInheritHandle = FALSE; return true; } @@ -4047,7 +4087,7 @@ static bool win_runtime_directory_secure(const wchar_t *runtime_dir) { if (!win_security_init(&security)) { return false; } - bool created = CreateDirectoryW(runtime_dir, &security.attributes) != 0; + bool created = CreateDirectoryW(runtime_dir, &security.directory_attributes) != 0; if (!created && GetLastError() != ERROR_ALREADY_EXISTS) { win_security_destroy(&security); return false; @@ -4094,7 +4134,7 @@ static bool win_runtime_directory_secure(const wchar_t *runtime_dir) { directory, SE_FILE_OBJECT, (owner_exact ? 0U : (DWORD)OWNER_SECURITY_INFORMATION) | DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, - owner_exact ? NULL : security.user_sid, NULL, security.acl, NULL); + owner_exact ? NULL : security.user_sid, NULL, security.directory_acl, NULL); } if (valid_handle && owner_ok && secure_result != ERROR_SUCCESS) { ipc_validation_detail_set("owner/DACL repair failed (status %lu%s)", @@ -4181,7 +4221,7 @@ static bool win_private_directory_tree_secure(const wchar_t *directory_path) { if (attributes == INVALID_FILE_ATTRIBUTES) { DWORD error = GetLastError(); ok = (error == ERROR_FILE_NOT_FOUND || error == ERROR_PATH_NOT_FOUND) && - CreateDirectoryW(path, &security.attributes) != 0; + CreateDirectoryW(path, &security.directory_attributes) != 0; } /* Ancestors are observe-only and must already be secure. The * final current-user directory is intentionally handled below by diff --git a/tests/test_daemon_ipc.c b/tests/test_daemon_ipc.c index cf9f5fb60..94754230e 100644 --- a/tests/test_daemon_ipc.c +++ b/tests/test_daemon_ipc.c @@ -776,6 +776,108 @@ TEST(daemon_ipc_windows_private_directory_rejects_untrusted_ancestor_acl) { PASS(); } +/* A private directory is a container, so its owner-only ACE must propagate. + * Otherwise the protected DACL severs inheritance and every child is created + * with an empty DACL, unreadable even by its owner. */ +TEST(daemon_ipc_windows_private_directory_ace_is_inheritable) { + char parent[TEST_PATH_CAP] = {0}; + char cache[TEST_PATH_CAP] = {0}; + bool paths_ok = false; + bool secured = false; + bool ace_read = false; + bool ace_single = false; + bool ace_inheritable = false; + bool child_has_dacl = false; + bool lock_directory_accepted = false; + + if (ipc_test_parent_new(parent, "win-inheritable-ace")) { + int written = snprintf(cache, sizeof(cache), "%s/cache", parent); + paths_ok = written > 0 && written < (int)sizeof(cache); + } + if (paths_ok) { + secured = cbm_daemon_ipc_private_directory_secure(cache); + } + if (secured) { + PACL dacl = NULL; + PSECURITY_DESCRIPTOR descriptor = NULL; + LPVOID opaque_ace = NULL; + ACL_SIZE_INFORMATION information; + memset(&information, 0, sizeof(information)); + if (GetNamedSecurityInfoA(cache, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, + &dacl, NULL, &descriptor) == ERROR_SUCCESS && + dacl && + GetAclInformation(dacl, &information, sizeof(information), AclSizeInformation) && + GetAce(dacl, 0, &opaque_ace) && opaque_ace) { + ACCESS_ALLOWED_ACE *ace = (ACCESS_ALLOWED_ACE *)opaque_ace; + ace_read = true; + /* The owner-only validators require exactly one ACE. An + * inheritable ACE built from GENERIC_ALL rather than specific + * rights is split by Windows into an effective ACE plus an + * INHERIT_ONLY one, which fails that check. */ + ace_single = information.AceCount == 1; + ace_inheritable = (ace->Header.AceFlags & CONTAINER_INHERIT_ACE) != 0 && + (ace->Header.AceFlags & OBJECT_INHERIT_ACE) != 0; + } + if (descriptor) { + (void)LocalFree(descriptor); + } + } + if (ace_inheritable) { + char child[TEST_PATH_CAP] = {0}; + int written = snprintf(child, sizeof(child), "%s/child", cache); + if (written > 0 && written < (int)sizeof(child) && CreateDirectoryA(child, NULL) != 0) { + PACL child_dacl = NULL; + PSECURITY_DESCRIPTOR child_descriptor = NULL; + ACL_SIZE_INFORMATION information; + memset(&information, 0, sizeof(information)); + child_has_dacl = + GetNamedSecurityInfoA(child, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, + &child_dacl, NULL, &child_descriptor) == ERROR_SUCCESS && + child_dacl && + GetAclInformation(child_dacl, &information, sizeof(information), + AclSizeInformation) && + information.AceCount > 0; + if (child_descriptor) { + (void)LocalFree(child_descriptor); + } + (void)RemoveDirectoryA(child); + } + } + + if (ace_single) { + /* The owner-only DACL validator gates every project lock: a secured + * directory it refuses takes the whole coordination layer down with + * "secure CLI coordination could not be created (project-locks)". */ + HANDLE handle = + CreateFileA(cache, FILE_READ_ATTRIBUTES | READ_CONTROL, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL, OPEN_EXISTING, + FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT, NULL); + if (handle != INVALID_HANDLE_VALUE) { + cbm_private_lock_directory_t *directory = NULL; + lock_directory_accepted = cbm_private_lock_directory_adopt_windows( + handle, cache, &directory) == CBM_PRIVATE_FILE_LOCK_OK && + directory; + if (directory) { + cbm_private_lock_directory_close(directory); + } else { + (void)CloseHandle(handle); + } + } + } + + (void)RemoveDirectoryA(cache); + ipc_test_remove_flat_dir(parent); + + ASSERT_TRUE(paths_ok); + ASSERT_TRUE(secured); + ASSERT_TRUE(ace_read); + ASSERT_TRUE(ace_single); + ASSERT_TRUE(ace_inheritable); + ASSERT_TRUE(child_has_dacl); + ASSERT_TRUE(lock_directory_accepted); + PASS(); +} + TEST(daemon_ipc_windows_private_directory_allows_add_subdirectory_only_ancestor) { char parent[TEST_PATH_CAP] = {0}; char add_only[TEST_PATH_CAP] = {0}; @@ -4620,6 +4722,7 @@ SUITE(daemon_ipc) { #ifdef _WIN32 RUN_TEST(daemon_ipc_windows_default_endpoint_ignores_temp_environment); RUN_TEST(daemon_ipc_windows_private_directory_rejects_untrusted_ancestor_acl); + RUN_TEST(daemon_ipc_windows_private_directory_ace_is_inheritable); RUN_TEST(daemon_ipc_windows_private_directory_allows_add_subdirectory_only_ancestor); RUN_TEST(daemon_ipc_windows_legacy_bridge_covers_handoff_and_lifetime); RUN_TEST(daemon_ipc_windows_local_transition_atomically_reserves_legacy_pipe);