Skip to content

Commit c1a5de3

Browse files
authored
Merge pull request DeusData#1685 from DeusData/fix/windows-acl-repair-v3
fix(windows): conditional DACL re-stamp + damaged-children repair, gated on the adoption-level owner-only predicate
2 parents 93e9308 + 47bd4b6 commit c1a5de3

1 file changed

Lines changed: 215 additions & 1 deletion

File tree

‎src/daemon/ipc.c‎

Lines changed: 215 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3365,6 +3365,8 @@ typedef BOOL(WINAPI *initialize_security_descriptor_fn)(PSECURITY_DESCRIPTOR, DW
33653365
typedef BOOL(WINAPI *set_security_descriptor_dacl_fn)(PSECURITY_DESCRIPTOR, BOOL, PACL, BOOL);
33663366
typedef BOOL(WINAPI *set_security_descriptor_owner_fn)(PSECURITY_DESCRIPTOR, PSID, BOOL);
33673367
typedef BOOL(WINAPI *get_acl_information_fn)(PACL, LPVOID, DWORD, ACL_INFORMATION_CLASS);
3368+
typedef BOOL(WINAPI *get_security_descriptor_control_fn)(PSECURITY_DESCRIPTOR,
3369+
PSECURITY_DESCRIPTOR_CONTROL, LPDWORD);
33683370
typedef BOOL(WINAPI *get_ace_fn)(PACL, DWORD, LPVOID *);
33693371
typedef DWORD(WINAPI *get_security_info_fn)(HANDLE, SE_OBJECT_TYPE, SECURITY_INFORMATION, PSID *,
33703372
PSID *, PACL *, PACL *, PSECURITY_DESCRIPTOR *);
@@ -3393,6 +3395,7 @@ typedef struct {
33933395
set_security_descriptor_dacl_fn set_security_descriptor_dacl;
33943396
set_security_descriptor_owner_fn set_security_descriptor_owner;
33953397
get_acl_information_fn get_acl_information;
3398+
get_security_descriptor_control_fn get_security_descriptor_control;
33963399
get_ace_fn get_ace;
33973400
get_security_info_fn get_security_info;
33983401
set_security_info_fn set_security_info;
@@ -3679,6 +3682,8 @@ static bool win_security_init(win_security_t *security) {
36793682
"SetSecurityDescriptorOwner");
36803683
RESOLVE_ADVAPI_MEMBER(security, get_acl_information, get_acl_information_fn,
36813684
"GetAclInformation");
3685+
RESOLVE_ADVAPI_MEMBER(security, get_security_descriptor_control,
3686+
get_security_descriptor_control_fn, "GetSecurityDescriptorControl");
36823687
RESOLVE_ADVAPI_MEMBER(security, get_ace, get_ace_fn, "GetAce");
36833688
RESOLVE_ADVAPI_MEMBER(security, get_security_info, get_security_info_fn, "GetSecurityInfo");
36843689
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,
41704175
win_file_acl_secure(security, file, mutation, ancestor);
41714176
}
41724177

4178+
/* Is this object's DACL present but EMPTY (zero ACEs)? That denies everyone,
4179+
* including the owner, for anything the owner-rights path does not cover.
4180+
*
4181+
* It needs its own test because win_file_acl_secure() cannot detect it: that
4182+
* function scans ACEs for untrusted mutation grants, and a DACL with zero ACEs
4183+
* trivially has none, so damage reads as compliance. */
4184+
/* The lock-directory ADOPTION predicate (private_win_owner_only_dacl in the
4185+
* foundation layer) demands a PROTECTED DACL whose single non-inherited ACE
4186+
* grants the CURRENT USER full access — strictly narrower than "no untrusted
4187+
* mutation rights", which also admits SYSTEM/Administrators ACEs. The
4188+
* already-correct fast path below must apply the CONSUMER'S predicate: a
4189+
* directory that merely passes the general secure() check but is not
4190+
* owner-only would skip the re-stamp and then strand every subsequent lock
4191+
* adoption (observed as 59 daemon-suite failures on a fresh runtime dir whose
4192+
* inherited DACL carried SYSTEM+Administrators). */
4193+
static bool win_file_dacl_is_owner_only(win_security_t *security, HANDLE file) {
4194+
if (!security->get_security_descriptor_control) {
4195+
return false;
4196+
}
4197+
PSID owner = NULL;
4198+
PACL dacl = NULL;
4199+
PSECURITY_DESCRIPTOR descriptor = NULL;
4200+
if (security->get_security_info(file, SE_FILE_OBJECT,
4201+
OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, &owner,
4202+
NULL, &dacl, NULL, &descriptor) != ERROR_SUCCESS) {
4203+
return false;
4204+
}
4205+
SECURITY_DESCRIPTOR_CONTROL control = 0;
4206+
DWORD revision = 0;
4207+
ACL_SIZE_INFORMATION information;
4208+
memset(&information, 0, sizeof(information));
4209+
LPVOID opaque_ace = NULL;
4210+
bool valid = descriptor && owner && dacl && security->is_valid_sid(owner) &&
4211+
security->equal_sid(owner, security->user_sid) &&
4212+
security->get_security_descriptor_control(descriptor, &control, &revision) &&
4213+
(control & SE_DACL_PRESENT) != 0 && (control & SE_DACL_PROTECTED) != 0 &&
4214+
security->is_valid_acl(dacl) &&
4215+
security->get_acl_information(dacl, &information, sizeof(information),
4216+
AclSizeInformation) &&
4217+
information.AceCount == 1U && security->get_ace(dacl, 0, &opaque_ace) &&
4218+
opaque_ace;
4219+
if (valid) {
4220+
ACCESS_ALLOWED_ACE *ace = (ACCESS_ALLOWED_ACE *)opaque_ace;
4221+
PSID ace_sid = (PSID)&ace->SidStart;
4222+
valid = ace->Header.AceType == ACCESS_ALLOWED_ACE_TYPE &&
4223+
ace->Header.AceSize >= sizeof(ACCESS_ALLOWED_ACE) &&
4224+
(ace->Header.AceFlags & (INHERITED_ACE | INHERIT_ONLY_ACE)) == 0 &&
4225+
security->is_valid_sid(ace_sid) &&
4226+
security->equal_sid(ace_sid, security->user_sid) &&
4227+
(ace->Mask == FILE_ALL_ACCESS || ace->Mask == GENERIC_ALL);
4228+
}
4229+
if (descriptor) {
4230+
(void)LocalFree(descriptor);
4231+
}
4232+
return valid;
4233+
}
4234+
4235+
static bool win_file_dacl_is_empty(win_security_t *security, HANDLE file) {
4236+
PACL dacl = NULL;
4237+
PSECURITY_DESCRIPTOR descriptor = NULL;
4238+
if (security->get_security_info(file, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL,
4239+
&dacl, NULL, &descriptor) != ERROR_SUCCESS) {
4240+
return false;
4241+
}
4242+
ACL_SIZE_INFORMATION information;
4243+
memset(&information, 0, sizeof(information));
4244+
bool empty = dacl && security->is_valid_acl(dacl) &&
4245+
security->get_acl_information(dacl, &information, sizeof(information),
4246+
AclSizeInformation) &&
4247+
information.AceCount == 0U;
4248+
if (descriptor) {
4249+
(void)LocalFree(descriptor);
4250+
}
4251+
return empty;
4252+
}
4253+
4254+
/* Repair cache/runtime children left unusable by the pre-v0.10.3 DACL regime.
4255+
*
4256+
* Between v0.9.1-rc and v0.10.2 the runtime directory carried a PROTECTED DACL
4257+
* whose ACE was not inheritable. Windows therefore gave every file created
4258+
* inside it either an empty DACL or the token default (SYSTEM + TokenOwner +
4259+
* logon SID). Under an elevated token TokenOwner is BUILTIN\Administrators, so
4260+
* the interactive user ends up with no durable grant at all and the file is
4261+
* unreadable after the next logon — #1601, where takeown and icacls both fail
4262+
* non-elevated and the daemon can no longer open _config.db.
4263+
*
4264+
* #1531 fixed the cause forward-only in v0.10.3: the directory ACE is
4265+
* inheritable now, so newly created children are fine. Nothing repaired the
4266+
* children already damaged, which is why upgrading did not rescue anyone whose
4267+
* cache was written under the old regime. This is that repair.
4268+
*
4269+
* Deliberately bounded and conservative:
4270+
* - immediate children only, no recursion, capped;
4271+
* - regular files only; directories, reparse points and symlinks are skipped
4272+
* entirely rather than followed;
4273+
* - a child is touched ONLY when it is demonstrably damaged - an empty DACL,
4274+
* or an owner that is not the current user. A child that is merely unusual
4275+
* is left alone;
4276+
* - failures are counted and reported, never fatal. This runs inside daemon
4277+
* startup and must not be able to prevent it.
4278+
*
4279+
* Scope note: this only ever runs on cbm's own runtime/cache directory, which
4280+
* we created and own. It does not reach into user directories. */
4281+
static void win_repair_runtime_children(win_security_t *security, const wchar_t *runtime_dir) {
4282+
enum { WIN_CHILD_REPAIR_MAX = 4096 };
4283+
if (!security || !runtime_dir || !security->user_sid) {
4284+
return;
4285+
}
4286+
size_t dir_length = wcslen(runtime_dir);
4287+
if (dir_length == 0U || dir_length > 32000U) {
4288+
return;
4289+
}
4290+
wchar_t *pattern = calloc(dir_length + 3U, sizeof(wchar_t));
4291+
if (!pattern) {
4292+
return;
4293+
}
4294+
(void)swprintf(pattern, dir_length + 3U, L"%ls\\*", runtime_dir);
4295+
WIN32_FIND_DATAW entry;
4296+
HANDLE search = FindFirstFileW(pattern, &entry);
4297+
free(pattern);
4298+
if (search == INVALID_HANDLE_VALUE) {
4299+
return;
4300+
}
4301+
unsigned examined = 0U;
4302+
unsigned repaired = 0U;
4303+
unsigned failed = 0U;
4304+
do {
4305+
if (wcscmp(entry.cFileName, L".") == 0 || wcscmp(entry.cFileName, L"..") == 0) {
4306+
continue;
4307+
}
4308+
if ((entry.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0U ||
4309+
(entry.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0U) {
4310+
continue;
4311+
}
4312+
if (++examined > (unsigned)WIN_CHILD_REPAIR_MAX) {
4313+
break;
4314+
}
4315+
size_t name_length = wcslen(entry.cFileName);
4316+
size_t child_capacity = dir_length + name_length + 2U;
4317+
wchar_t *child_path = calloc(child_capacity, sizeof(wchar_t));
4318+
if (!child_path) {
4319+
continue;
4320+
}
4321+
(void)swprintf(child_path, child_capacity, L"%ls\\%ls", runtime_dir, entry.cFileName);
4322+
HANDLE child = CreateFileW(child_path, READ_CONTROL | WRITE_DAC | WRITE_OWNER,
4323+
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,
4324+
OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, NULL);
4325+
free(child_path);
4326+
if (child == INVALID_HANDLE_VALUE) {
4327+
continue;
4328+
}
4329+
BY_HANDLE_FILE_INFORMATION child_info;
4330+
bool regular = GetFileInformationByHandle(child, &child_info) != 0 &&
4331+
(child_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) == 0U &&
4332+
(child_info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) == 0U;
4333+
bool damaged = regular && (win_file_dacl_is_empty(security, child) ||
4334+
!win_file_owner_secure(security, child, true));
4335+
if (damaged) {
4336+
if (security->set_security_info(
4337+
child, SE_FILE_OBJECT,
4338+
(DWORD)OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION |
4339+
PROTECTED_DACL_SECURITY_INFORMATION,
4340+
security->user_sid, NULL, security->acl, NULL) == ERROR_SUCCESS) {
4341+
repaired++;
4342+
} else {
4343+
failed++;
4344+
}
4345+
}
4346+
(void)CloseHandle(child);
4347+
} while (FindNextFileW(search, &entry) != 0);
4348+
(void)FindClose(search);
4349+
if (repaired > 0U || failed > 0U) {
4350+
char repaired_text[16];
4351+
char failed_text[16];
4352+
(void)snprintf(repaired_text, sizeof(repaired_text), "%u", repaired);
4353+
(void)snprintf(failed_text, sizeof(failed_text), "%u", failed);
4354+
cbm_log_warn("daemon.runtime_child_acl_repaired", "repaired", repaired_text, "failed",
4355+
failed_text);
4356+
}
4357+
}
4358+
41734359
static bool win_runtime_directory_secure(const wchar_t *runtime_dir) {
41744360
win_security_t security;
41754361
if (!win_security_init(&security)) {
@@ -4216,8 +4402,30 @@ static bool win_runtime_directory_secure(const wchar_t *runtime_dir) {
42164402
* refused, and the final validation below still demands the exact user. */
42174403
bool owner_ok = owner_exact || (valid_handle && can_write_owner &&
42184404
win_file_owner_secure(&security, directory, false));
4405+
/* Re-stamp only when the directory is not ALREADY correct.
4406+
*
4407+
* This used to fire on every process start, whether or not anything was
4408+
* wrong. Two costs, both observed in the field:
4409+
*
4410+
* - It rewrites the security descriptor of a directory that already has
4411+
* the right one, and Windows propagates that to children. #1601 counted
4412+
* ELEVEN "Security change" USN records against a single _config.db in
4413+
* one day, none of which changed anything.
4414+
* - Every rewrite is a window. #1620 loses an atomic publish to exactly
4415+
* this: MoveFileEx needs DELETE on the destination, and a concurrent
4416+
* re-protect of the parent is a chance to be refused for a state that is
4417+
* about to be correct again anyway.
4418+
*
4419+
* The repair is what matters, not the ritual. If the owner is already the
4420+
* exact current user and the DACL already passes the private-directory
4421+
* check, there is nothing to fix and the correct action is to leave it
4422+
* alone. When it IS wrong we still repair exactly as before. */
42194423
DWORD secure_result = ERROR_ACCESS_DENIED;
4220-
if (valid_handle && owner_ok) {
4424+
bool already_correct =
4425+
valid_handle && owner_exact && win_file_dacl_is_owner_only(&security, directory);
4426+
if (already_correct) {
4427+
secure_result = ERROR_SUCCESS;
4428+
} else if (valid_handle && owner_ok) {
42214429
secure_result = security.set_security_info(
42224430
directory, SE_FILE_OBJECT,
42234431
(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) {
42324440
bool final_private =
42334441
secure_result == ERROR_SUCCESS &&
42344442
win_file_security_secure(&security, directory, true, win_private_mutation_rights(), false);
4443+
/* Repair damaged children only once the directory itself is known good.
4444+
* Repairing into a parent we have not secured would re-derive the same
4445+
* broken state on the next file created there. */
4446+
if (final_private) {
4447+
win_repair_runtime_children(&security, runtime_dir);
4448+
}
42354449
(void)CloseHandle(directory);
42364450
win_security_destroy(&security);
42374451
return valid_handle && owner_ok && final_private;

0 commit comments

Comments
 (0)