fix(security): restrict Windows private files with fail-safe ACL ordering - #164
Conversation
…ring PR HKUDS#148 introduced Windows ACL restriction for private storage paths, but ran icacls /inheritance:r before /grant:r. If the grant failed (service account, transient timeout, ...), the path was left with inherited ACEs stripped and no usable ACE for the current user, making it unopenable (e.g. 'attempt to write a readonly database' on Windows CI). HKUDS#158 reverted the original change for this reason. Redo the restriction in a fail-safe order: 1. grant the current user full control first (icacls /grant:r); 2. only then strip inherited ACEs (icacls /inheritance:r); 3. if the grant fails, leave inherited ACLs untouched so the path stays accessible; if the strip fails, the path is merely less restricted. Wire the restriction into open_private_file, harden_private_tree and _chmod so every private-storage entry point applies it on Windows, and add Windows-only tests asserting dangerous well-known ACEs are removed while the current user retains full control.
|
@Zongwei9888 您好,已处理 CI 失败,请帮忙 review: 1. Linting 失败已修复 — 2. Bundle macOS x64 失败是 GitHub Actions 基础设施故障 — 失败发生在 "Set up job" 阶段( 3. 背景说明 — #148 合并后因 ACL 操作非原子(先 请批准 CI 运行并 review,谢谢! |
1 similar comment
|
@Zongwei9888 您好,已处理 CI 失败,请帮忙 review: 1. Linting 失败已修复 — 2. Bundle macOS x64 失败是 GitHub Actions 基础设施故障 — 失败发生在 "Set up job" 阶段( 3. 背景说明 — #148 合并后因 ACL 操作非原子(先 请批准 CI 运行并 review,谢谢! |
|
@Zongwei9888 您好,补充一个关于 Security CI "Dependency and license audit" 失败的分析: 该 check 失败在 "Audit Node dependencies" 步骤(
本 PR 仅修改 如确认该判断,可忽略此 check 或触发依赖升级。谢谢! |
Summary
Redoes the Windows private-file ACL restriction from #148 in a fail-safe order, addressing the exact regression that forced the revert in #158.
Why #148 was reverted (#158)
The original
_restrict_windows_acl()ran:icacls <path> /inheritance:r(strip inherited ACEs)icacls <path> /grant:r <user>:F(grant the current user)If the grant failed (service account, transient timeout, ...), the path had already lost every inherited ACE and had no usable ACE for the current user — the file became unopenable, surfacing as
sqlite3.OperationalError: attempt to write a readonly databaseon the Windows lifecycle CI job.This fix — grant first, strip second, fail safe
_restrict_windows_acl()now:icacls <path> /grant:r <user>:Ficacls <path> /inheritance:rThe identity is resolved via
whoami(mbcs encoding,errors="replace"), same as the original PR.Wiring
_restrict_windows_acl()is applied at every private-storage entry point on Windows:open_private_file()— Windows branch calls_restrict_windows_acl(target)instead of no-opharden_private_tree()— removed theif os.name == "nt": return baseearly exit so the tree walk repairs ACLs too_chmod()— Windows branch calls_restrict_windows_acl(path)instead of returning earlyTests
New
tests/test_private_storage_windows.py(Windows-only, skipped elsewhere) asserts:Everyone,Authenticated Users,BUILTIN\Users) are removed after restriction(F)after restrictionVerified locally on Windows (3 passed) and POSIX suite unaffected (8 skipped as expected).
References