Background
Raised by @aponcedeleonch in the PR-summary section of #5201's review. Splitting it out because the underlying code is pre-existing (not introduced by #5201) and the fix is its own change.
Problem
writeServerInfoTo in pkg/server/discovery/discovery.go creates the discovery directory under xdg.StateHome with POSIX modes (os.MkdirAll(dir, dirPermissions) and os.Chmod(dir, dirPermissions)). On Windows, xdg.StateHome lands under %LOCALAPPDATA%, which is per-user by default — so the trust boundary mostly holds — but the POSIX modes passed to those calls are advisory on NTFS.
Combined with the named-pipe transport added in #5201, this matters because:
- The discovery file (
server.json) records the listener URL, including the npipe://<name> form.
discovery.Discover reads that file and routes the URL through HTTPClientForURL, which dials the recorded pipe with no further validation beyond the parser.
- If a local attacker can write to that directory, they can rewrite
server.json to point at their own pipe (npipe://attacker-pipe), and the next thv serve startup health check will dial the attacker's pipe over plaintext HTTP — leaking the discovery nonce in the process.
Proposed fix
After os.MkdirAll(dir, dirPermissions) on Windows, set an explicit DACL on the directory via windows.SetNamedSecurityInfo granting the current user only (and SYSTEM). This needs to be Windows-only build-tagged code so the POSIX path is unchanged.
The pipe-side counterpart is being addressed in #5214.
Acceptance criteria
Out of scope
- Migrating existing on-disk directories — the discovery file is per-user under
%LOCALAPPDATA% and overwritten on every thv serve startup, so a one-off Chmod-equivalent on startup is enough.
Background
Raised by @aponcedeleonch in the PR-summary section of #5201's review. Splitting it out because the underlying code is pre-existing (not introduced by #5201) and the fix is its own change.
Problem
writeServerInfoToinpkg/server/discovery/discovery.gocreates the discovery directory underxdg.StateHomewith POSIX modes (os.MkdirAll(dir, dirPermissions)andos.Chmod(dir, dirPermissions)). On Windows,xdg.StateHomelands under%LOCALAPPDATA%, which is per-user by default — so the trust boundary mostly holds — but the POSIX modes passed to those calls are advisory on NTFS.Combined with the named-pipe transport added in #5201, this matters because:
server.json) records the listener URL, including thenpipe://<name>form.discovery.Discoverreads that file and routes the URL throughHTTPClientForURL, which dials the recorded pipe with no further validation beyond the parser.server.jsonto point at their own pipe (npipe://attacker-pipe), and the nextthv servestartup health check will dial the attacker's pipe over plaintext HTTP — leaking the discovery nonce in the process.Proposed fix
After
os.MkdirAll(dir, dirPermissions)on Windows, set an explicit DACL on the directory viawindows.SetNamedSecurityInfogranting the current user only (and SYSTEM). This needs to be Windows-only build-tagged code so the POSIX path is unchanged.The pipe-side counterpart is being addressed in #5214.
Acceptance criteria
thvand SYSTEM.Out of scope
%LOCALAPPDATA%and overwritten on everythv servestartup, so a one-offChmod-equivalent on startup is enough.