From de7ac8b0501b09567668fe5e4229b94b34fa5225 Mon Sep 17 00:00:00 2001 From: stantheman0128 Date: Fri, 24 Jul 2026 02:15:28 +0800 Subject: [PATCH] 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 Co-authored-by: Cursor --- pkg/server/discovery/discovery.go | 6 +- pkg/server/discovery/discovery_test.go | 17 ++ pkg/server/discovery/permissions_other.go | 21 +++ pkg/server/discovery/permissions_windows.go | 79 +++++++++ .../discovery/permissions_windows_test.go | 167 ++++++++++++++++++ 5 files changed, 288 insertions(+), 2 deletions(-) create mode 100644 pkg/server/discovery/permissions_other.go create mode 100644 pkg/server/discovery/permissions_windows.go create mode 100644 pkg/server/discovery/permissions_windows_test.go diff --git a/pkg/server/discovery/discovery.go b/pkg/server/discovery/discovery.go index b00ecddfc2..76e81ce913 100644 --- a/pkg/server/discovery/discovery.go +++ b/pkg/server/discovery/discovery.go @@ -78,8 +78,10 @@ func writeServerInfoTo(dir string, info *ServerInfo) error { // Tighten permissions on the directory in case it already existed with // looser permissions. MkdirAll only applies mode to newly-created dirs. - if err := os.Chmod(dir, dirPermissions); err != nil { - return fmt.Errorf("failed to set discovery directory permissions: %w", err) + // On Windows this sets an explicit protected DACL (POSIX modes are + // advisory on NTFS); elsewhere it is os.Chmod(dirPermissions). + if err := restrictDiscoveryDirPermissions(dir); err != nil { + return err } path := filepath.Join(dir, "server.json") diff --git a/pkg/server/discovery/discovery_test.go b/pkg/server/discovery/discovery_test.go index fadf0ec46a..ee8da17b9c 100644 --- a/pkg/server/discovery/discovery_test.go +++ b/pkg/server/discovery/discovery_test.go @@ -6,6 +6,7 @@ package discovery import ( "os" "path/filepath" + "runtime" "testing" "time" @@ -115,6 +116,12 @@ func TestRemoveServerInfo_NotFound(t *testing.T) { func TestWriteServerInfo_FilePermissions(t *testing.T) { t.Parallel() + // POSIX file modes are advisory on NTFS; Windows reports 0666 for + // newly written files regardless of the mode passed to AtomicWriteFile. + // Directory ACL coverage for Windows lives in permissions_windows_test.go. + if runtime.GOOS == "windows" { + t.Skip("POSIX file modes are not meaningful on Windows") + } dir := t.TempDir() info := &ServerInfo{ @@ -132,6 +139,11 @@ func TestWriteServerInfo_FilePermissions(t *testing.T) { func TestWriteServerInfo_CreatesDirectoryWithCorrectPermissions(t *testing.T) { t.Parallel() + // On Windows, writeServerInfoTo sets an explicit DACL instead of relying + // on os.Chmod; see TestWriteServerInfo_WindowsDACL_NoOtherInteractiveUsers. + if runtime.GOOS == "windows" { + t.Skip("POSIX directory modes are not meaningful on Windows; see DACL tests") + } parent := t.TempDir() dir := filepath.Join(parent, "nested", "server") @@ -195,6 +207,11 @@ func TestReadServerInfo_RejectsSymlink(t *testing.T) { func TestWriteServerInfo_TightensExistingDirPermissions(t *testing.T) { t.Parallel() + // On Windows the equivalent "tighten existing" path is covered by + // TestRestrictDiscoveryDirPermissions_ReplacesExistingLooseACL. + if runtime.GOOS == "windows" { + t.Skip("POSIX directory modes are not meaningful on Windows; see DACL tests") + } // Create a directory with deliberately too-loose permissions. dir := t.TempDir() diff --git a/pkg/server/discovery/permissions_other.go b/pkg/server/discovery/permissions_other.go new file mode 100644 index 0000000000..d8b9ed6b01 --- /dev/null +++ b/pkg/server/discovery/permissions_other.go @@ -0,0 +1,21 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package discovery + +import ( + "fmt" + "os" +) + +// restrictDiscoveryDirPermissions tightens POSIX mode bits on the discovery +// directory. On non-Windows platforms this is the Chmod that previously lived +// inline in writeServerInfoTo. +func restrictDiscoveryDirPermissions(dir string) error { + if err := os.Chmod(dir, dirPermissions); err != nil { + return fmt.Errorf("failed to set discovery directory permissions: %w", err) + } + return nil +} diff --git a/pkg/server/discovery/permissions_windows.go b/pkg/server/discovery/permissions_windows.go new file mode 100644 index 0000000000..384f1afefb --- /dev/null +++ b/pkg/server/discovery/permissions_windows.go @@ -0,0 +1,79 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build windows + +package discovery + +import ( + "fmt" + + "golang.org/x/sys/windows" +) + +// restrictDiscoveryDirPermissions replaces the discovery directory DACL with +// an explicit ACL granting FILE-equivalent GenericAll only to the process +// user and SYSTEM, and marks the DACL protected so parent ACEs cannot +// re-inherit. os.Chmod is advisory on NTFS and does not strip inherited +// ACEs under %LOCALAPPDATA%, which is the gap that lets another interactive +// user rewrite server.json (for example to point at an attacker named pipe). +// +// Inheritance (OICI) is intentional: server.json and any future children +// pick up the same restriction instead of inheriting a looser parent ACL. +func restrictDiscoveryDirPermissions(dir string) error { + userSID, err := currentProcessUserSID() + if err != nil { + return fmt.Errorf("failed to resolve current user SID: %w", err) + } + systemSID, err := windows.CreateWellKnownSid(windows.WinLocalSystemSid) + if err != nil { + return fmt.Errorf("failed to resolve SYSTEM SID: %w", err) + } + + acl, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{ + { + AccessPermissions: windows.GENERIC_ALL, + AccessMode: windows.GRANT_ACCESS, + Inheritance: windows.OBJECT_INHERIT_ACE | windows.CONTAINER_INHERIT_ACE, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeValue: windows.TrusteeValueFromSID(userSID), + }, + }, + { + AccessPermissions: windows.GENERIC_ALL, + AccessMode: windows.GRANT_ACCESS, + Inheritance: windows.OBJECT_INHERIT_ACE | windows.CONTAINER_INHERIT_ACE, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_WELL_KNOWN_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(systemSID), + }, + }, + }, nil) + if err != nil { + return fmt.Errorf("failed to build discovery directory DACL: %w", err) + } + + if err := windows.SetNamedSecurityInfo( + dir, + windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, + nil, + nil, + acl, + nil, + ); err != nil { + return fmt.Errorf("failed to set discovery directory DACL: %w", err) + } + return nil +} + +func currentProcessUserSID() (*windows.SID, error) { + token := windows.GetCurrentProcessToken() + tokenUser, err := token.GetTokenUser() + if err != nil { + return nil, err + } + return tokenUser.User.Sid, nil +} diff --git a/pkg/server/discovery/permissions_windows_test.go b/pkg/server/discovery/permissions_windows_test.go new file mode 100644 index 0000000000..b913b026b5 --- /dev/null +++ b/pkg/server/discovery/permissions_windows_test.go @@ -0,0 +1,167 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build windows + +package discovery + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + "unsafe" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" +) + +// TestWriteServerInfo_WindowsDACL_NoOtherInteractiveUsers asserts the +// acceptance criterion from #5217: after writeServerInfoTo, the discovery +// directory DACL grants access only to the current user and SYSTEM, and does +// not retain Everyone / Authenticated Users / other interactive-user ACEs +// that MkdirAll would otherwise inherit (and that os.Chmod cannot strip). +func TestWriteServerInfo_WindowsDACL_NoOtherInteractiveUsers(t *testing.T) { + t.Parallel() + + parent := t.TempDir() + dir := filepath.Join(parent, "toolhive", "server") + + // Seed a deliberately loose ACL on the parent so newly created children + // inherit Everyone. This models a shared / misconfigured LOCALAPPDATA + // tree better than relying on whatever TempDir happens to carry. + grantEveryone(t, parent) + + info := &ServerInfo{ + URL: "npipe://thv-api", + PID: 1, + Nonce: "dacl-nonce", + StartedAt: time.Now().UTC(), + } + require.NoError(t, writeServerInfoTo(dir, info)) + + assertDiscoveryDACLRestricted(t, dir) +} + +// TestRestrictDiscoveryDirPermissions_ReplacesExistingLooseACL covers the +// sibling failure mode: the discovery directory already exists with a loose +// DACL (Everyone + inherited ACEs). restrictDiscoveryDirPermissions must +// replace that ACL rather than merge, and block further inheritance. +func TestRestrictDiscoveryDirPermissions_ReplacesExistingLooseACL(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + grantEveryone(t, dir) + + before := discoveryDirSDDL(t, dir) + require.Contains(t, strings.ToUpper(before), "WD", "precondition: Everyone (WD) must be present before restrict") + + require.NoError(t, restrictDiscoveryDirPermissions(dir)) + assertDiscoveryDACLRestricted(t, dir) +} + +// TestRestrictDiscoveryDirPermissions_NewDirectory covers the create path +// (no pre-existing ACL to replace) so MkdirAll + restrict still lands a +// protected owner/SYSTEM-only DACL. +func TestRestrictDiscoveryDirPermissions_NewDirectory(t *testing.T) { + t.Parallel() + + parent := t.TempDir() + grantEveryone(t, parent) + dir := filepath.Join(parent, "fresh-server") + require.NoError(t, os.MkdirAll(dir, dirPermissions)) + + require.NoError(t, restrictDiscoveryDirPermissions(dir)) + assertDiscoveryDACLRestricted(t, dir) +} + +func grantEveryone(t *testing.T, path string) { + t.Helper() + // icacls grants are the product-path way to introduce a loose ACE; + // quoting keeps PowerShell from expanding (OI)/(CI). + cmd := exec.Command("icacls", path, "/grant", "*S-1-1-0:(OI)(CI)M") + out, err := cmd.CombinedOutput() + require.NoError(t, err, "icacls grant Everyone failed: %s", out) +} + +func assertDiscoveryDACLRestricted(t *testing.T, dir string) { + t.Helper() + + userSID, err := currentProcessUserSID() + require.NoError(t, err) + systemSID, err := windows.CreateWellKnownSid(windows.WinLocalSystemSid) + require.NoError(t, err) + everyoneSID, err := windows.CreateWellKnownSid(windows.WinWorldSid) + require.NoError(t, err) + authUsersSID, err := windows.CreateWellKnownSid(windows.WinAuthenticatedUserSid) + require.NoError(t, err) + + sd, err := windows.GetNamedSecurityInfo( + dir, + windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION, + ) + require.NoError(t, err) + + control, _, err := sd.Control() + require.NoError(t, err) + assert.NotZero(t, control&windows.SE_DACL_PROTECTED, "DACL must be protected against inheritance") + + dacl, _, err := sd.DACL() + require.NoError(t, err) + require.NotNil(t, dacl) + + aces, err := allowACEsFromACL(dacl) + require.NoError(t, err) + + var userSeen, systemSeen bool + for _, ace := range aces { + sid := (*windows.SID)(unsafe.Pointer(&ace.SidStart)) + switch { + case userSID.Equals(sid): + userSeen = true + case systemSID.Equals(sid): + systemSeen = true + case everyoneSID.Equals(sid): + t.Fatalf("DACL still grants Everyone (%s)", sid) + case authUsersSID.Equals(sid): + t.Fatalf("DACL still grants Authenticated Users (%s)", sid) + default: + // Administrators / package SIDs / other interactive users must + // not remain after an explicit replace. Fail closed on anything + // that is not the process user or SYSTEM. + t.Fatalf("unexpected allow ACE for SID %s (want only current user + SYSTEM)", sid) + } + } + assert.True(t, userSeen, "DACL must grant the current process user") + assert.True(t, systemSeen, "DACL must grant SYSTEM") +} + +func discoveryDirSDDL(t *testing.T, dir string) string { + t.Helper() + sd, err := windows.GetNamedSecurityInfo( + dir, + windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION, + ) + require.NoError(t, err) + return sd.String() +} + +func allowACEsFromACL(acl *windows.ACL) ([]*windows.ACCESS_ALLOWED_ACE, error) { + aces := make([]*windows.ACCESS_ALLOWED_ACE, 0, acl.AceCount) + for i := uint16(0); i < acl.AceCount; i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(acl, uint32(i), &ace); err != nil { + return nil, err + } + if ace.Header.AceType != windows.ACCESS_ALLOWED_ACE_TYPE { + continue + } + aces = append(aces, ace) + } + return aces, nil +}