fix(config): enforce owner-only ACL on Windows saves

.changeset/windows-private-config-acl.md

@@ -0,0 +1,5 @@
+---
+"ftw": patch
+---
+
+Create saved config temp files with an owner-only Windows ACL before writing credentials, including when an atomic save replaces an existing config.

.github/workflows/windows-config.yml

@@ -0,0 +1,35 @@
+name: Windows config
+
+on:
+  pull_request:
+    branches: [master]
+    paths:
+      - "go/internal/config/**"
+      - "go/go.mod"
+      - "go/go.sum"
+      - ".github/workflows/windows-config.yml"
+  push:
+    branches: [master]
+    paths:
+      - "go/internal/config/**"
+      - "go/go.mod"
+      - "go/go.sum"
+      - ".github/workflows/windows-config.yml"
+
+permissions:
+  contents: read
+
+jobs:
+  windows-config:
+    name: Windows config ACL
+    runs-on: windows-latest
+    timeout-minutes: 10
+    steps:
+      - uses: actions/checkout@v5
+      - uses: actions/setup-go@v7
+        with:
+          go-version-file: go/go.mod
+          cache: false
+      - name: Test config package and Windows ACLs
+        working-directory: go
+        run: go test -count=1 -timeout 2m ./internal/config

go/go.mod

@@ -17,6 +17,7 @@ require (
 	github.com/simonvetter/modbus v1.6.4
 	github.com/yuin/gopher-lua v1.1.2
 	golang.org/x/net v0.56.0
+	golang.org/x/sys v0.46.0
 	gopkg.in/yaml.v3 v3.0.1
 	modernc.org/sqlite v1.48.2
 )
