From 08c6945d612ac48c9e38dd56d6e5677c99deca68 Mon Sep 17 00:00:00 2001 From: aneesh-n <99904+aneesh-n@users.noreply.github.com> Date: Mon, 29 Apr 2024 16:21:38 -0600 Subject: [PATCH] Fix review comments --- changelog/unreleased/pull-4708 | 9 ++- doc/040_backup.rst | 4 +- doc/050_restore.rst | 4 +- internal/debug/debug.go | 4 +- internal/fs/sd_windows.go | 90 ++++++++------------------ internal/fs/sd_windows_test.go | 4 +- internal/fs/sd_windows_test_helpers.go | 17 +++++ internal/restic/node.go | 2 +- internal/restic/node_windows.go | 2 +- 9 files changed, 56 insertions(+), 80 deletions(-) diff --git a/changelog/unreleased/pull-4708 b/changelog/unreleased/pull-4708 index 2c666c300..5c5d426b5 100644 --- a/changelog/unreleased/pull-4708 +++ b/changelog/unreleased/pull-4708 @@ -1,10 +1,9 @@ Enhancement: Back up and restore SecurityDescriptors on Windows -Restic did not back up SecurityDescriptors of files on Windows. -Restic now backs up and restores SecurityDescriptors (which includes owner, group, -discretionary access control list (DACL), system access control list (SACL)) -when backing up files and folders on Windows. This requires the user to be -a member of backup operators or the application must be run as admin. +Restic now backs up and restores SecurityDescriptors when backing up files and folders +on Windows which includes owner, group, discretionary access control list (DACL), +system access control list (SACL). This requires the user to be a member of backup +operators or the application must be run as admin. If that is not the case, only the current user's owner, group and DACL will be backed up and during restore only the DACL of the backed file will be restored while the current user's owner and group will be set during the restore. diff --git a/doc/040_backup.rst b/doc/040_backup.rst index 3a332ca75..e125d2c65 100644 --- a/doc/040_backup.rst +++ b/doc/040_backup.rst @@ -514,9 +514,9 @@ written, and the next backup needs to write new metadata again. If you really want to save the access time for files and directories, you can pass the ``--with-atime`` option to the ``backup`` command. -Backing up full security descriptors on windows is only possible when the user +Backing up full security descriptors on Windows is only possible when the user has ``SeBackupPrivilege``privilege or is running as admin. This is a restriction -of windows not restic. +of Windows not restic. If either of these conditions are not met, only the owner, group and DACL will be backed up. diff --git a/doc/050_restore.rst b/doc/050_restore.rst index 5ab0286f1..193a00870 100644 --- a/doc/050_restore.rst +++ b/doc/050_restore.rst @@ -72,9 +72,9 @@ Restoring symbolic links on windows is only possible when the user has ``SeCreateSymbolicLinkPrivilege`` privilege or is running as admin. This is a restriction of windows not restic. -Restoring full security descriptors on windows is only possible when the user has +Restoring full security descriptors on Windows is only possible when the user has ``SeRestorePrivilege``, ``SeSecurityPrivilege`` and ``SeTakeOwnershipPrivilege`` -privilege or is running as admin. This is a restriction of windows not restic. +privilege or is running as admin. This is a restriction of Windows not restic. If either of these conditions are not met, only the DACL will be restored. By default, restic does not restore files as sparse. Use ``restore --sparse`` to diff --git a/internal/debug/debug.go b/internal/debug/debug.go index 62c145e1a..7bc3291d1 100644 --- a/internal/debug/debug.go +++ b/internal/debug/debug.go @@ -8,8 +8,6 @@ import ( "path/filepath" "runtime" "strings" - - "github.com/restic/restic/internal/fs" ) var opts struct { @@ -46,7 +44,7 @@ func initDebugLogger() { fmt.Fprintf(os.Stderr, "debug log file %v\n", debugfile) - f, err := fs.OpenFile(debugfile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) + f, err := os.OpenFile(debugfile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) if err != nil { fmt.Fprintf(os.Stderr, "unable to open debug log file: %v\n", err) os.Exit(2) diff --git a/internal/fs/sd_windows.go b/internal/fs/sd_windows.go index 9d53b3974..d7f2152b1 100644 --- a/internal/fs/sd_windows.go +++ b/internal/fs/sd_windows.go @@ -9,7 +9,7 @@ import ( "unicode/utf16" "unsafe" - "github.com/restic/restic/internal/errors" + "github.com/restic/restic/internal/debug" "golang.org/x/sys/windows" ) @@ -26,9 +26,7 @@ var ( // SeTakeOwnershipPrivilege allows the application to take ownership of files and directories, regardless of the permissions set on them. SeTakeOwnershipPrivilege = "SeTakeOwnershipPrivilege" - backupPrivilegeError error - restorePrivilegeError error - lowerPrivileges bool + lowerPrivileges bool ) // Flags for backup and restore with admin permissions @@ -49,14 +47,14 @@ func GetSecurityDescriptor(filePath string) (securityDescriptor *[]byte, err err var sd *windows.SECURITY_DESCRIPTOR if lowerPrivileges { - sd, err = getNamedSecurityInfoLow(sd, err, filePath) + sd, err = getNamedSecurityInfoLow(filePath) } else { - sd, err = getNamedSecurityInfoHigh(sd, err, filePath) + sd, err = getNamedSecurityInfoHigh(filePath) } if err != nil { if isHandlePrivilegeNotHeldError(err) { lowerPrivileges = true - sd, err = getNamedSecurityInfoLow(sd, err, filePath) + sd, err = getNamedSecurityInfoLow(filePath) if err != nil { return nil, fmt.Errorf("get low-level named security info failed with: %w", err) } @@ -128,12 +126,12 @@ func SetSecurityDescriptor(filePath string, securityDescriptor *[]byte) error { } // getNamedSecurityInfoHigh gets the higher level SecurityDescriptor which requires admin permissions. -func getNamedSecurityInfoHigh(sd *windows.SECURITY_DESCRIPTOR, err error, filePath string) (*windows.SECURITY_DESCRIPTOR, error) { +func getNamedSecurityInfoHigh(filePath string) (*windows.SECURITY_DESCRIPTOR, error) { return windows.GetNamedSecurityInfo(filePath, windows.SE_FILE_OBJECT, highSecurityFlags) } // getNamedSecurityInfoLow gets the lower level SecurityDescriptor which requires no admin permissions. -func getNamedSecurityInfoLow(sd *windows.SECURITY_DESCRIPTOR, err error, filePath string) (*windows.SECURITY_DESCRIPTOR, error) { +func getNamedSecurityInfoLow(filePath string) (*windows.SECURITY_DESCRIPTOR, error) { return windows.GetNamedSecurityInfo(filePath, windows.SE_FILE_OBJECT, lowBackupSecurityFlags) } @@ -151,7 +149,7 @@ func setNamedSecurityInfoLow(filePath string, dacl *windows.ACL) error { func enableBackupPrivilege() { err := enableProcessPrivileges([]string{SeBackupPrivilege}) if err != nil { - backupPrivilegeError = fmt.Errorf("error enabling backup privilege: %w", err) + debug.Log("error enabling backup privilege: %v", err) } } @@ -159,26 +157,10 @@ func enableBackupPrivilege() { func enableRestorePrivilege() { err := enableProcessPrivileges([]string{SeRestorePrivilege, SeSecurityPrivilege, SeTakeOwnershipPrivilege}) if err != nil { - restorePrivilegeError = fmt.Errorf("error enabling restore/security privilege: %w", err) + debug.Log("error enabling restore/security privilege: %v", err) } } -// DisableBackupPrivileges disables privileges that are needed for backup operations. -// They may be reenabled if GetSecurityDescriptor is called again. -func DisableBackupPrivileges() error { - //Reset the once so that backup privileges can be enabled again if needed. - onceBackup = sync.Once{} - return enableDisableProcessPrivilege([]string{SeBackupPrivilege}, 0) -} - -// DisableRestorePrivileges disables privileges that are needed for restore operations. -// They may be reenabled if SetSecurityDescriptor is called again. -func DisableRestorePrivileges() error { - //Reset the once so that restore privileges can be enabled again if needed. - onceRestore = sync.Once{} - return enableDisableProcessPrivilege([]string{SeRestorePrivilege, SeSecurityPrivilege}, 0) -} - // 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 @@ -189,23 +171,26 @@ func isHandlePrivilegeNotHeldError(err error) bool { return false } -// IsAdmin checks if current user is an administrator. -func IsAdmin() (isAdmin bool, err error) { - var sid *windows.SID - err = windows.AllocateAndInitializeSid(&windows.SECURITY_NT_AUTHORITY, 2, windows.SECURITY_BUILTIN_DOMAIN_RID, windows.DOMAIN_ALIAS_RID_ADMINS, - 0, 0, 0, 0, 0, 0, &sid) - if err != nil { - return false, errors.Errorf("sid error: %s", err) +// SecurityDescriptorBytesToStruct converts the security descriptor bytes representation +// into a pointer to windows SECURITY_DESCRIPTOR. +func SecurityDescriptorBytesToStruct(sd []byte) (*windows.SECURITY_DESCRIPTOR, error) { + if l := int(unsafe.Sizeof(windows.SECURITY_DESCRIPTOR{})); len(sd) < l { + return nil, fmt.Errorf("securityDescriptor (%d) smaller than expected (%d): %w", len(sd), l, windows.ERROR_INCORRECT_SIZE) } - token := windows.Token(0) - member, err := token.IsMember(sid) - if err != nil { - return false, errors.Errorf("token membership error: %s", err) - } - return member, nil + s := (*windows.SECURITY_DESCRIPTOR)(unsafe.Pointer(&sd[0])) + return s, nil } -// The code below was adapted from github.com/Microsoft/go-winio under MIT license. +// securityDescriptorStructToBytes converts the pointer to windows SECURITY_DESCRIPTOR +// into a security descriptor bytes representation. +func securityDescriptorStructToBytes(sd *windows.SECURITY_DESCRIPTOR) ([]byte, error) { + b := unsafe.Slice((*byte)(unsafe.Pointer(sd)), sd.Length()) + return b, nil +} + +// The code below was adapted from +// https://github.com/microsoft/go-winio/blob/3c9576c9346a1892dee136329e7e15309e82fb4f/privilege.go +// under MIT license. // The MIT License (MIT) @@ -262,23 +247,6 @@ type PrivilegeError struct { privileges []uint64 } -// SecurityDescriptorBytesToStruct converts the security descriptor bytes representation -// into a pointer to windows SECURITY_DESCRIPTOR. -func SecurityDescriptorBytesToStruct(sd []byte) (*windows.SECURITY_DESCRIPTOR, error) { - if l := int(unsafe.Sizeof(windows.SECURITY_DESCRIPTOR{})); len(sd) < l { - return nil, fmt.Errorf("securityDescriptor (%d) smaller than expected (%d): %w", len(sd), l, windows.ERROR_INCORRECT_SIZE) - } - s := (*windows.SECURITY_DESCRIPTOR)(unsafe.Pointer(&sd[0])) - return s, nil -} - -// securityDescriptorStructToBytes converts the pointer to windows SECURITY_DESCRIPTOR -// into a security descriptor bytes representation. -func securityDescriptorStructToBytes(sd *windows.SECURITY_DESCRIPTOR) ([]byte, error) { - b := unsafe.Slice((*byte)(unsafe.Pointer(sd)), sd.Length()) - return b, nil -} - // Error returns the string message for the error. func (e *PrivilegeError) Error() string { s := "Could not enable privilege " @@ -293,12 +261,6 @@ func (e *PrivilegeError) Error() string { s += getPrivilegeName(p) s += `"` } - if backupPrivilegeError != nil { - s += " backupPrivilegeError:" + backupPrivilegeError.Error() - } - if restorePrivilegeError != nil { - s += " restorePrivilegeError:" + restorePrivilegeError.Error() - } return s } diff --git a/internal/fs/sd_windows_test.go b/internal/fs/sd_windows_test.go index e4e37cb4a..e78241ed3 100644 --- a/internal/fs/sd_windows_test.go +++ b/internal/fs/sd_windows_test.go @@ -13,7 +13,7 @@ import ( "github.com/restic/restic/internal/test" ) -func Test_SetGetFileSecurityDescriptors(t *testing.T) { +func TestSetGetFileSecurityDescriptors(t *testing.T) { tempDir := t.TempDir() testfilePath := filepath.Join(tempDir, "testfile.txt") // create temp file @@ -31,7 +31,7 @@ func Test_SetGetFileSecurityDescriptors(t *testing.T) { testSecurityDescriptors(t, TestFileSDs, testfilePath) } -func Test_SetGetFolderSecurityDescriptors(t *testing.T) { +func TestSetGetFolderSecurityDescriptors(t *testing.T) { tempDir := t.TempDir() testfolderPath := filepath.Join(tempDir, "testfolder") // create temp folder diff --git a/internal/fs/sd_windows_test_helpers.go b/internal/fs/sd_windows_test_helpers.go index 877408796..8b3be5fd7 100644 --- a/internal/fs/sd_windows_test_helpers.go +++ b/internal/fs/sd_windows_test_helpers.go @@ -23,6 +23,23 @@ var ( } ) +// IsAdmin checks if current user is an administrator. +func IsAdmin() (isAdmin bool, err error) { + var sid *windows.SID + err = windows.AllocateAndInitializeSid(&windows.SECURITY_NT_AUTHORITY, 2, windows.SECURITY_BUILTIN_DOMAIN_RID, windows.DOMAIN_ALIAS_RID_ADMINS, + 0, 0, 0, 0, 0, 0, &sid) + if err != nil { + return false, errors.Errorf("sid error: %s", err) + } + windows.GetCurrentProcessToken() + token := windows.Token(0) + member, err := token.IsMember(sid) + if err != nil { + return false, errors.Errorf("token membership error: %s", err) + } + return member, nil +} + // CompareSecurityDescriptors runs tests for comparing 2 security descriptors in []byte format. func CompareSecurityDescriptors(t *testing.T, testPath string, sdInputBytes, sdOutputBytes []byte) { sdInput, err := SecurityDescriptorBytesToStruct(sdInputBytes) diff --git a/internal/restic/node.go b/internal/restic/node.go index 1e7e5d68e..807ee0c0f 100644 --- a/internal/restic/node.go +++ b/internal/restic/node.go @@ -721,7 +721,7 @@ func (node *Node) fillExtra(path string, fi os.FileInfo, ignoreXattrListError bo allowExtended, err := node.fillGenericAttributes(path, fi, stat) if allowExtended { // Skip processing ExtendedAttributes if allowExtended is false. - err = errors.CombineErrors(err, node.fillExtendedAttributes(path)) + err = errors.CombineErrors(err, node.fillExtendedAttributes(path, ignoreXattrListError)) } return err } diff --git a/internal/restic/node_windows.go b/internal/restic/node_windows.go index 043a05091..0c6d3775e 100644 --- a/internal/restic/node_windows.go +++ b/internal/restic/node_windows.go @@ -24,7 +24,7 @@ type WindowsAttributes struct { // FileAttributes is used for storing file attributes for windows files. FileAttributes *uint32 `generic:"file_attributes"` // SecurityDescriptor is used for storing security descriptors which includes - // owner, group, discretionary access control list (DACL), system access control list (SACL)) + // owner, group, discretionary access control list (DACL), system access control list (SACL) SecurityDescriptor *[]byte `generic:"security_descriptor"` }