diff --git a/changelog/unreleased/pull-5424 b/changelog/unreleased/pull-5424 new file mode 100644 index 000000000..43fbe7fdf --- /dev/null +++ b/changelog/unreleased/pull-5424 @@ -0,0 +1,11 @@ +Enhancement: Enable file system privileges on Windows 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/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/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/node_windows_test.go b/internal/fs/node_windows_test.go index d19736983..92d841d83 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)) 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..7851194a6 --- /dev/null +++ b/internal/fs/priv_windows_test.go @@ -0,0 +1,62 @@ +//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. + // 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() + 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