Skip to content

fix(security): restrict Windows private files with fail-safe ACL ordering - #164

Open
raymondginger2018-sudo wants to merge 2 commits into
HKUDS:mainfrom
raymondginger2018-sudo:fix/win-private-acl-safe-order
Open

fix(security): restrict Windows private files with fail-safe ACL ordering#164
raymondginger2018-sudo wants to merge 2 commits into
HKUDS:mainfrom
raymondginger2018-sudo:fix/win-private-acl-safe-order

Conversation

@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor

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:

  1. icacls <path> /inheritance:r (strip inherited ACEs)
  2. 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 database on the Windows lifecycle CI job.

This fix — grant first, strip second, fail safe

_restrict_windows_acl() now:

  1. Grants the current user full control first: icacls <path> /grant:r <user>:F
  2. Only if the grant succeeds, strips inherited ACEs: icacls <path> /inheritance:r
  3. Grant fails → return, leaving the inherited ACLs untouched; the path stays accessible.
  4. Strip fails → the path is merely less restricted, still fully usable.

The 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-op
  • harden_private_tree() — removed the if os.name == "nt": return base early exit so the tree walk repairs ACLs too
  • _chmod() — Windows branch calls _restrict_windows_acl(path) instead of returning early

Tests

New tests/test_private_storage_windows.py (Windows-only, skipped elsewhere) asserts:

  • Dangerous well-known ACEs (Everyone, Authenticated Users, BUILTIN\Users) are removed after restriction
  • The current user retains Full Control (F) after restriction
  • Covers private directories, private files, and full tree hardening

Verified locally on Windows (3 passed) and POSIX suite unaffected (8 skipped as expected).

References

…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.
@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor Author

@Zongwei9888 您好,已处理 CI 失败,请帮忙 review:

1. Linting 失败已修复tests/test_private_storage_windows.py_acl_lines 的列表推导式已按 ruff format 折叠为单行(commit 147a539)。

2. Bundle macOS x64 失败是 GitHub Actions 基础设施故障 — 失败发生在 "Set up job" 阶段(Failed to resolve action download info. Error: Service UnavailableInternal Server Error),早于 checkout,与代码无关;Linux x64 / Windows x64 / macOS arm64 均通过。重跑即可。

3. 背景说明#148 合并后因 ACL 操作非原子(先 /inheritance:r/grant:r,grant 失败时路径已剥离继承 ACE → 只读数据库)被 #158 revert。本 PR 以 fail-safe 顺序重做:先 grant 当前用户完全控制,成功后才剥离继承 ACE;grant 失败则 ACL 原样保留,strip 失败仅导致限制更宽松,不会出现只读问题。

请批准 CI 运行并 review,谢谢!

1 similar comment
@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor Author

@Zongwei9888 您好,已处理 CI 失败,请帮忙 review:

1. Linting 失败已修复tests/test_private_storage_windows.py_acl_lines 的列表推导式已按 ruff format 折叠为单行(commit 147a539)。

2. Bundle macOS x64 失败是 GitHub Actions 基础设施故障 — 失败发生在 "Set up job" 阶段(Failed to resolve action download info. Error: Service UnavailableInternal Server Error),早于 checkout,与代码无关;Linux x64 / Windows x64 / macOS arm64 均通过。重跑即可。

3. 背景说明#148 合并后因 ACL 操作非原子(先 /inheritance:r/grant:r,grant 失败时路径已剥离继承 ACE → 只读数据库)被 #158 revert。本 PR 以 fail-safe 顺序重做:先 grant 当前用户完全控制,成功后才剥离继承 ACE;grant 失败则 ACL 原样保留,strip 失败仅导致限制更宽松,不会出现只读问题。

请批准 CI 运行并 review,谢谢!

@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor Author

@Zongwei9888 您好,补充一个关于 Security CI "Dependency and license audit" 失败的分析:

该 check 失败在 "Audit Node dependencies" 步骤(npm ci && npm audit --audit-level=high)。经查 Security CI 运行历史:

本 PR 仅修改 core/private_storage.pytests/test_private_storage_windows.py,未触碰 desktop/ 依赖树。代码无差异、仅有时间差异,推断失败原因是 8/7–8/8 之间 npm registry 发布了新的高危漏洞公告,使 npm audit --audit-level=high 从绿变红——这与 PR 代码无关,是上游依赖需要升级(类似 #166 raise aiohttp floor 的处理方式)。

如确认该判断,可忽略此 check 或触发依赖升级。谢谢!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants