stacklok / toolhive / 30927597747

70%

Build Type

push

github

Committed by web-flow

Commit Message

Set Windows DACL on discovery directory under LOCALAPPDATA (#5951)

* Set Windows DACL on discovery directory

os.Chmod is advisory on NTFS, so the discovery directory under
%LOCALAPPDATA% could keep inherited ACEs (Everyone, other interactive
users). A local attacker who can write server.json can redirect the
npipe URL and steal the discovery nonce on the next health check.

On Windows, replace the directory DACL with a protected ACL granting
GenericAll only to the process user and SYSTEM (create and existing-dir
paths). Non-Windows keeps os.Chmod. Windows tests assert no other
interactive users remain; POSIX mode assertions skip on Windows.

Fixes #5217

Signed-off-by: stantheman0128 <stanshih888@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

* Lock down discovery dir before startup trusts it

Review follow-up. The DACL work only ran inside writeServerInfoTo, which is
the last step of startup: pkg/api creates the directory, takes
server.json.lock inside it, and runs Discover before it ever writes. A
StateRunning result returns before the write, so a directory left loose by
an earlier run stayed loose for the whole startup, and the server.json in it
was trusted regardless.

Move the lockdown into discovery.EnsureSecureDir and call it from
writeDiscoveryFile ahead of the lock and Discover. Restrict the intermediate
toolhive directory as well as the server leaf: inherited Modify on the
intermediate carries DELETE, so another interactive user could rename it
aside, recreate the chain with an ACL of their own, and leave the protected
leaf unread.

Validate ownership before treating a directory as locked down. Owners keep
WRITE_DAC implicitly, so replacing the DACL of a directory somebody else
pre-created is cosmetic, since that owner can make it permissive again. Fail
closed unless the owner is the process user, SYSTEM, or Administrators; a
standard user cannot create a directory owned by any of those, and an
elevated thv serve legitimately ... (continued)

Coverage Stats

51 of 122 new or added lines in 5 files covered. (41.8%)

8 existing lines in 4 files now uncovered.

82990 of 118658 relevant lines covered (69.94%)

98.04 hits per line