fix(daemon): validate status directory access

Author: gnanam1990

internal/daemon/status_dir_owner_unix.go

@@ -8,10 +8,10 @@ import (
 	"syscall"
 )
 
-func checkStatusDirOwner(info os.FileInfo) error {
+func checkStatusDirOwner(_ *os.Root, info os.FileInfo) error {
 	stat, ok := info.Sys().(*syscall.Stat_t)
 	if !ok {
-		return nil
+		return fmt.Errorf("status directory ownership metadata is unavailable")
 	}
 	if int(stat.Uid) != os.Geteuid() {
 		return fmt.Errorf("status directory is owned by uid %d, not the current user", stat.Uid)

internal/daemon/status_dir_owner_unix_test.go

@@ -0,0 +1,44 @@
+//go:build !windows
+
+package daemon
+
+import (
+	"os"
+	"strings"
+	"syscall"
+	"testing"
+	"time"
+)
+
+func secureStatusTestDirPlatform(t *testing.T, dir string) {
+	t.Helper()
+	if err := os.Chmod(dir, 0o700); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestCheckStatusDirOwnerRejectsMissingMetadata(t *testing.T) {
+	err := checkStatusDirOwner(nil, statusDirOwnerTestInfo{})
+	if err == nil || !strings.Contains(err.Error(), "metadata is unavailable") {
+		t.Fatalf("checkStatusDirOwner error = %v, want unavailable metadata rejection", err)
+	}
+}
+
+func TestCheckStatusDirOwnerRejectsDifferentUser(t *testing.T) {
+	info := statusDirOwnerTestInfo{sys: &syscall.Stat_t{Uid: uint32(os.Geteuid() + 1)}}
+	err := checkStatusDirOwner(nil, info)
+	if err == nil || !strings.Contains(err.Error(), "not the current user") {
+		t.Fatalf("checkStatusDirOwner error = %v, want owner mismatch rejection", err)
+	}
+}
+
+type statusDirOwnerTestInfo struct {
+	sys any
+}
+
+func (statusDirOwnerTestInfo) Name() string       { return "." }
+func (statusDirOwnerTestInfo) Size() int64        { return 0 }
+func (statusDirOwnerTestInfo) Mode() os.FileMode  { return os.ModeDir | 0o700 }
+func (statusDirOwnerTestInfo) ModTime() time.Time { return time.Time{} }
+func (statusDirOwnerTestInfo) IsDir() bool        { return true }
+func (info statusDirOwnerTestInfo) Sys() any      { return info.sys }

internal/daemon/status_dir_owner_windows.go

@@ -2,11 +2,110 @@
 
 package daemon
 
-import "os"
+import (
+	"errors"
+	"fmt"
+	"os"
+	"unsafe"
 
-// Windows has no portable uid to compare. os.Root still binds every operation
-// to one directory handle and rejects reparse-point traversal, while the normal
-// daemon directory lives below the current user's profile.
-func checkStatusDirOwner(os.FileInfo) error {
+	"golang.org/x/sys/windows"
+)
+
+const statusDirectoryWriteAccess = windows.ACCESS_MASK(
+	windows.GENERIC_ALL |
+		windows.GENERIC_WRITE |
+		windows.DELETE |
+		windows.WRITE_DAC |
+		windows.WRITE_OWNER |
+		windows.FILE_WRITE_DATA |
+		windows.FILE_APPEND_DATA |
+		windows.FILE_WRITE_ATTRIBUTES |
+		windows.FILE_WRITE_EA |
+		0x40, // FILE_DELETE_CHILD
+)
+
+// checkStatusDirOwner validates ownership and write access through a handle
+// opened beneath root. Path-based ACL inspection would recreate the ancestor
+// swap race that Root is intended to close.
+func checkStatusDirOwner(root *os.Root, _ os.FileInfo) (returnErr error) {
+	directory, err := root.Open(".")
+	if err != nil {
+		return fmt.Errorf("open status directory for access validation: %w", err)
+	}
+	defer func() {
+		if err := directory.Close(); err != nil {
+			returnErr = errors.Join(returnErr, fmt.Errorf("close status directory access handle: %w", err))
+		}
+	}()
+
+	raw, err := directory.SyscallConn()
+	if err != nil {
+		return fmt.Errorf("access status directory handle: %w", err)
+	}
+	var descriptor *windows.SECURITY_DESCRIPTOR
+	var queryErr error
+	if err := raw.Control(func(handle uintptr) {
+		descriptor, queryErr = windows.GetSecurityInfo(
+			windows.Handle(handle),
+			windows.SE_FILE_OBJECT,
+			windows.OWNER_SECURITY_INFORMATION|windows.DACL_SECURITY_INFORMATION,
+		)
+	}); err != nil {
+		return fmt.Errorf("inspect status directory access: %w", err)
+	}
+	if queryErr != nil {
+		return fmt.Errorf("inspect status directory owner and DACL: %w", queryErr)
+	}
+	if descriptor == nil {
+		return fmt.Errorf("status directory security descriptor is unavailable")
+	}
+
+	user, err := windows.GetCurrentProcessToken().GetTokenUser()
+	if err != nil {
+		return fmt.Errorf("resolve current Windows user: %w", err)
+	}
+	owner, _, err := descriptor.Owner()
+	if err != nil {
+		return fmt.Errorf("read status directory owner: %w", err)
+	}
+	if owner == nil || !owner.Equals(user.User.Sid) {
+		return fmt.Errorf("status directory is not owned by the current Windows user")
+	}
+
+	dacl, _, err := descriptor.DACL()
+	if err != nil {
+		return fmt.Errorf("read status directory DACL: %w", err)
+	}
+	if dacl == nil {
+		return fmt.Errorf("status directory has an unrestricted Windows DACL")
+	}
+	for index := uint16(0); index < dacl.AceCount; index++ {
+		var ace *windows.ACCESS_ALLOWED_ACE
+		if err := windows.GetAce(dacl, uint32(index), &ace); err != nil {
+			return fmt.Errorf("read status directory DACL entry %d: %w", index, err)
+		}
+		switch ace.Header.AceType {
+		case windows.ACCESS_DENIED_ACE_TYPE:
+			continue
+		case windows.ACCESS_ALLOWED_ACE_TYPE:
+		default:
+			return fmt.Errorf("status directory DACL entry %d has unsupported type %d", index, ace.Header.AceType)
+		}
+		if ace.Mask&statusDirectoryWriteAccess == 0 {
+			continue
+		}
+		trustee := (*windows.SID)(unsafe.Pointer(&ace.SidStart))
+		if !allowedStatusDirectoryTrustee(trustee, user.User.Sid) {
+			return fmt.Errorf("status directory DACL grants write access to unexpected trustee %s", trustee.String())
+		}
+	}
 	return nil
 }
+
+func allowedStatusDirectoryTrustee(trustee, user *windows.SID) bool {
+	return trustee != nil && (trustee.Equals(user) ||
+		trustee.IsWellKnown(windows.WinLocalSystemSid) ||
+		trustee.IsWellKnown(windows.WinBuiltinAdministratorsSid) ||
+		trustee.IsWellKnown(windows.WinCreatorOwnerSid) ||
+		trustee.IsWellKnown(windows.WinCreatorOwnerRightsSid))
+}

internal/daemon/status_dir_owner_windows_test.go

@@ -0,0 +1,92 @@
+//go:build windows
+
+package daemon
+
+import (
+	"fmt"
+	"os"
+	"runtime"
+	"strings"
+	"testing"
+
+	"golang.org/x/sys/windows"
+)
+
+func secureStatusTestDirPlatform(t *testing.T, dir string) {
+	t.Helper()
+	user, err := windows.GetCurrentProcessToken().GetTokenUser()
+	if err != nil {
+		t.Fatal(err)
+	}
+	descriptor, err := windows.SecurityDescriptorFromString(
+		fmt.Sprintf("O:%sD:P(A;OICI;GA;;;%s)(A;OICI;GA;;;SY)", user.User.Sid.String(), user.User.Sid.String()),
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+	dacl, _, err := descriptor.DACL()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := windows.SetNamedSecurityInfo(
+		dir,
+		windows.SE_FILE_OBJECT,
+		windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION,
+		nil,
+		nil,
+		dacl,
+		nil,
+	); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestCheckStatusDirOwnerRejectsBroadDACL(t *testing.T) {
+	dir := t.TempDir()
+	secureStatusTestDirPlatform(t, dir)
+	worldSID, err := windows.CreateWellKnownSid(windows.WinWorldSid)
+	if err != nil {
+		t.Fatal(err)
+	}
+	var pinner runtime.Pinner
+	pinner.Pin(worldSID)
+	defer pinner.Unpin()
+	broadDACL, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{{
+		AccessPermissions: windows.GENERIC_ALL,
+		AccessMode:        windows.GRANT_ACCESS,
+		Inheritance:       windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT,
+		Trustee: windows.TRUSTEE{
+			TrusteeForm:  windows.TRUSTEE_IS_SID,
+			TrusteeType:  windows.TRUSTEE_IS_WELL_KNOWN_GROUP,
+			TrusteeValue: windows.TrusteeValueFromSID(worldSID),
+		},
+	}}, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := windows.SetNamedSecurityInfo(
+		dir,
+		windows.SE_FILE_OBJECT,
+		windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION,
+		nil,
+		nil,
+		broadDACL,
+		nil,
+	); err != nil {
+		t.Fatal(err)
+	}
+
+	root, err := os.OpenRoot(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer root.Close()
+	info, err := root.Stat(".")
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = checkStatusDirOwner(root, info)
+	if err == nil || !strings.Contains(err.Error(), "unexpected trustee") {
+		t.Fatalf("checkStatusDirOwner error = %v, want broad DACL rejection", err)
+	}
+}

internal/daemon/status_file.go

@@ -108,15 +108,23 @@ func writeStatusFileAtomically(
 	if replace != nil {
 		rename = func(src, dst string) error { return replace(root, src, dst) }
 	}
+	var committedWarning error
 	if err := fsutil.RenameWithRetry(tempName, statusName, rename); err != nil {
-		return fmt.Errorf("replace status file: %w", err)
+		var committedReplacement *fsutil.CommittedReplacementCleanupError
+		if !errors.As(err, &committedReplacement) {
+			return fmt.Errorf("replace status file: %w", err)
+		}
+		committedWarning = fmt.Errorf("clean up replaced status file: %w", err)
 	}
 	committed = true
 	if syncParent == nil {
 		syncParent = syncStatusRoot
 	}
 	if err := syncParent(root); err != nil {
-		return &statusFileCommittedError{cause: fmt.Errorf("sync status directory: %w", err)}
+		committedWarning = errors.Join(committedWarning, fmt.Errorf("sync status directory: %w", err))
+	}
+	if committedWarning != nil {
+		return &statusFileCommittedError{cause: committedWarning}
 	}
 	return nil
 }
@@ -132,7 +140,7 @@ func validateStatusRoot(root *os.Root) error {
 	if runtime.GOOS != "windows" && info.Mode().Perm()&0o077 != 0 {
 		return fmt.Errorf("status directory permissions are %04o, want owner-only", info.Mode().Perm())
 	}
-	if err := checkStatusDirOwner(info); err != nil {
+	if err := checkStatusDirOwner(root, info); err != nil {
 		return err
 	}
 	return nil