From 01bf8977e79b1fc2b95625c44f4c963e74365c97 Mon Sep 17 00:00:00 2001 From: crazycatz00 Date: Wed, 18 Jun 2025 20:32:59 -0400 Subject: [PATCH 1/3] fs: Use backup privileges when reading extended attributes for files too. --- internal/fs/file_windows.go | 5 +---- internal/fs/node_windows_test.go | 4 +--- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/internal/fs/file_windows.go b/internal/fs/file_windows.go index f1ff733fa..2aa050d2c 100644 --- a/internal/fs/file_windows.go +++ b/internal/fs/file_windows.go @@ -123,10 +123,7 @@ func openHandleForEA(nodeType data.NodeType, path string, writeAccess bool) (han } switch nodeType { - case data.NodeTypeFile: - utf16Path := windows.StringToUTF16Ptr(path) - handle, err = windows.CreateFile(utf16Path, uint32(fileAccess), 0, nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL, 0) - case data.NodeTypeDir: + case data.NodeTypeFile, data.NodeTypeDir: utf16Path := windows.StringToUTF16Ptr(path) handle, err = windows.CreateFile(utf16Path, uint32(fileAccess), 0, nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_BACKUP_SEMANTICS, 0) default: diff --git a/internal/fs/node_windows_test.go b/internal/fs/node_windows_test.go index 426a074d5..fcfa3ed37 100644 --- a/internal/fs/node_windows_test.go +++ b/internal/fs/node_windows_test.go @@ -302,9 +302,7 @@ func TestRestoreExtendedAttributes(t *testing.T) { var handle windows.Handle var err error utf16Path := windows.StringToUTF16Ptr(testPath) - if node.Type == data.NodeTypeFile { - handle, err = windows.CreateFile(utf16Path, windows.FILE_READ_EA, 0, nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL, 0) - } else if node.Type == data.NodeTypeDir { + if node.Type == data.NodeTypeFile || node.Type == data.NodeTypeDir { handle, err = windows.CreateFile(utf16Path, windows.FILE_READ_EA, 0, nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_BACKUP_SEMANTICS, 0) } test.OK(t, errors.Wrapf(err, "Error opening file/directory for: %s", testPath)) From d14823eb817e3700552b500a9ea3ae5a6534b4b7 Mon Sep 17 00:00:00 2001 From: crazycatz00 Date: Fri, 7 Nov 2025 18:37:45 -0500 Subject: [PATCH 2/3] fs: Attempt to enable file system privileges on initialization. Add tests to verify privileges' effects. --- changelog/unreleased/pull-5424 | 11 ++++++ internal/fs/fs_local.go | 7 ++++ internal/fs/priv.go | 9 +++++ internal/fs/priv_windows.go | 33 +++++++++++++++++ internal/fs/priv_windows_test.go | 61 ++++++++++++++++++++++++++++++++ internal/fs/sd_windows.go | 42 +--------------------- 6 files changed, 122 insertions(+), 41 deletions(-) create mode 100644 changelog/unreleased/pull-5424 create mode 100644 internal/fs/priv.go create mode 100644 internal/fs/priv_windows.go create mode 100644 internal/fs/priv_windows_test.go diff --git a/changelog/unreleased/pull-5424 b/changelog/unreleased/pull-5424 new file mode 100644 index 000000000..044095914 --- /dev/null +++ b/changelog/unreleased/pull-5424 @@ -0,0 +1,11 @@ +Enhancement: Enable file system privileges before access + +Restic attempted to enable (Windows) file system privileges when +reading or writing security descriptors - after potentially being wholly +denied access to previous items. It also read file extended attributes without +using the privilege, possibly missing them and producing errors. + +Restic now attempts to enable all file system privileges before any file +access. It also requests extended attribute reads use the backup privilege. + +https://github.com/restic/restic/pull/5424 diff --git a/internal/fs/fs_local.go b/internal/fs/fs_local.go index 9d9ba9035..5b2f192fd 100644 --- a/internal/fs/fs_local.go +++ b/internal/fs/fs_local.go @@ -5,8 +5,15 @@ import ( "path/filepath" "github.com/restic/restic/internal/data" + "github.com/restic/restic/internal/debug" ) +func init() { + if err := enableProcessPrivileges(); err != nil { + debug.Log("error enabling privileges: %v", err) + } +} + // Local is the local file system. Most methods are just passed on to the stdlib. type Local struct{} diff --git a/internal/fs/priv.go b/internal/fs/priv.go new file mode 100644 index 000000000..fc0089827 --- /dev/null +++ b/internal/fs/priv.go @@ -0,0 +1,9 @@ +//go:build !windows +// +build !windows + +package fs + +// enableProcessPrivileges enables additional file system privileges for the current process. +func enableProcessPrivileges() error { + return nil +} diff --git a/internal/fs/priv_windows.go b/internal/fs/priv_windows.go new file mode 100644 index 000000000..5268951e2 --- /dev/null +++ b/internal/fs/priv_windows.go @@ -0,0 +1,33 @@ +//go:build windows +// +build windows + +package fs + +import ( + "github.com/Microsoft/go-winio" + "github.com/restic/restic/internal/errors" +) + +var processPrivileges = []string{ + // seBackupPrivilege allows the application to bypass file and directory ACLs to back up files and directories. + "SeBackupPrivilege", + // seRestorePrivilege allows the application to bypass file and directory ACLs to restore files and directories. + "SeRestorePrivilege", + // seSecurityPrivilege allows read and write access to all SACLs. + "SeSecurityPrivilege", + // seTakeOwnershipPrivilege allows the application to take ownership of files and directories, regardless of the permissions set on them. + "SeTakeOwnershipPrivilege", +} + +// enableProcessPrivileges enables additional file system privileges for the current process. +func enableProcessPrivileges() error { + var errs []error + + // EnableProcessPrivileges may enable some but not all requested privileges, yet its error lists all requested. + // Request one at a time to return what actually fails. + for _, p := range processPrivileges { + errs = append(errs, winio.EnableProcessPrivileges([]string{p})) + } + + return errors.Join(errs...) +} diff --git a/internal/fs/priv_windows_test.go b/internal/fs/priv_windows_test.go new file mode 100644 index 000000000..01e94c8b2 --- /dev/null +++ b/internal/fs/priv_windows_test.go @@ -0,0 +1,61 @@ +//go:build windows +// +build windows + +package fs + +import ( + "os" + "path/filepath" + "testing" + + "github.com/restic/restic/internal/errors" + "github.com/restic/restic/internal/test" + "golang.org/x/sys/windows" +) + +func TestBackupPrivilegeBypassACL(t *testing.T) { + testPath := testGetRestrictedFilePath(t) + + // Read-only Open/OpenFile should automatically use FILE_FLAG_BACKUP_SEMANTICS since Go v1.20. + testfile, err := os.Open(testPath) + test.OK(t, errors.Wrapf(err, "failed to open file for reading: %s", testPath)) + test.OK(t, testfile.Close()) +} + +func TestRestorePrivilegeBypassACL(t *testing.T) { + testPath := testGetRestrictedFilePath(t) + + // Writable OpenFile needs explicit FILE_FLAG_BACKUP_SEMANTICS. + // Go with issue #73676 merged would allow: os.OpenFile(testPath, os.O_WRONLY|windows.O_FILE_FLAG_BACKUP_SEMANTICS, 0) + utf16Path := windows.StringToUTF16Ptr(testPath) + handle, err := windows.CreateFile(utf16Path, windows.GENERIC_WRITE, 0, nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_BACKUP_SEMANTICS, 0) + test.OK(t, errors.Wrapf(err, "failed to open file for writing: %s", testPath)) + test.OK(t, windows.Close(handle)) +} + +func testGetRestrictedFilePath(t *testing.T) string { + // Non-admin is unlikely to have needed privileges. + isAdmin, err := isAdmin() + test.OK(t, errors.Wrap(err, "failed to check if user is admin")) + if !isAdmin { + t.Skip("not running with administrator access, skipping") + } + + // Create temporary file. + tempDir := t.TempDir() + testPath := filepath.Join(tempDir, "testfile.txt") + + testfile, err := os.Create(testPath) + test.OK(t, errors.Wrapf(err, "failed to create temporary file: %s", testPath)) + test.OK(t, testfile.Close()) + + // Set restricted permissions. + sd, err := windows.SecurityDescriptorFromString("D:PAI(D;;FRFWFX;;;WD)(A;;SD;;;WD)") + test.OK(t, errors.Wrap(err, "failed to parse SDDL: %s")) + dacl, _, err := sd.DACL() + test.OK(t, errors.Wrap(err, "failed to extract SD DACL")) + err = windows.SetNamedSecurityInfo(testPath, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, nil, nil, dacl, nil) + test.OK(t, errors.Wrapf(err, "failed to set SD: %s", testPath)) + + return testPath +} diff --git a/internal/fs/sd_windows.go b/internal/fs/sd_windows.go index 04623e8d3..1a1ee6d14 100644 --- a/internal/fs/sd_windows.go +++ b/internal/fs/sd_windows.go @@ -2,32 +2,15 @@ package fs import ( "fmt" - "sync" "sync/atomic" "syscall" "unsafe" - "github.com/Microsoft/go-winio" - "github.com/restic/restic/internal/debug" "github.com/restic/restic/internal/errors" "golang.org/x/sys/windows" ) -var ( - onceBackup sync.Once - onceRestore sync.Once - - // seBackupPrivilege allows the application to bypass file and directory ACLs to back up files and directories. - seBackupPrivilege = "SeBackupPrivilege" - // seRestorePrivilege allows the application to bypass file and directory ACLs to restore files and directories. - seRestorePrivilege = "SeRestorePrivilege" - // seSecurityPrivilege allows read and write access to all SACLs. - seSecurityPrivilege = "SeSecurityPrivilege" - // seTakeOwnershipPrivilege allows the application to take ownership of files and directories, regardless of the permissions set on them. - seTakeOwnershipPrivilege = "SeTakeOwnershipPrivilege" - - lowerPrivileges atomic.Bool -) +var lowerPrivileges atomic.Bool // Flags for backup and restore with admin permissions var highSecurityFlags windows.SECURITY_INFORMATION = windows.OWNER_SECURITY_INFORMATION | windows.GROUP_SECURITY_INFORMATION | windows.DACL_SECURITY_INFORMATION | windows.SACL_SECURITY_INFORMATION | windows.LABEL_SECURITY_INFORMATION | windows.ATTRIBUTE_SECURITY_INFORMATION | windows.SCOPE_SECURITY_INFORMATION | windows.BACKUP_SECURITY_INFORMATION | windows.PROTECTED_DACL_SECURITY_INFORMATION | windows.PROTECTED_SACL_SECURITY_INFORMATION | windows.UNPROTECTED_DACL_SECURITY_INFORMATION | windows.UNPROTECTED_SACL_SECURITY_INFORMATION @@ -42,8 +25,6 @@ var lowRestoreSecurityFlags windows.SECURITY_INFORMATION = windows.DACL_SECURITY // This needs admin permissions or SeBackupPrivilege for getting the full SD. // If there are no admin permissions, only the current user's owner, group and DACL will be got. func getSecurityDescriptor(filePath string) (securityDescriptor *[]byte, err error) { - onceBackup.Do(enableBackupPrivilege) - var sd *windows.SECURITY_DESCRIPTOR // store original value to avoid unrelated changes in the error check @@ -87,7 +68,6 @@ func getSecurityDescriptor(filePath string) (securityDescriptor *[]byte, err err // If there are no admin permissions/required privileges, only the DACL from the SD can be set and // owner and group will be set based on the current user. func setSecurityDescriptor(filePath string, securityDescriptor *[]byte) error { - onceRestore.Do(enableRestorePrivilege) // Set the security descriptor on the file sd, err := securityDescriptorBytesToStruct(*securityDescriptor) if err != nil { @@ -159,26 +139,6 @@ func setNamedSecurityInfoLow(filePath string, dacl *windows.ACL) error { return windows.SetNamedSecurityInfo(fixpath(filePath), windows.SE_FILE_OBJECT, lowRestoreSecurityFlags, nil, nil, dacl, nil) } -func enableProcessPrivileges(privileges []string) error { - return winio.EnableProcessPrivileges(privileges) -} - -// enableBackupPrivilege enables privilege for backing up security descriptors -func enableBackupPrivilege() { - err := enableProcessPrivileges([]string{seBackupPrivilege}) - if err != nil { - debug.Log("error enabling backup privilege: %v", err) - } -} - -// enableRestorePrivilege enables privilege for restoring security descriptors -func enableRestorePrivilege() { - err := enableProcessPrivileges([]string{seRestorePrivilege, seSecurityPrivilege, seTakeOwnershipPrivilege}) - if err != nil { - debug.Log("error enabling restore/security privilege: %v", err) - } -} - // isHandlePrivilegeNotHeldError checks if the error is ERROR_PRIVILEGE_NOT_HELD func isHandlePrivilegeNotHeldError(err error) bool { // Use a type assertion to check if the error is of type syscall.Errno From 3ab68d4d11f16a7a1e62253da14b1bd1214f5d23 Mon Sep 17 00:00:00 2001 From: crazycatz00 Date: Sun, 16 Nov 2025 11:48:29 -0500 Subject: [PATCH 3/3] fs: Clarified documentation --- changelog/unreleased/pull-5424 | 4 ++-- internal/fs/priv_windows_test.go | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/changelog/unreleased/pull-5424 b/changelog/unreleased/pull-5424 index 044095914..43fbe7fdf 100644 --- a/changelog/unreleased/pull-5424 +++ b/changelog/unreleased/pull-5424 @@ -1,6 +1,6 @@ -Enhancement: Enable file system privileges before access +Enhancement: Enable file system privileges on Windows before access -Restic attempted to enable (Windows) file system privileges when +Restic attempted to enable Windows file system privileges when reading or writing security descriptors - after potentially being wholly denied access to previous items. It also read file extended attributes without using the privilege, possibly missing them and producing errors. diff --git a/internal/fs/priv_windows_test.go b/internal/fs/priv_windows_test.go index 01e94c8b2..7851194a6 100644 --- a/internal/fs/priv_windows_test.go +++ b/internal/fs/priv_windows_test.go @@ -50,6 +50,7 @@ func testGetRestrictedFilePath(t *testing.T) string { test.OK(t, testfile.Close()) // Set restricted permissions. + // Deny file read/write/execute to "Everyone" (all accounts); allow delete to "Everyone". sd, err := windows.SecurityDescriptorFromString("D:PAI(D;;FRFWFX;;;WD)(A;;SD;;;WD)") test.OK(t, errors.Wrap(err, "failed to parse SDDL: %s")) dacl, _, err := sd.DACL()