@@ -51,7 +52,6 @@ require (
 	github.com/yusufpapurcu/wmi v1.2.4 // indirect
 	golang.org/x/crypto v0.53.0 // indirect
 	golang.org/x/sync v0.19.0 // indirect
-	golang.org/x/sys v0.46.0 // indirect
 	google.golang.org/protobuf v1.34.2 // indirect
 	gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect
 	modernc.org/libc v1.70.0 // indirect

go/internal/config/config.go

@@ -13,7 +13,6 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
-	"runtime"
 	"strings"
 	"sync"
 	"time"
@@ -1914,20 +1913,6 @@ var defaultDurableWriter = durableWriter{
 	syncDir:  syncDir,
 }
 
-// syncDir fsyncs a directory so a completed rename survives power loss.
-// Best-effort on platforms where directories can't be fsynced (Windows).
-func syncDir(dir string) error {
-	d, err := os.Open(dir)
-	if err != nil {
-		return err
-	}
-	defer d.Close()
-	if err := d.Sync(); err != nil && runtime.GOOS != "windows" {
-		return err
-	}
-	return nil
-}
-
 // SaveAtomic writes config to disk via tmp-file + rename. Safe from partial
 // writes and from power loss: the temp file is fsynced before the rename and
 // the containing directory is fsynced after it.
@@ -1964,7 +1949,7 @@ func saveAtomic(w durableWriter, path string, c *Config) error {
 	if err := os.Remove(tmp); err != nil && !errors.Is(err, os.ErrNotExist) {
 		return fmt.Errorf("clear stale tmp: %w", err)
 	}
-	f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_EXCL, configFileMode)
+	f, err := createConfigTemp(tmp, configFileMode)
 	if err != nil {
 		return fmt.Errorf("create tmp: %w", err)
 	}
@@ -1986,7 +1971,7 @@ func saveAtomic(w durableWriter, path string, c *Config) error {
 		os.Remove(tmp)
 		return fmt.Errorf("close tmp: %w", err)
 	}
-	if err := os.Rename(tmp, path); err != nil {
+	if err := replaceConfigTemp(tmp, path); err != nil {
 		os.Remove(tmp)
 		return fmt.Errorf("rename tmp: %w", err)
 	}

go/internal/config/config_file_unix.go

@@ -0,0 +1,23 @@
+//go:build !windows
+
+package config
+
+import "os"
+
+func createConfigTemp(path string, mode os.FileMode) (*os.File, error) {
+	return os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, mode)
+}
+
+func replaceConfigTemp(tmp, path string) error {
+	return os.Rename(tmp, path)
+}
+
+// syncDir fsyncs a directory so a completed rename survives power loss.
+func syncDir(dir string) error {
+	d, err := os.Open(dir)
+	if err != nil {
+		return err
+	}
+	defer d.Close()
+	return d.Sync()
+}

go/internal/config/config_file_unix_test.go

@@ -0,0 +1,19 @@
+//go:build !windows
+
+package config
+
+import (
+	"fmt"
+	"os"
+)
+
+func verifyConfigFileOwnerOnly(path string) error {
+	info, err := os.Stat(path)
+	if err != nil {
+		return err
+	}
+	if got := info.Mode().Perm(); got != configFileMode.Perm() {
+		return fmt.Errorf("saved config mode = %04o, want %04o", got, configFileMode.Perm())
+	}
+	return nil
+}

go/internal/config/config_file_windows.go

@@ -0,0 +1,112 @@
+//go:build windows
+
+package config
+
+import (
+	"fmt"
+	"os"
+	"unsafe"
+
+	"golang.org/x/sys/windows"
+)
+
+// createConfigTemp passes a protected security descriptor to CreateFile. The
+// descriptor must be present at creation time: applying an ACL after
+// os.OpenFile would leave a race in which another local user could read the
+// credentials in the temp file before the ACL was tightened.
+func createConfigTemp(path string, _ os.FileMode) (*os.File, error) {
+	sd, err := ownerOnlyConfigSecurityDescriptor()
+	if err != nil {
+		return nil, fmt.Errorf("build private config security descriptor: %w", err)
+	}
+	pathPtr, err := windows.UTF16PtrFromString(path)
+	if err != nil {
+		return nil, err
+	}
+	sa := &windows.SecurityAttributes{
+		Length:             uint32(unsafe.Sizeof(windows.SecurityAttributes{})),
+		SecurityDescriptor: sd,
+	}
+	h, err := windows.CreateFile(
+		pathPtr,
+		windows.GENERIC_WRITE,
+		windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE,
+		sa,
+		windows.CREATE_NEW,
+		windows.FILE_ATTRIBUTE_NORMAL,
+		0,
+	)
+	if err != nil {
+		return nil, err
+	}
+	f := os.NewFile(uintptr(h), path)
+	if f == nil {
+		_ = windows.CloseHandle(h)
+		return nil, fmt.Errorf("wrap config temp handle")
+	}
+	return f, nil
+}
+
+func ownerOnlyConfigSecurityDescriptor() (*windows.SECURITY_DESCRIPTOR, error) {
+	token, err := windows.OpenCurrentProcessToken()
+	if err != nil {
+		return nil, fmt.Errorf("open current process token: %w", err)
+	}
+	defer token.Close()
+	user, err := token.GetTokenUser()
+	if err != nil {
+		return nil, fmt.Errorf("read current user SID: %w", err)
+	}
+	ownerSID := user.User.Sid
+
+	acl, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{{
+		AccessPermissions: windows.GENERIC_ALL,
+		AccessMode:        windows.GRANT_ACCESS,
+		Inheritance:       windows.NO_INHERITANCE,
+		Trustee: windows.TRUSTEE{
+			TrusteeForm:  windows.TRUSTEE_IS_SID,
+			TrusteeType:  windows.TRUSTEE_IS_USER,
+			TrusteeValue: windows.TrusteeValueFromSID(ownerSID),
+		},
+	}}, nil)
+	if err != nil {
+		return nil, fmt.Errorf("build owner ACL: %w", err)
+	}
+	sd, err := windows.NewSecurityDescriptor()
+	if err != nil {
+		return nil, fmt.Errorf("initialize security descriptor: %w", err)
+	}
+	if err := sd.SetOwner(ownerSID, false); err != nil {
+		return nil, fmt.Errorf("set security descriptor owner: %w", err)
+	}
+	if err := sd.SetDACL(acl, true, false); err != nil {
+		return nil, fmt.Errorf("set security descriptor DACL: %w", err)
+	}
+	// Do not let an inheritable ACE from the config directory re-open the
+	// file to another local account after CreateFile applies this descriptor.
+	if err := sd.SetControl(windows.SE_DACL_PROTECTED, windows.SE_DACL_PROTECTED); err != nil {
+		return nil, fmt.Errorf("protect security descriptor DACL: %w", err)
+	}
+	return sd.ToSelfRelative()
+}
+
+func replaceConfigTemp(tmp, path string) error {
+	// os.Rename uses MoveFileEx with REPLACE_EXISTING on Windows. Keep Go's
+	// long-path handling and the existing atomic replacement semantics; the
+	// temp file's protected DACL becomes the destination file's DACL.
+	return os.Rename(tmp, path)
+}
+
+// syncDir fsyncs a directory where Windows permits it. Windows filesystems
+// may reject directory FlushFileBuffers, so retain #792's best-effort rule.
+func syncDir(dir string) error {
+	d, err := os.Open(dir)
+	if err != nil {
+		return err
+	}
+	defer d.Close()
+	if err := d.Sync(); err != nil {
+		return nil
+	}
+	return nil
+}

