diff --git a/src/daemon/ipc.c b/src/daemon/ipc.c index 141882cc9..5fdb7efe1 100644 --- a/src/daemon/ipc.c +++ b/src/daemon/ipc.c @@ -3987,8 +3987,45 @@ static bool win_sid_trusted(win_security_t *security, PSID sid) { win_sid_is_trusted_installer((const uint8_t *)sid, (size_t)sid_length); } +/* AppContainer identities: package SIDs (S-1-15-2-*) and capability SIDs + * (S-1-15-3-*), under the APP_PACKAGE identifier authority (15). + * + * These are tolerated on ANCESTOR components only — never on the private + * runtime directory itself, which keeps demanding the exact current user. + * + * Why they are admissible there: a sandboxed package's ACE on %LOCALAPPDATA% + * grants that package, and a process cannot select which AppContainer it runs + * in — the identity is stamped by the OS at process creation from the package + * it was launched from. So such an ACE cannot be exercised by arbitrary local + * code the way a live local group can. What it does permit is the packaged + * application itself; that is the residual risk this exemption accepts, and it + * is the same trust already extended to whatever installed that package. + * + * Why BOTH forms: capability SIDs alone are not enough. The most common real + * ACE of this shape is `S-1-15-2-*` — a package SID. On reported machines it + * resolves through HKCR\...\AppContainer\Mappings to Anthropic Claude + * Desktop, an application many of our users run and cannot be asked to + * uninstall. Covering only S-1-15-3-* leaves exactly that case failing. + * + * Grounded in #1533 (four independent reproductions across four SID classes) + * and #1574. Approach and the ancestor-only boundary follow @mlandolfi90's + * PR #1447, extended to package SIDs. */ +static bool win_sid_is_app_container(const uint8_t *sid, size_t sid_length) { + if (!windows_sid_valid(sid, sid_length) || sid[1] < 1U) { + return false; + } + /* identifier authority must be exactly 15 (APP_PACKAGE_AUTHORITY) */ + if (sid[2] != 0U || sid[3] != 0U || sid[4] != 0U || sid[5] != 0U || sid[6] != 0U || + sid[7] != 15U) { + return false; + } + uint32_t first = win_sid_read_u32_le(sid + 8U); + return first == 2U || first == 3U; +} + static bool win_bounded_sid_trusted(win_security_t *security, const uint8_t *sid, - size_t sid_capacity, bool creator_owner_inherit_only) { + size_t sid_capacity, bool creator_owner_inherit_only, + bool ancestor) { if (!security || !sid || sid_capacity < 8U || sid[1] > 15U) { return false; } @@ -4004,7 +4041,8 @@ static bool win_bounded_sid_trusted(win_security_t *security, const uint8_t *sid * user, so such an ACE only ever grants to us. Default Windows * profile/temp ACLs (and GitHub runner profiles) carry it, and * rejecting it locked real current-user directories out. */ - security->is_well_known_sid((PSID)sid, WinCreatorOwnerRightsSid)); + security->is_well_known_sid((PSID)sid, WinCreatorOwnerRightsSid) || + (ancestor && win_sid_is_app_container(sid, sid_length))); } static bool win_file_owner_secure(win_security_t *security, HANDLE file, @@ -4039,7 +4077,8 @@ static DWORD win_private_mutation_rights(void) { DELETE | WRITE_DAC | WRITE_OWNER | ACCESS_SYSTEM_SECURITY; } -static bool win_file_acl_secure(win_security_t *security, HANDLE file, DWORD mutation) { +static bool win_file_acl_secure(win_security_t *security, HANDLE file, DWORD mutation, + bool ancestor) { PACL dacl = NULL; PSECURITY_DESCRIPTOR descriptor = NULL; DWORD status = security->get_security_info(file, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, @@ -4080,7 +4119,8 @@ static bool win_file_acl_secure(win_security_t *security, HANDLE file, DWORD mut const uint8_t *sid = (const uint8_t *)&ace->SidStart; size_t sid_capacity = (size_t)header->AceSize - sid_offset; bool creator_owner_inherit_only = (header->AceFlags & INHERIT_ONLY_ACE) != 0U; - if (!win_bounded_sid_trusted(security, sid, sid_capacity, creator_owner_inherit_only)) { + if (!win_bounded_sid_trusted(security, sid, sid_capacity, creator_owner_inherit_only, + ancestor)) { /* Name the untrusted identity class so a harness/profile ACL leak * (an inherited Users / Authenticated Users / Everyone ACE) is * distinguishable from a genuinely hostile grant. */ @@ -4113,9 +4153,9 @@ static bool win_file_acl_secure(win_security_t *security, HANDLE file, DWORD mut } static bool win_file_security_secure(win_security_t *security, HANDLE file, - bool require_current_user, DWORD mutation) { + bool require_current_user, DWORD mutation, bool ancestor) { return win_file_owner_secure(security, file, require_current_user) && - win_file_acl_secure(security, file, mutation); + win_file_acl_secure(security, file, mutation, ancestor); } static bool win_runtime_directory_secure(const wchar_t *runtime_dir) { @@ -4179,7 +4219,7 @@ 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()); + win_file_security_secure(&security, directory, true, win_private_mutation_rights(), false); (void)CloseHandle(directory); win_security_destroy(&security); return valid_handle && owner_ok && final_private; @@ -4202,7 +4242,7 @@ static bool win_directory_component_secure(win_security_t *security, const wchar bool valid = GetFileInformationByHandle(directory, &info) != 0 && (info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0 && (info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) == 0 && - win_file_security_secure(security, directory, false, mutation); + win_file_security_secure(security, directory, false, mutation, true); (void)CloseHandle(directory); return valid; } diff --git a/tests/test_windows_bundle_contract.sh b/tests/test_windows_bundle_contract.sh index d41a9ba68..fab5023d5 100644 --- a/tests/test_windows_bundle_contract.sh +++ b/tests/test_windows_bundle_contract.sh @@ -415,7 +415,7 @@ require( needle in read("src/daemon/ipc.c") for needle in ( "win_directory_component_secure", - "win_file_security_secure(security, directory, false, mutation)", + "win_file_security_secure(security, directory, false, mutation, true)", "win_private_mutation_rights()", "~((DWORD)FILE_ADD_SUBDIRECTORY)", "FILE_ADD_FILE", @@ -429,6 +429,30 @@ require( "src/daemon/ipc.c must enforce the shared cross-account ancestor trust policy", ) +# ── 6b. AppContainer tolerance is ANCESTOR-ONLY ───────────────────────────── +# Package (S-1-15-2-*) and capability (S-1-15-3-*) SIDs are admitted on ancestor +# components so a machine running a sandboxed desktop app (Claude Desktop stamps +# its package SID on %LOCALAPPDATA%) is not locked out. The private runtime +# directory must keep demanding the exact current user: it is passed +# ancestor=false and that is the whole boundary. +require( + all( + needle in read("src/daemon/ipc.c") + for needle in ( + "win_sid_is_app_container", + "ancestor && win_sid_is_app_container", + # both AppContainer forms, not capability alone + "first == 2U || first == 3U", + # APP_PACKAGE identifier authority + "sid[7] != 15U", + # the private runtime directory is validated with ancestor=false + "win_private_mutation_rights(), false)", + ) + ), + "AppContainer SIDs must be tolerated on ancestors ONLY, covering both package " + "and capability forms, with the private runtime directory still strict", +) + # On Windows subprocess supervision receives a non-NULL lpApplicationName, so a # literal `git` would not use PATH. Resolve only git.exe beneath inherited # absolute PATH entries and never permit the current-directory search implied by