Merge pull request #5424 from Crazycatz00/sebackup-fixes

Windows Backup Privilege Tweaks

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

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:

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{}
 

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))

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
+}

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...)
+}

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
+}

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