From 47bd4b68471f58b0ec4ae69a80fe46d29c326b65 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Mon, 17 Aug 2026 06:12:11 +0200 Subject: [PATCH] fix(windows): conditional DACL re-stamp + damaged-children repair MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-stamp the runtime DACL only when it is actually wrong, and repair cache children left unusable by the pre-v0.10.3 DACL regime. The unconditional per-start re-stamp rewrote an already-correct security descriptor and propagated it to children (#1601 counted eleven no-op "Security change" USN records against one _config.db in a day), and every rewrite is a window in which a concurrent atomic publish can be refused DELETE on the destination (#1620). The old regime's PROTECTED, non-inheritable ACE also left every child born unusable — the 0-byte worker-log class behind #1416's diagnosis — so the secured directory now walks its regular children and repairs any with an empty DACL or a foreign owner. The fast path is gated on the ADOPTION-level predicate, not the general secure() check: lock-directory adoption (private_win_owner_only_dacl) demands the exact protected owner-only single-ACE descriptor the stamp writes, while secure() also admits SYSTEM/Administrators ACEs. A fresh directory with an inherited DACL passed secure(), skipped the stamp, and stranded every subsequent lock adoption — 59/77 daemon-suite failures on the real Windows VM. With the ported predicate (SE_DACL_PROTECTED, single non-inherited owner ACE, FILE_ALL_ACCESS/GENERIC_ALL) the same VM runs 77/77 and the full suite 7346/0. Signed-off-by: Martin Vogel --- src/daemon/ipc.c | 216 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 215 insertions(+), 1 deletion(-) diff --git a/src/daemon/ipc.c b/src/daemon/ipc.c index 4ef1a238a..e62189c98 100644 --- a/src/daemon/ipc.c +++ b/src/daemon/ipc.c @@ -3365,6 +3365,8 @@ typedef BOOL(WINAPI *initialize_security_descriptor_fn)(PSECURITY_DESCRIPTOR, DW 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); typedef BOOL(WINAPI *get_acl_information_fn)(PACL, LPVOID, DWORD, ACL_INFORMATION_CLASS); +typedef BOOL(WINAPI *get_security_descriptor_control_fn)(PSECURITY_DESCRIPTOR, + PSECURITY_DESCRIPTOR_CONTROL, LPDWORD); typedef BOOL(WINAPI *get_ace_fn)(PACL, DWORD, LPVOID *); typedef DWORD(WINAPI *get_security_info_fn)(HANDLE, SE_OBJECT_TYPE, SECURITY_INFORMATION, PSID *, PSID *, PACL *, PACL *, PSECURITY_DESCRIPTOR *); @@ -3393,6 +3395,7 @@ typedef struct { set_security_descriptor_dacl_fn set_security_descriptor_dacl; set_security_descriptor_owner_fn set_security_descriptor_owner; get_acl_information_fn get_acl_information; + get_security_descriptor_control_fn get_security_descriptor_control; get_ace_fn get_ace; get_security_info_fn get_security_info; set_security_info_fn set_security_info; @@ -3679,6 +3682,8 @@ static bool win_security_init(win_security_t *security) { "SetSecurityDescriptorOwner"); RESOLVE_ADVAPI_MEMBER(security, get_acl_information, get_acl_information_fn, "GetAclInformation"); + RESOLVE_ADVAPI_MEMBER(security, get_security_descriptor_control, + get_security_descriptor_control_fn, "GetSecurityDescriptorControl"); RESOLVE_ADVAPI_MEMBER(security, get_ace, get_ace_fn, "GetAce"); RESOLVE_ADVAPI_MEMBER(security, get_security_info, get_security_info_fn, "GetSecurityInfo"); RESOLVE_ADVAPI_MEMBER(security, set_security_info, set_security_info_fn, "SetSecurityInfo"); @@ -4170,6 +4175,187 @@ static bool win_file_security_secure(win_security_t *security, HANDLE file, win_file_acl_secure(security, file, mutation, ancestor); } +/* Is this object's DACL present but EMPTY (zero ACEs)? That denies everyone, + * including the owner, for anything the owner-rights path does not cover. + * + * It needs its own test because win_file_acl_secure() cannot detect it: that + * function scans ACEs for untrusted mutation grants, and a DACL with zero ACEs + * trivially has none, so damage reads as compliance. */ +/* The lock-directory ADOPTION predicate (private_win_owner_only_dacl in the + * foundation layer) demands a PROTECTED DACL whose single non-inherited ACE + * grants the CURRENT USER full access — strictly narrower than "no untrusted + * mutation rights", which also admits SYSTEM/Administrators ACEs. The + * already-correct fast path below must apply the CONSUMER'S predicate: a + * directory that merely passes the general secure() check but is not + * owner-only would skip the re-stamp and then strand every subsequent lock + * adoption (observed as 59 daemon-suite failures on a fresh runtime dir whose + * inherited DACL carried SYSTEM+Administrators). */ +static bool win_file_dacl_is_owner_only(win_security_t *security, HANDLE file) { + if (!security->get_security_descriptor_control) { + return false; + } + PSID owner = NULL; + PACL dacl = NULL; + PSECURITY_DESCRIPTOR descriptor = NULL; + if (security->get_security_info(file, SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, &owner, + NULL, &dacl, NULL, &descriptor) != ERROR_SUCCESS) { + return false; + } + SECURITY_DESCRIPTOR_CONTROL control = 0; + DWORD revision = 0; + ACL_SIZE_INFORMATION information; + memset(&information, 0, sizeof(information)); + LPVOID opaque_ace = NULL; + bool valid = descriptor && owner && dacl && security->is_valid_sid(owner) && + security->equal_sid(owner, security->user_sid) && + security->get_security_descriptor_control(descriptor, &control, &revision) && + (control & SE_DACL_PRESENT) != 0 && (control & SE_DACL_PROTECTED) != 0 && + security->is_valid_acl(dacl) && + security->get_acl_information(dacl, &information, sizeof(information), + AclSizeInformation) && + information.AceCount == 1U && security->get_ace(dacl, 0, &opaque_ace) && + opaque_ace; + if (valid) { + ACCESS_ALLOWED_ACE *ace = (ACCESS_ALLOWED_ACE *)opaque_ace; + PSID ace_sid = (PSID)&ace->SidStart; + valid = ace->Header.AceType == ACCESS_ALLOWED_ACE_TYPE && + ace->Header.AceSize >= sizeof(ACCESS_ALLOWED_ACE) && + (ace->Header.AceFlags & (INHERITED_ACE | INHERIT_ONLY_ACE)) == 0 && + security->is_valid_sid(ace_sid) && + security->equal_sid(ace_sid, security->user_sid) && + (ace->Mask == FILE_ALL_ACCESS || ace->Mask == GENERIC_ALL); + } + if (descriptor) { + (void)LocalFree(descriptor); + } + return valid; +} + +static bool win_file_dacl_is_empty(win_security_t *security, HANDLE file) { + PACL dacl = NULL; + PSECURITY_DESCRIPTOR descriptor = NULL; + if (security->get_security_info(file, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, + &dacl, NULL, &descriptor) != ERROR_SUCCESS) { + return false; + } + ACL_SIZE_INFORMATION information; + memset(&information, 0, sizeof(information)); + bool empty = dacl && security->is_valid_acl(dacl) && + security->get_acl_information(dacl, &information, sizeof(information), + AclSizeInformation) && + information.AceCount == 0U; + if (descriptor) { + (void)LocalFree(descriptor); + } + return empty; +} + +/* Repair cache/runtime children left unusable by the pre-v0.10.3 DACL regime. + * + * Between v0.9.1-rc and v0.10.2 the runtime directory carried a PROTECTED DACL + * whose ACE was not inheritable. Windows therefore gave every file created + * inside it either an empty DACL or the token default (SYSTEM + TokenOwner + + * logon SID). Under an elevated token TokenOwner is BUILTIN\Administrators, so + * the interactive user ends up with no durable grant at all and the file is + * unreadable after the next logon — #1601, where takeown and icacls both fail + * non-elevated and the daemon can no longer open _config.db. + * + * #1531 fixed the cause forward-only in v0.10.3: the directory ACE is + * inheritable now, so newly created children are fine. Nothing repaired the + * children already damaged, which is why upgrading did not rescue anyone whose + * cache was written under the old regime. This is that repair. + * + * Deliberately bounded and conservative: + * - immediate children only, no recursion, capped; + * - regular files only; directories, reparse points and symlinks are skipped + * entirely rather than followed; + * - a child is touched ONLY when it is demonstrably damaged - an empty DACL, + * or an owner that is not the current user. A child that is merely unusual + * is left alone; + * - failures are counted and reported, never fatal. This runs inside daemon + * startup and must not be able to prevent it. + * + * Scope note: this only ever runs on cbm's own runtime/cache directory, which + * we created and own. It does not reach into user directories. */ +static void win_repair_runtime_children(win_security_t *security, const wchar_t *runtime_dir) { + enum { WIN_CHILD_REPAIR_MAX = 4096 }; + if (!security || !runtime_dir || !security->user_sid) { + return; + } + size_t dir_length = wcslen(runtime_dir); + if (dir_length == 0U || dir_length > 32000U) { + return; + } + wchar_t *pattern = calloc(dir_length + 3U, sizeof(wchar_t)); + if (!pattern) { + return; + } + (void)swprintf(pattern, dir_length + 3U, L"%ls\\*", runtime_dir); + WIN32_FIND_DATAW entry; + HANDLE search = FindFirstFileW(pattern, &entry); + free(pattern); + if (search == INVALID_HANDLE_VALUE) { + return; + } + unsigned examined = 0U; + unsigned repaired = 0U; + unsigned failed = 0U; + do { + if (wcscmp(entry.cFileName, L".") == 0 || wcscmp(entry.cFileName, L"..") == 0) { + continue; + } + if ((entry.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0U || + (entry.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0U) { + continue; + } + if (++examined > (unsigned)WIN_CHILD_REPAIR_MAX) { + break; + } + size_t name_length = wcslen(entry.cFileName); + size_t child_capacity = dir_length + name_length + 2U; + wchar_t *child_path = calloc(child_capacity, sizeof(wchar_t)); + if (!child_path) { + continue; + } + (void)swprintf(child_path, child_capacity, L"%ls\\%ls", runtime_dir, entry.cFileName); + HANDLE child = CreateFileW(child_path, READ_CONTROL | WRITE_DAC | WRITE_OWNER, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL, + OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, NULL); + free(child_path); + if (child == INVALID_HANDLE_VALUE) { + continue; + } + BY_HANDLE_FILE_INFORMATION child_info; + bool regular = GetFileInformationByHandle(child, &child_info) != 0 && + (child_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) == 0U && + (child_info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) == 0U; + bool damaged = regular && (win_file_dacl_is_empty(security, child) || + !win_file_owner_secure(security, child, true)); + if (damaged) { + if (security->set_security_info( + child, SE_FILE_OBJECT, + (DWORD)OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION | + PROTECTED_DACL_SECURITY_INFORMATION, + security->user_sid, NULL, security->acl, NULL) == ERROR_SUCCESS) { + repaired++; + } else { + failed++; + } + } + (void)CloseHandle(child); + } while (FindNextFileW(search, &entry) != 0); + (void)FindClose(search); + if (repaired > 0U || failed > 0U) { + char repaired_text[16]; + char failed_text[16]; + (void)snprintf(repaired_text, sizeof(repaired_text), "%u", repaired); + (void)snprintf(failed_text, sizeof(failed_text), "%u", failed); + cbm_log_warn("daemon.runtime_child_acl_repaired", "repaired", repaired_text, "failed", + failed_text); + } +} + static bool win_runtime_directory_secure(const wchar_t *runtime_dir) { win_security_t security; if (!win_security_init(&security)) { @@ -4216,8 +4402,30 @@ static bool win_runtime_directory_secure(const wchar_t *runtime_dir) { * refused, and the final validation below still demands the exact user. */ bool owner_ok = owner_exact || (valid_handle && can_write_owner && win_file_owner_secure(&security, directory, false)); + /* Re-stamp only when the directory is not ALREADY correct. + * + * This used to fire on every process start, whether or not anything was + * wrong. Two costs, both observed in the field: + * + * - It rewrites the security descriptor of a directory that already has + * the right one, and Windows propagates that to children. #1601 counted + * ELEVEN "Security change" USN records against a single _config.db in + * one day, none of which changed anything. + * - Every rewrite is a window. #1620 loses an atomic publish to exactly + * this: MoveFileEx needs DELETE on the destination, and a concurrent + * re-protect of the parent is a chance to be refused for a state that is + * about to be correct again anyway. + * + * The repair is what matters, not the ritual. If the owner is already the + * exact current user and the DACL already passes the private-directory + * check, there is nothing to fix and the correct action is to leave it + * alone. When it IS wrong we still repair exactly as before. */ DWORD secure_result = ERROR_ACCESS_DENIED; - if (valid_handle && owner_ok) { + bool already_correct = + valid_handle && owner_exact && win_file_dacl_is_owner_only(&security, directory); + if (already_correct) { + secure_result = ERROR_SUCCESS; + } else if (valid_handle && owner_ok) { secure_result = security.set_security_info( directory, SE_FILE_OBJECT, (owner_exact ? 0U : (DWORD)OWNER_SECURITY_INFORMATION) | DACL_SECURITY_INFORMATION | @@ -4232,6 +4440,12 @@ static bool win_runtime_directory_secure(const wchar_t *runtime_dir) { bool final_private = secure_result == ERROR_SUCCESS && win_file_security_secure(&security, directory, true, win_private_mutation_rights(), false); + /* Repair damaged children only once the directory itself is known good. + * Repairing into a parent we have not secured would re-derive the same + * broken state on the next file created there. */ + if (final_private) { + win_repair_runtime_children(&security, runtime_dir); + } (void)CloseHandle(directory); win_security_destroy(&security); return valid_handle && owner_ok && final_private;