Skip to content

Commit 3e540aa

Browse files
committed
fix(daemon): make private directory DACL inheritable
win_security_init built its owner-only ACL with AddAccessAllowedAce, which has no AceFlags parameter and can only emit an ACE with flags set to zero. Applied to a directory together with PROTECTED_DACL_SECURITY_INFORMATION that yields D:PAI(A;;FA;;;<user>): the protection severs the inherited ACEs while the new ACE propagates nothing, so every child created afterwards is born with an empty DACL and is unreadable even by its owner. win_runtime_directory_secure is also a repair path. It continues on ERROR_ALREADY_EXISTS and re-stamps the existing directory, so a tree restored by hand with icacls is destroyed again on the next daemon or client start. Add a second ACL for containers, built with AddAccessAllowedAceEx and CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE, and use it at the three directory sites. Files and kernel objects are leaves and keep the flagless ACL. The container ACE carries FILE_ALL_ACCESS rather than GENERIC_ALL. Windows splits an inheritable generic-rights ACE into an effective mapped ACE plus an INHERIT_ONLY one holding the generic bits, and the owner-only DACL validators require exactly one ACE, so the generic form fails every lock directory with "secure CLI coordination could not be created (project-locks)". Fixes DeusData#1351 Signed-off-by: PETRO YAKOVYSHYN <petroyakovyshyn@gmail.com>
1 parent 564d32c commit 3e540aa

2 files changed

Lines changed: 146 additions & 3 deletions

File tree

‎src/daemon/ipc.c‎

