Merge pull request #1531 from Kiborgik/fix/private-directory-dacl-inheritable

fix(daemon): make private directory DACL inheritable

Author: DeusData

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

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