Skip to content

fix(security): restrict Windows private files to the current user - #148

Closed
raymondginger2018-sudo wants to merge 1 commit into
HKUDS:mainfrom
raymondginger2018-sudo:fix/win-private-acl
Closed

fix(security): restrict Windows private files to the current user#148
raymondginger2018-sudo wants to merge 1 commit into
HKUDS:mainfrom
raymondginger2018-sudo:fix/win-private-acl

Conversation

@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor

On Windows, POSIX mode bits are ignored, so credentials.json and other private state inherited the permissive profile ACL (Authenticated Users typically gets Modify). An attacker with a foothold could read API keys from a file the process thought was private.

Fix

  • private_storage._restrict_windows_acl: removes inherited ACEs (icacls /inheritance:r) then grants the current user exclusive full control (/grant:r <user>:F). Best-effort, mirroring POSIX chmod semantics.
  • _windows_identity: resolves the fully-qualified principal via whoami.
  • open_private_file now applies the ACL on Windows (previously skipped the whole branch).
  • harden_private_tree walks and repairs ACLs on Windows too (previously bailed out early).
  • icacls/whoami output decoded as mbcs (OEM codepage) to avoid UnicodeDecodeError on CJK systems.

Tests

tests/test_private_storage_windows.py: directory restricted, file restricted, current-user grant retained. Runs on Windows only; skipped on POSIX.

On Windows, POSIX mode bits are ignored, so credentials.json and other
private state inherited the permissive profile ACL (Authenticated Users
typically gets Modify). An attacker with a foothold could read API keys
from a file that only the process *thought* was private.

- private_storage._restrict_windows_acl: removes inherited ACEs with
  icacls /inheritance:r, then grants the current user exclusive full
  control (/grant:r <user>:F). Best-effort like POSIX chmod.
- _windows_identity: resolves DOMAIN\user via whoami (machine or domain).
- open_private_file now applies the ACL on Windows (previously skipped).
- harden_private_tree walks and repairs ACLs on Windows too (previously
  bailed out early).
- icacls/whoami output decoded as mbcs (Windows OEM codepage) to avoid
  UnicodeDecodeError on CJK systems.

Adds tests/test_private_storage_windows.py: directory, file, and
current-user grant assertions (skipped on POSIX).
@Zongwei9888

Copy link
Copy Markdown
Collaborator

Merged as 576ed69 (via #156), then reverted in #158. The problem it fixes is real, but the implementation is not fail-safe. Details so this can come back correctly:

_restrict_windows_acl() runs two icacls calls in sequence — /inheritance:r then /grant:r <user>:F. That sequence is not atomic, and the except branch returns without undoing step 1. When the grant fails or has not taken effect, the file is left with no usable ACE at all. On main this surfaced as:

sqlite3.OperationalError: attempt to write a readonly database
core\persistence\database.py:160

It is also intermittent — the same tree passed the Windows job on the PR branch and failed on main, so a single green run does not clear it.

There is a second issue worth folding in: with the Windows early-return removed from harden_private_tree(), _chmod() spawns two icacls processes per path during a full tree walk. The Windows lifecycle step went from 21s to 128s. That cost is what made the race easy to hit.

Two changes would make me comfortable merging this:

  1. Grant before stripping inheritance, and return without touching inheritance if the grant fails. Worst case then is permissions not tightened, rather than a file nothing can open.
  2. Do not re-apply the ACL on every open. open_private_file() currently calls it on each call; once at creation is enough.

The underlying point stands — POSIX mode bits are ignored on Windows and credentials.json really does inherit the profile ACL. I'd like this fixed. Happy to review a revised version, or to take it over if you'd rather not.

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