Lines changed: 43 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3312,6 +3312,7 @@ typedef BOOL(WINAPI *is_well_known_sid_fn)(PSID, WELL_KNOWN_SID_TYPE);
33123312
typedef BOOL(WINAPI *is_valid_acl_fn)(PACL);
33133313
typedef BOOL(WINAPI *initialize_acl_fn)(PACL, DWORD, DWORD);
33143314
typedef BOOL(WINAPI *add_access_allowed_ace_fn)(PACL, DWORD, DWORD, PSID);
3315+
typedef BOOL(WINAPI *add_access_allowed_ace_ex_fn)(PACL, DWORD, DWORD, DWORD, PSID);
33153316
typedef BOOL(WINAPI *initialize_security_descriptor_fn)(PSECURITY_DESCRIPTOR, DWORD);
33163317
typedef BOOL(WINAPI *set_security_descriptor_dacl_fn)(PSECURITY_DESCRIPTOR, BOOL, PACL, BOOL);
33173318
typedef BOOL(WINAPI *set_security_descriptor_owner_fn)(PSECURITY_DESCRIPTOR, PSID, BOOL);
@@ -3339,6 +3340,7 @@ typedef struct {
33393340
is_valid_acl_fn is_valid_acl;
33403341
initialize_acl_fn initialize_acl;
33413342
add_access_allowed_ace_fn add_access_allowed_ace;
3343+
add_access_allowed_ace_ex_fn add_access_allowed_ace_ex;
33423344
initialize_security_descriptor_fn initialize_security_descriptor;
33433345
set_security_descriptor_dacl_fn set_security_descriptor_dacl;
33443346
set_security_descriptor_owner_fn set_security_descriptor_owner;
@@ -3352,6 +3354,9 @@ typedef struct {
33523354
PACL acl;
33533355
PSECURITY_DESCRIPTOR descriptor;
33543356
SECURITY_ATTRIBUTES attributes;
3357+
PACL directory_acl;
3358+
PSECURITY_DESCRIPTOR directory_descriptor;
3359+
SECURITY_ATTRIBUTES directory_attributes;
33553360
} win_security_t;
33563361

33573362
typedef struct win_generation_address {
@@ -3560,6 +3565,8 @@ static void win_security_destroy(win_security_t *security) {
35603565
}
35613566
free(security->descriptor);
35623567
free(security->acl);
3568+
free(security->directory_descriptor);
3569+
free(security->directory_acl);
35633570
free(security->user_sid);
35643571
if (security->advapi) {
35653572
(void)FreeLibrary(security->advapi);
@@ -3614,6 +3621,8 @@ static bool win_security_init(win_security_t *security) {
36143621
RESOLVE_ADVAPI_MEMBER(security, initialize_acl, initialize_acl_fn, "InitializeAcl");
36153622
RESOLVE_ADVAPI_MEMBER(security, add_access_allowed_ace, add_access_allowed_ace_fn,
36163623
"AddAccessAllowedAce");
3624+
RESOLVE_ADVAPI_MEMBER(security, add_access_allowed_ace_ex, add_access_allowed_ace_ex_fn,
3625+
"AddAccessAllowedAceEx");
36173626
RESOLVE_ADVAPI_MEMBER(security, initialize_security_descriptor,
36183627
initialize_security_descriptor_fn, "InitializeSecurityDescriptor");
36193628
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) {
36793688
security->attributes.nLength = sizeof(security->attributes);
36803689
security->attributes.lpSecurityDescriptor = security->descriptor;
36813690
security->attributes.bInheritHandle = FALSE;
3691+
3692+
/* Containers need a second, inheritable ACL. AddAccessAllowedAce above
3693+
* cannot express inheritance flags, and a flagless ACE applied to a
3694+
* directory together with PROTECTED_DACL_SECURITY_INFORMATION yields
3695+
* D:PAI(A;;FA;;;<user>): the protection severs the inherited ACEs while
3696+
* the new one propagates nothing, so every child is created with an empty
3697+
* DACL. Files and kernel objects are leaves and keep the flagless ACL.
3698+
*
3699+
* The rights here must be specific (FILE_ALL_ACCESS) rather than
3700+
* GENERIC_ALL. Windows splits an inheritable generic-rights ACE into an
3701+
* effective mapped ACE plus an INHERIT_ONLY one carrying the generic bits,
3702+
* and the owner-only DACL validators require exactly one ACE. */
3703+
security->directory_acl = malloc(acl_size);
3704+
security->directory_descriptor = malloc(SECURITY_DESCRIPTOR_MIN_LENGTH);
3705+
if (!security->directory_acl || !security->directory_descriptor ||
3706+
!security->initialize_acl(security->directory_acl, (DWORD)acl_size, ACL_REVISION) ||
3707+
!security->add_access_allowed_ace_ex(security->directory_acl, ACL_REVISION,
3708+
CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE,
3709+
FILE_ALL_ACCESS, security->user_sid) ||
3710+
!security->initialize_security_descriptor(security->directory_descriptor,
3711+
SECURITY_DESCRIPTOR_REVISION) ||
3712+
!security->set_security_descriptor_dacl(security->directory_descriptor, TRUE,
3713+
security->directory_acl, FALSE) ||
3714+
!security->set_security_descriptor_owner(security->directory_descriptor, security->user_sid,
3715+
FALSE)) {
3716+
win_security_destroy(security);
3717+
return false;
3718+
}
3719+
security->directory_attributes.nLength = sizeof(security->directory_attributes);
3720+
security->directory_attributes.lpSecurityDescriptor = security->directory_descriptor;
3721+
security->directory_attributes.bInheritHandle = FALSE;
36823722
return true;
36833723
}
36843724

@@ -4047,7 +4087,7 @@ static bool win_runtime_directory_secure(const wchar_t *runtime_dir) {
40474087
if (!win_security_init(&security)) {
40484088
return false;
40494089
}
4050-
bool created = CreateDirectoryW(runtime_dir, &security.attributes) != 0;
4090+
bool created = CreateDirectoryW(runtime_dir, &security.directory_attributes) != 0;
40514091
if (!created && GetLastError() != ERROR_ALREADY_EXISTS) {
40524092
win_security_destroy(&security);
40534093
return false;
@@ -4094,7 +4134,7 @@ static bool win_runtime_directory_secure(const wchar_t *runtime_dir) {
40944134
directory, SE_FILE_OBJECT,
40954135
(owner_exact ? 0U : (DWORD)OWNER_SECURITY_INFORMATION) | DACL_SECURITY_INFORMATION |
40964136
PROTECTED_DACL_SECURITY_INFORMATION,
4097-
owner_exact ? NULL : security.user_sid, NULL, security.acl, NULL);
4137+
owner_exact ? NULL : security.user_sid, NULL, security.directory_acl, NULL);
40984138
}
40994139
if (valid_handle && owner_ok && secure_result != ERROR_SUCCESS) {
41004140
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) {
41814221
if (attributes == INVALID_FILE_ATTRIBUTES) {
41824222
DWORD error = GetLastError();
41834223
ok = (error == ERROR_FILE_NOT_FOUND || error == ERROR_PATH_NOT_FOUND) &&
4184-
CreateDirectoryW(path, &security.attributes) != 0;
4224+
CreateDirectoryW(path, &security.directory_attributes) != 0;
41854225
}
41864226
/* Ancestors are observe-only and must already be secure. The
41874227
* final current-user directory is intentionally handled below by

‎tests/test_daemon_ipc.c‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -776,6 +776,108 @@ TEST(daemon_ipc_windows_private_directory_rejects_untrusted_ancestor_acl) {
776776
PASS();
777777
}
778778

779+
/* A private directory is a container, so its owner-only ACE must propagate.
780+
* Otherwise the protected DACL severs inheritance and every child is created
781+
* with an empty DACL, unreadable even by its owner. */
782+
TEST(daemon_ipc_windows_private_directory_ace_is_inheritable) {
783+
char parent[TEST_PATH_CAP] = {0};
784+
char cache[TEST_PATH_CAP] = {0};
785+
bool paths_ok = false;
786+
bool secured = false;
787+
bool ace_read = false;
788+
bool ace_single = false;
789+
bool ace_inheritable = false;
790+
bool child_has_dacl = false;
791+
bool lock_directory_accepted = false;
792+
793+
if (ipc_test_parent_new(parent, "win-inheritable-ace")) {
794+
int written = snprintf(cache, sizeof(cache), "%s/cache", parent);
795+
paths_ok = written > 0 && written < (int)sizeof(cache);
796+
}
797+
if (paths_ok) {
798+
secured = cbm_daemon_ipc_private_directory_secure(cache);
799+
}
800+
if (secured) {
801+
PACL dacl = NULL;
802+
PSECURITY_DESCRIPTOR descriptor = NULL;
803+
LPVOID opaque_ace = NULL;
804+
ACL_SIZE_INFORMATION information;
805+
memset(&information, 0, sizeof(information));
806+
if (GetNamedSecurityInfoA(cache, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL,
807+
&dacl, NULL, &descriptor) == ERROR_SUCCESS &&
808+
dacl &&
809+
GetAclInformation(dacl, &information, sizeof(information), AclSizeInformation) &&
810+
GetAce(dacl, 0, &opaque_ace) && opaque_ace) {
811+
ACCESS_ALLOWED_ACE *ace = (ACCESS_ALLOWED_ACE *)opaque_ace;
812+
ace_read = true;
813+
/* The owner-only validators require exactly one ACE. An
814+
* inheritable ACE built from GENERIC_ALL rather than specific
815+
* rights is split by Windows into an effective ACE plus an
816+
* INHERIT_ONLY one, which fails that check. */
817+
ace_single = information.AceCount == 1;
818+
ace_inheritable = (ace->Header.AceFlags & CONTAINER_INHERIT_ACE) != 0 &&
819+
(ace->Header.AceFlags & OBJECT_INHERIT_ACE) != 0;
820+
}
821+
if (descriptor) {
822+
(void)LocalFree(descriptor);
823+
}
824+
}
825+
if (ace_inheritable) {
826+
char child[TEST_PATH_CAP] = {0};
827+
int written = snprintf(child, sizeof(child), "%s/child", cache);
828+
if (written > 0 && written < (int)sizeof(child) && CreateDirectoryA(child, NULL) != 0) {
829+
PACL child_dacl = NULL;
830+
PSECURITY_DESCRIPTOR child_descriptor = NULL;
831+
ACL_SIZE_INFORMATION information;
832+
memset(&information, 0, sizeof(information));
833+
child_has_dacl =
834+
GetNamedSecurityInfoA(child, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL,
835+
&child_dacl, NULL, &child_descriptor) == ERROR_SUCCESS &&
836+
child_dacl &&
837+
GetAclInformation(child_dacl, &information, sizeof(information),
838+
AclSizeInformation) &&
839+
information.AceCount > 0;
840+
if (child_descriptor) {
841+
(void)LocalFree(child_descriptor);
842+
}
843+
(void)RemoveDirectoryA(child);
844+
}
845+
}
846+
847+
if (ace_single) {
848+
/* The owner-only DACL validator gates every project lock: a secured
849+
* directory it refuses takes the whole coordination layer down with
850+
* "secure CLI coordination could not be created (project-locks)". */
851+
HANDLE handle =
852+
CreateFileA(cache, FILE_READ_ATTRIBUTES | READ_CONTROL,
853+
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL, OPEN_EXISTING,
854+
FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT, NULL);
855+
if (handle != INVALID_HANDLE_VALUE) {
856+
cbm_private_lock_directory_t *directory = NULL;
857+
lock_directory_accepted = cbm_private_lock_directory_adopt_windows(
858+
handle, cache, &directory) == CBM_PRIVATE_FILE_LOCK_OK &&
859+
directory;
860+
if (directory) {
861+
cbm_private_lock_directory_close(directory);
862+
} else {
863+
(void)CloseHandle(handle);
864+
}
865+
}
866+
}
867+
868+
(void)RemoveDirectoryA(cache);
869+
ipc_test_remove_flat_dir(parent);
870+
871+
ASSERT_TRUE(paths_ok);
872+
ASSERT_TRUE(secured);
873+
ASSERT_TRUE(ace_read);
874+
ASSERT_TRUE(ace_single);
875+
ASSERT_TRUE(ace_inheritable);
876+
ASSERT_TRUE(child_has_dacl);
877+
ASSERT_TRUE(lock_directory_accepted);
878+
PASS();
879+
}
880+
779881
TEST(daemon_ipc_windows_private_directory_allows_add_subdirectory_only_ancestor) {
780882
char parent[TEST_PATH_CAP] = {0};
781883
char add_only[TEST_PATH_CAP] = {0};
@@ -4620,6 +4722,7 @@ SUITE(daemon_ipc) {
46204722
#ifdef _WIN32
46214723
RUN_TEST(daemon_ipc_windows_default_endpoint_ignores_temp_environment);
46224724
RUN_TEST(daemon_ipc_windows_private_directory_rejects_untrusted_ancestor_acl);
4725+
RUN_TEST(daemon_ipc_windows_private_directory_ace_is_inheritable);
46234726
RUN_TEST(daemon_ipc_windows_private_directory_allows_add_subdirectory_only_ancestor);
46244727
RUN_TEST(daemon_ipc_windows_legacy_bridge_covers_handoff_and_lifetime);
46254728
RUN_TEST(daemon_ipc_windows_local_transition_atomically_reserves_legacy_pipe);

0 commit comments

Comments
 (0)