go/internal/config/config_file_windows_test.go

@@ -0,0 +1,85 @@
+//go:build windows
+
+package config
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"testing"
+	"unsafe"
+
+	"golang.org/x/sys/windows"
+)
+
+func verifyConfigFileOwnerOnly(path string) error {
+	token, err := windows.OpenCurrentProcessToken()
+	if err != nil {
+		return fmt.Errorf("open current process token: %w", err)
+	}
+	defer token.Close()
+	user, err := token.GetTokenUser()
+	if err != nil {
+		return fmt.Errorf("read current user SID: %w", err)
+	}
+	sd, err := windows.GetNamedSecurityInfo(
+		path,
+		windows.SE_FILE_OBJECT,
+		windows.OWNER_SECURITY_INFORMATION|windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION,
+	)
+	if err != nil {
+		return fmt.Errorf("read config security descriptor: %w", err)
+	}
+	control, _, err := sd.Control()
+	if err != nil {
+		return fmt.Errorf("read security descriptor control: %w", err)
+	}
+	if control&windows.SE_DACL_PROTECTED == 0 {
+		return fmt.Errorf("config DACL is inheritable: control=%#x", control)
+	}
+	owner, _, err := sd.Owner()
+	if err != nil {
+		return fmt.Errorf("read config owner: %w", err)
+	}
+	if !owner.Equals(user.User.Sid) {
+		return fmt.Errorf("config owner SID = %s, want current user %s", owner.String(), user.User.Sid.String())
+	}
+	dacl, _, err := sd.DACL()
+	if err != nil {
+		return fmt.Errorf("read config DACL: %w", err)
+	}
+	if dacl.AceCount != 1 {
+		return fmt.Errorf("config DACL ACE count = %d, want one owner ACE", dacl.AceCount)
+	}
+	var ace *windows.ACCESS_ALLOWED_ACE
+	if err := windows.GetAce(dacl, 0, &ace); err != nil {
+		return fmt.Errorf("read config DACL ACE: %w", err)
+	}
+	if ace.Header.AceType != windows.ACCESS_ALLOWED_ACE_TYPE {
+		return fmt.Errorf("config DACL ACE type = %d, want allow", ace.Header.AceType)
+	}
+	aceSID := (*windows.SID)(unsafe.Pointer(&ace.SidStart))
+	if !aceSID.Equals(user.User.Sid) {
+		return fmt.Errorf("config DACL ACE SID = %s, want current user %s", aceSID.String(), user.User.Sid.String())
+	}
+	required := uint32(windows.FILE_READ_DATA | windows.FILE_WRITE_DATA)
+	if uint32(ace.Mask)&required != required {
+		return fmt.Errorf("config owner ACE mask = %#x, lacks read/write", ace.Mask)
+	}
+	return nil
+}
+
+func TestCreateConfigTempUsesOwnerOnlyACLBeforeFirstWrite(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "config.yaml.tmp")
+	f, err := createConfigTemp(path, configFileMode)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.Remove(path)
+	if err := verifyConfigFileOwnerOnly(path); err != nil {
+		t.Fatal(err)
+	}
+	if err := f.Close(); err != nil {
+		t.Fatal(err)
+	}
+}

go/internal/config/config_test.go

@@ -719,12 +719,8 @@ func TestSaveAtomicWritesOwnerOnlyMode(t *testing.T) {
 			if err := SaveAtomic(path, c); err != nil {
 				t.Fatal(err)
 			}
-			info, err := os.Stat(path)
-			if err != nil {
-				t.Fatal(err)
-			}
-			if got := info.Mode().Perm(); got != 0o600 {
-				t.Errorf("saved config mode = %04o, want 0600 — the file holds MQTT passwords and OAuth refresh tokens", got)
+			if err := verifyConfigFileOwnerOnly(path); err != nil {
+				t.Errorf("saved config is not owner-only — the file holds MQTT passwords and OAuth refresh tokens: %v", err)
 			}
 			if _, err := os.Stat(path + ".tmp"); !os.IsNotExist(err) {
 				t.Errorf("tmp file survived the save: %v", err)