From 3d9f48207581d9293c84a6a0a71a3ebcad2ce2f4 Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Tue, 4 Aug 2026 15:36:23 +0200 Subject: [PATCH 1/2] fix(config): protect Windows config ACL --- .changeset/windows-private-config-acl.md | 5 + go/go.mod | 2 +- go/internal/config/config.go | 19 +-- go/internal/config/config_file_unix.go | 23 ++++ go/internal/config/config_file_unix_test.go | 19 +++ go/internal/config/config_file_windows.go | 112 ++++++++++++++++++ .../config/config_file_windows_test.go | 85 +++++++++++++ go/internal/config/config_test.go | 8 +- 8 files changed, 249 insertions(+), 24 deletions(-) create mode 100644 .changeset/windows-private-config-acl.md create mode 100644 go/internal/config/config_file_unix.go create mode 100644 go/internal/config/config_file_unix_test.go create mode 100644 go/internal/config/config_file_windows.go create mode 100644 go/internal/config/config_file_windows_test.go diff --git a/.changeset/windows-private-config-acl.md b/.changeset/windows-private-config-acl.md new file mode 100644 index 00000000..a3378247 --- /dev/null +++ b/.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. diff --git a/go/go.mod b/go/go.mod index 697b6962..6b6c97a4 100644 --- a/go/go.mod +++ b/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 diff --git a/go/internal/config/config.go b/go/internal/config/config.go index a99b357f..95376fef 100644 --- a/go/internal/config/config.go +++ b/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) } diff --git a/go/internal/config/config_file_unix.go b/go/internal/config/config_file_unix.go new file mode 100644 index 00000000..bd15396c --- /dev/null +++ b/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() +} diff --git a/go/internal/config/config_file_unix_test.go b/go/internal/config/config_file_unix_test.go new file mode 100644 index 00000000..231fc764 --- /dev/null +++ b/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 +} diff --git a/go/internal/config/config_file_windows.go b/go/internal/config/config_file_windows.go new file mode 100644 index 00000000..d6115a54 --- /dev/null +++ b/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 +} diff --git a/go/internal/config/config_file_windows_test.go b/go/internal/config/config_file_windows_test.go new file mode 100644 index 00000000..f0b20e93 --- /dev/null +++ b/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) + } +} diff --git a/go/internal/config/config_test.go b/go/internal/config/config_test.go index cbfcd10f..31fe0d37 100644 --- a/go/internal/config/config_test.go +++ b/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) From 62ae1ce2054fbe2609994b78c1605b9fe2a5d4db Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Tue, 4 Aug 2026 15:48:24 +0200 Subject: [PATCH 2/2] ci: run Windows config ACL tests --- .github/workflows/windows-config.yml | 35 ++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 .github/workflows/windows-config.yml diff --git a/.github/workflows/windows-config.yml b/.github/workflows/windows-config.yml new file mode 100644 index 00000000..4567c19f --- /dev/null +++ b/.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