From fcf90260cef795fc4f4ce98d425da6c69070eecb Mon Sep 17 00:00:00 2001 From: Aaron Klotz Date: Mon, 13 Jan 2025 13:02:47 -0700 Subject: [PATCH] atomicfile: use ReplaceFile on Windows so that attributes and ACLs are preserved I moved the actual rename into separate, GOOS-specific files. On non-Windows, we do a simple os.Rename. On Windows, we first try ReplaceFile with a fallback to os.Rename if the target file does not exist. ReplaceFile is the recommended way to rename the file in this use case, as it preserves attributes and ACLs set on the target file. Updates #14428 Signed-off-by: Aaron Klotz --- atomicfile/atomicfile.go | 7 +- atomicfile/atomicfile_notwindows.go | 14 +++ atomicfile/atomicfile_windows.go | 33 ++++++ atomicfile/atomicfile_windows_test.go | 146 ++++++++++++++++++++++++++ atomicfile/mksyscall.go | 8 ++ atomicfile/zsyscall_windows.go | 52 +++++++++ cmd/derper/depaware.txt | 2 +- cmd/k8s-operator/depaware.txt | 2 +- cmd/tailscale/depaware.txt | 2 +- cmd/tailscaled/depaware.txt | 2 +- 10 files changed, 261 insertions(+), 7 deletions(-) create mode 100644 atomicfile/atomicfile_notwindows.go create mode 100644 atomicfile/atomicfile_windows.go create mode 100644 atomicfile/atomicfile_windows_test.go create mode 100644 atomicfile/mksyscall.go create mode 100644 atomicfile/zsyscall_windows.go diff --git a/atomicfile/atomicfile.go b/atomicfile/atomicfile.go index 5c18e85a8..b3c8c93da 100644 --- a/atomicfile/atomicfile.go +++ b/atomicfile/atomicfile.go @@ -15,8 +15,9 @@ import ( ) // WriteFile writes data to filename+some suffix, then renames it into filename. -// The perm argument is ignored on Windows. If the target filename already -// exists but is not a regular file, WriteFile returns an error. +// The perm argument is ignored on Windows, but if the target filename already +// exists then the target file's attributes and ACLs are preserved. If the target +// filename already exists but is not a regular file, WriteFile returns an error. func WriteFile(filename string, data []byte, perm os.FileMode) (err error) { fi, err := os.Stat(filename) if err == nil && !fi.Mode().IsRegular() { @@ -47,5 +48,5 @@ func WriteFile(filename string, data []byte, perm os.FileMode) (err error) { if err := f.Close(); err != nil { return err } - return os.Rename(tmpName, filename) + return rename(tmpName, filename) } diff --git a/atomicfile/atomicfile_notwindows.go b/atomicfile/atomicfile_notwindows.go new file mode 100644 index 000000000..1ce2bb8ac --- /dev/null +++ b/atomicfile/atomicfile_notwindows.go @@ -0,0 +1,14 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +//go:build !windows + +package atomicfile + +import ( + "os" +) + +func rename(srcFile, destFile string) error { + return os.Rename(srcFile, destFile) +} diff --git a/atomicfile/atomicfile_windows.go b/atomicfile/atomicfile_windows.go new file mode 100644 index 000000000..c67762df2 --- /dev/null +++ b/atomicfile/atomicfile_windows.go @@ -0,0 +1,33 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package atomicfile + +import ( + "os" + + "golang.org/x/sys/windows" +) + +func rename(srcFile, destFile string) error { + // Use replaceFile when possible to preserve the original file's attributes and ACLs. + if err := replaceFile(destFile, srcFile); err == nil || err != windows.ERROR_FILE_NOT_FOUND { + return err + } + // destFile doesn't exist. Just do a normal rename. + return os.Rename(srcFile, destFile) +} + +func replaceFile(destFile, srcFile string) error { + destFile16, err := windows.UTF16PtrFromString(destFile) + if err != nil { + return err + } + + srcFile16, err := windows.UTF16PtrFromString(srcFile) + if err != nil { + return err + } + + return replaceFileW(destFile16, srcFile16, nil, 0, nil, nil) +} diff --git a/atomicfile/atomicfile_windows_test.go b/atomicfile/atomicfile_windows_test.go new file mode 100644 index 000000000..4dec1493e --- /dev/null +++ b/atomicfile/atomicfile_windows_test.go @@ -0,0 +1,146 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package atomicfile + +import ( + "os" + "testing" + "unsafe" + + "golang.org/x/sys/windows" +) + +var _SECURITY_RESOURCE_MANAGER_AUTHORITY = windows.SidIdentifierAuthority{[6]byte{0, 0, 0, 0, 0, 9}} + +// makeRandomSID generates a SID derived from a v4 GUID. +// This is basically the same algorithm used by browser sandboxes for generating +// random SIDs. +func makeRandomSID() (*windows.SID, error) { + guid, err := windows.GenerateGUID() + if err != nil { + return nil, err + } + + rids := *((*[4]uint32)(unsafe.Pointer(&guid))) + + var pSID *windows.SID + if err := windows.AllocateAndInitializeSid(&_SECURITY_RESOURCE_MANAGER_AUTHORITY, 4, rids[0], rids[1], rids[2], rids[3], 0, 0, 0, 0, &pSID); err != nil { + return nil, err + } + defer windows.FreeSid(pSID) + + // Make a copy that lives on the Go heap + return pSID.Copy() +} + +func getExistingFileSD(name string) (*windows.SECURITY_DESCRIPTOR, error) { + const infoFlags = windows.DACL_SECURITY_INFORMATION + return windows.GetNamedSecurityInfo(name, windows.SE_FILE_OBJECT, infoFlags) +} + +func getExistingFileDACL(name string) (*windows.ACL, error) { + sd, err := getExistingFileSD(name) + if err != nil { + return nil, err + } + + dacl, _, err := sd.DACL() + return dacl, err +} + +func addDenyACEForRandomSID(dacl *windows.ACL) (*windows.ACL, error) { + randomSID, err := makeRandomSID() + if err != nil { + return nil, err + } + + randomSIDTrustee := windows.TRUSTEE{nil, windows.NO_MULTIPLE_TRUSTEE, + windows.TRUSTEE_IS_SID, windows.TRUSTEE_IS_UNKNOWN, + windows.TrusteeValueFromSID(randomSID)} + + entries := []windows.EXPLICIT_ACCESS{ + { + windows.GENERIC_ALL, + windows.DENY_ACCESS, + windows.NO_INHERITANCE, + randomSIDTrustee, + }, + } + + return windows.ACLFromEntries(entries, dacl) +} + +func setExistingFileDACL(name string, dacl *windows.ACL) error { + return windows.SetNamedSecurityInfo(name, windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION, nil, nil, dacl, nil) +} + +// makeOrigFileWithCustomDACL creates a new, temporary file with a custom +// DACL that we can check for later. It returns the name of the temporary +// file and the security descriptor for the file in SDDL format. +func makeOrigFileWithCustomDACL() (name, sddl string, err error) { + f, err := os.CreateTemp("", "foo*.tmp") + if err != nil { + return "", "", err + } + name = f.Name() + if err := f.Close(); err != nil { + return "", "", err + } + f = nil + defer func() { + if err != nil { + os.Remove(name) + } + }() + + dacl, err := getExistingFileDACL(name) + if err != nil { + return "", "", err + } + + // Add a harmless, deny-only ACE for a random SID that isn't used for anything + // (but that we can check for later). + dacl, err = addDenyACEForRandomSID(dacl) + if err != nil { + return "", "", err + } + + if err := setExistingFileDACL(name, dacl); err != nil { + return "", "", err + } + + sd, err := getExistingFileSD(name) + if err != nil { + return "", "", err + } + + return name, sd.String(), nil +} + +func TestPreserveSecurityInfo(t *testing.T) { + // Make a test file with a custom ACL. + origFileName, want, err := makeOrigFileWithCustomDACL() + if err != nil { + t.Fatalf("makeOrigFileWithCustomDACL returned %v", err) + } + t.Cleanup(func() { + os.Remove(origFileName) + }) + + if err := WriteFile(origFileName, []byte{}, 0); err != nil { + t.Fatalf("WriteFile returned %v", err) + } + + // We expect origFileName's security descriptor to be unchanged despite + // the WriteFile call. + sd, err := getExistingFileSD(origFileName) + if err != nil { + t.Fatalf("getExistingFileSD(%q) returned %v", origFileName, err) + } + + if got := sd.String(); got != want { + t.Errorf("security descriptor comparison failed: got %q, want %q", got, want) + } +} diff --git a/atomicfile/mksyscall.go b/atomicfile/mksyscall.go new file mode 100644 index 000000000..d8951a77c --- /dev/null +++ b/atomicfile/mksyscall.go @@ -0,0 +1,8 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package atomicfile + +//go:generate go run golang.org/x/sys/windows/mkwinsyscall -output zsyscall_windows.go mksyscall.go + +//sys replaceFileW(replaced *uint16, replacement *uint16, backup *uint16, flags uint32, exclude unsafe.Pointer, reserved unsafe.Pointer) (err error) [int32(failretval)==0] = kernel32.ReplaceFileW diff --git a/atomicfile/zsyscall_windows.go b/atomicfile/zsyscall_windows.go new file mode 100644 index 000000000..f2f0b6d08 --- /dev/null +++ b/atomicfile/zsyscall_windows.go @@ -0,0 +1,52 @@ +// Code generated by 'go generate'; DO NOT EDIT. + +package atomicfile + +import ( + "syscall" + "unsafe" + + "golang.org/x/sys/windows" +) + +var _ unsafe.Pointer + +// Do the interface allocations only once for common +// Errno values. +const ( + errnoERROR_IO_PENDING = 997 +) + +var ( + errERROR_IO_PENDING error = syscall.Errno(errnoERROR_IO_PENDING) + errERROR_EINVAL error = syscall.EINVAL +) + +// errnoErr returns common boxed Errno values, to prevent +// allocations at runtime. +func errnoErr(e syscall.Errno) error { + switch e { + case 0: + return errERROR_EINVAL + case errnoERROR_IO_PENDING: + return errERROR_IO_PENDING + } + // TODO: add more here, after collecting data on the common + // error values see on Windows. (perhaps when running + // all.bat?) + return e +} + +var ( + modkernel32 = windows.NewLazySystemDLL("kernel32.dll") + + procReplaceFileW = modkernel32.NewProc("ReplaceFileW") +) + +func replaceFileW(replaced *uint16, replacement *uint16, backup *uint16, flags uint32, exclude unsafe.Pointer, reserved unsafe.Pointer) (err error) { + r1, _, e1 := syscall.Syscall6(procReplaceFileW.Addr(), 6, uintptr(unsafe.Pointer(replaced)), uintptr(unsafe.Pointer(replacement)), uintptr(unsafe.Pointer(backup)), uintptr(flags), uintptr(exclude), uintptr(reserved)) + if int32(r1) == 0 { + err = errnoErr(e1) + } + return +} diff --git a/cmd/derper/depaware.txt b/cmd/derper/depaware.txt index d4b406d9d..729122d79 100644 --- a/cmd/derper/depaware.txt +++ b/cmd/derper/depaware.txt @@ -85,7 +85,7 @@ tailscale.com/cmd/derper dependencies: (generated by github.com/tailscale/depawa google.golang.org/protobuf/runtime/protoimpl from github.com/prometheus/client_model/go+ google.golang.org/protobuf/types/known/timestamppb from github.com/prometheus/client_golang/prometheus+ tailscale.com from tailscale.com/version - tailscale.com/atomicfile from tailscale.com/cmd/derper+ + 💣 tailscale.com/atomicfile from tailscale.com/cmd/derper+ tailscale.com/client/tailscale from tailscale.com/derp tailscale.com/client/tailscale/apitype from tailscale.com/client/tailscale tailscale.com/derp from tailscale.com/cmd/derper+ diff --git a/cmd/k8s-operator/depaware.txt b/cmd/k8s-operator/depaware.txt index f757cda18..cb02038e3 100644 --- a/cmd/k8s-operator/depaware.txt +++ b/cmd/k8s-operator/depaware.txt @@ -643,7 +643,7 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/ sigs.k8s.io/yaml/goyaml.v2 from sigs.k8s.io/yaml tailscale.com from tailscale.com/version tailscale.com/appc from tailscale.com/ipn/ipnlocal - tailscale.com/atomicfile from tailscale.com/ipn+ + 💣 tailscale.com/atomicfile from tailscale.com/ipn+ tailscale.com/client/tailscale from tailscale.com/client/web+ tailscale.com/client/tailscale/apitype from tailscale.com/client/tailscale+ tailscale.com/client/web from tailscale.com/ipn/ipnlocal diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index e894e0674..9ccd6eebd 100644 --- a/cmd/tailscale/depaware.txt +++ b/cmd/tailscale/depaware.txt @@ -69,7 +69,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep software.sslmate.com/src/go-pkcs12 from tailscale.com/cmd/tailscale/cli software.sslmate.com/src/go-pkcs12/internal/rc2 from software.sslmate.com/src/go-pkcs12 tailscale.com from tailscale.com/version - tailscale.com/atomicfile from tailscale.com/cmd/tailscale/cli+ + 💣 tailscale.com/atomicfile from tailscale.com/cmd/tailscale/cli+ tailscale.com/client/tailscale from tailscale.com/client/web+ tailscale.com/client/tailscale/apitype from tailscale.com/client/tailscale+ tailscale.com/client/web from tailscale.com/cmd/tailscale/cli diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 19254b616..8af347319 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -232,7 +232,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de gvisor.dev/gvisor/pkg/waiter from gvisor.dev/gvisor/pkg/context+ tailscale.com from tailscale.com/version tailscale.com/appc from tailscale.com/ipn/ipnlocal - tailscale.com/atomicfile from tailscale.com/ipn+ + 💣 tailscale.com/atomicfile from tailscale.com/ipn+ LD tailscale.com/chirp from tailscale.com/cmd/tailscaled tailscale.com/client/tailscale from tailscale.com/client/web+ tailscale.com/client/tailscale/apitype from tailscale.com/client/tailscale+