mirror of
https://github.com/restic/restic.git
synced 2025-12-08 06:09:56 +00:00
Fix: Correctly restore ACL inheritance state (#5465)
* Fix: Correctly restore ACL inheritance state When restoring a file or directory on Windows, the `IsInherited` property of its Access Control Entries (ACEs) was always being set to `False`, even if the ACEs were inherited in the original backup. This was caused by the restore process calling the `SetNamedSecurityInfo` API without providing context about the object's inheritance policy. By default, this API applies the provided Discretionary Access Control List (DACL) as an explicit set of permissions, thereby losing the original inheritance state. This commit fixes the issue by inspecting the `Control` flags of the saved Security Descriptor during restore. Based on whether the `SE_DACL_PROTECTED` flag is present, the code now adds the appropriate `PROTECTED_DACL_SECURITY_INFORMATION` or `UNPROTECTED_DACL_SECURITY_INFORMATION` flag to the `SetNamedSecurityInfo` API call. By providing this crucial inheritance context, the Windows API can now correctly reconstruct the ACL, ensuring the `IsInherited` status of each ACE is preserved as it was at the time of backup. * Fix: Correctly restore ACL inheritance flags This commit resolves an issue where the ACL inheritance state (`IsInherited` property) was not being correctly restored for files and directories on Windows. The root cause was that the `SECURITY_INFORMATION` flags used in the `SetNamedSecurityInfo` API call contained both the `PROTECTED_DACL_SECURITY_INFORMATION` and `UNPROTECTED_DACL_SECURITY_INFORMATION` flags simultaneously. When faced with this conflicting information, the Windows API defaulted to the more restrictive `PROTECTED` behavior, incorrectly disabling inheritance on restored items. The fix modifies the `setNamedSecurityInfoHigh` function to first clear all existing inheritance-related flags from the `securityInfo` bitmask. It then adds the single, correct flag (`PROTECTED` or `UNPROTECTED`) based on the `SE_DACL_PROTECTED` control bit from the original, saved Security Descriptor. This ensures that the API receives unambiguous instructions, allowing it to correctly preserve the inheritance state as it was at the time of backup. The accompanying test case for ACL inheritance now passes with this change. * Fix inheritance flag handling in low-privilege security descriptor restore When restoring files without admin privileges, the IsInherited property of Access Control Entries (ACEs) was not being preserved correctly. The low-privilege restore path (setNamedSecurityInfoLow) was using a static PROTECTED_DACL_SECURITY_INFORMATION flag, which always marked the restored DACL as explicitly set rather than inherited. This commit updates setNamedSecurityInfoLow to dynamically determine the correct inheritance flag based on the SE_DACL_PROTECTED control flag from the original security descriptor, matching the behavior of the high-privilege path (setNamedSecurityInfoHigh). Changes: - Update setNamedSecurityInfoLow to accept control flags parameter - Add logic to set either PROTECTED_DACL_SECURITY_INFORMATION or UNPROTECTED_DACL_SECURITY_INFORMATION based on the original SD - Add TestRestoreSecurityDescriptorInheritanceLowPrivilege to verify inheritance is correctly restored in low-privilege scenarios This ensures that both admin and non-admin restore operations correctly preserve the inheritance state of ACLs, maintaining the original permissions flow on child objects. Addresses review feedback on PR for issue #5427 * Refactor security flags into separate backup/restore variants Split highSecurityFlags into highBackupSecurityFlags and highRestoreSecurityFlags to avoid runtime bitwise operations. This makes the code cleaner and more maintainable by using appropriate flags for GET vs SET operations. Addresses review feedback on PR for issue #5427 --------- Co-authored-by: Aneesh Nireshwalia <anireshw@akamai.com>
This commit is contained in:
Aneesh N
GitHub
parent
ce57961f14
commit
b9afdf795e
3 changed files with 227 additions and 11 deletions
10
changelog/unreleased/pull-5465
Normal file
10
changelog/unreleased/pull-5465
Normal file
|
|
@ -0,0 +1,10 @@
|
|||
Bugfix: Correctly restore ACL inheritance state on Windows
|
||||
|
||||
Since the introduction of Security Descriptor backups in restic 0.17.0, the inheritance property of Access Control Entries (ACEs) was not restored correctly. This resulted in all restored permissions being marked as explicit (IsInherited: False), even if they were originally inherited from a parent folder.
|
||||
|
||||
The issue was caused by sending conflicting inheritance flags (PROTECTED_... and UNPROTECTED_...) to the Windows API during the restore process. The API would default to the more restrictive PROTECTED state, effectively disabling inheritance.
|
||||
|
||||
This has been fixed by ensuring that only the correct, non-conflicting inheritance flag is used when applying the security descriptor, preserving the original permission structure from the backup.
|
||||
|
||||
https://github.com/restic/restic/pull/5465
|
||||
https://github.com/restic/restic/issues/5427
|
||||
|
|
@ -56,6 +56,176 @@ func testRestoreSecurityDescriptor(t *testing.T, sd string, tempDir string, file
|
|||
compareSecurityDescriptors(t, testPath, *sdByteFromRestoredNode, *sdBytesFromRestoredPath)
|
||||
}
|
||||
|
||||
// TestRestoreSecurityDescriptorInheritance checks that the DACL protection (inheritance)
|
||||
// flags are correctly restored. This is the mechanism that preserves the `IsInherited`
|
||||
// property on individual ACEs.
|
||||
func TestRestoreSecurityDescriptorInheritance(t *testing.T) {
|
||||
// This test requires admin privileges to manipulate ACLs effectively.
|
||||
isAdmin, err := isAdmin()
|
||||
test.OK(t, err)
|
||||
if !isAdmin {
|
||||
t.Skip("Skipping inheritance test, requires admin privileges")
|
||||
}
|
||||
|
||||
tempDir := t.TempDir()
|
||||
|
||||
// 1. Create a parent/child directory structure.
|
||||
parentDir := filepath.Join(tempDir, "parent")
|
||||
err = os.Mkdir(parentDir, 0755)
|
||||
test.OK(t, err)
|
||||
|
||||
childDir := filepath.Join(parentDir, "child")
|
||||
err = os.Mkdir(childDir, 0755)
|
||||
test.OK(t, err)
|
||||
|
||||
// 2. Set inheritable permissions on the parent.
|
||||
// We will give the "Users" group inheritable read access.
|
||||
users, err := windows.StringToSid("S-1-5-32-545") // BUILTIN\Users
|
||||
test.OK(t, err)
|
||||
|
||||
// Create an EXPLICIT_ACCESS structure for the new ACE.
|
||||
explicitAccess := windows.EXPLICIT_ACCESS{
|
||||
AccessPermissions: windows.GENERIC_READ,
|
||||
AccessMode: windows.GRANT_ACCESS,
|
||||
Inheritance: windows.OBJECT_INHERIT_ACE | windows.CONTAINER_INHERIT_ACE,
|
||||
Trustee: windows.TRUSTEE{
|
||||
TrusteeForm: windows.TRUSTEE_IS_SID,
|
||||
TrusteeType: windows.TRUSTEE_IS_GROUP,
|
||||
TrusteeValue: windows.TrusteeValueFromSID(users),
|
||||
},
|
||||
}
|
||||
|
||||
// Create a new DACL from the entry.
|
||||
dacl, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{explicitAccess}, nil)
|
||||
test.OK(t, err)
|
||||
|
||||
// Apply this new DACL to the parent, marking it as unprotected so it can be inherited.
|
||||
err = windows.SetNamedSecurityInfo(
|
||||
parentDir,
|
||||
windows.SE_FILE_OBJECT,
|
||||
windows.DACL_SECURITY_INFORMATION|windows.UNPROTECTED_DACL_SECURITY_INFORMATION,
|
||||
nil, nil, dacl, nil,
|
||||
)
|
||||
test.OK(t, errors.Wrapf(err, "failed to set inheritable ACL on parent dir"))
|
||||
|
||||
// 3. Get the Security Descriptor of the child, which should now have inherited the ACE.
|
||||
sdBytesOriginal, err := getSecurityDescriptor(childDir)
|
||||
test.OK(t, err)
|
||||
|
||||
// Sanity check: verify the original child SD is NOT protected from inheritance.
|
||||
sdOriginal, err := securityDescriptorBytesToStruct(*sdBytesOriginal)
|
||||
test.OK(t, err)
|
||||
control, _, err := sdOriginal.Control()
|
||||
test.OK(t, err)
|
||||
test.Assert(t, control&windows.SE_DACL_PROTECTED == 0, "Pre-condition failed: child directory should have inheritance enabled")
|
||||
|
||||
// 4. Create a restic node for the child directory.
|
||||
genericAttrs, err := data.WindowsAttrsToGenericAttributes(data.WindowsAttributes{SecurityDescriptor: sdBytesOriginal})
|
||||
test.OK(t, err)
|
||||
childNode := getNode("child-restored", "dir", genericAttrs)
|
||||
|
||||
// 5. Restore the node to a new location.
|
||||
restoreDir := filepath.Join(tempDir, "restore")
|
||||
err = os.Mkdir(restoreDir, 0755)
|
||||
test.OK(t, err)
|
||||
|
||||
restoredPath, _ := restoreAndGetNode(t, restoreDir, &childNode, false)
|
||||
|
||||
// 6. Get the Security Descriptor of the restored child directory.
|
||||
sdBytesRestored, err := getSecurityDescriptor(restoredPath)
|
||||
test.OK(t, err)
|
||||
|
||||
// 7. Compare the control flags of the original and restored SDs.
|
||||
sdRestored, err := securityDescriptorBytesToStruct(*sdBytesRestored)
|
||||
test.OK(t, err)
|
||||
controlRestored, _, err := sdRestored.Control()
|
||||
test.OK(t, err)
|
||||
|
||||
// The core of the test: Ensure the restored DACL protection flag matches the original.
|
||||
originalIsProtected := (control & windows.SE_DACL_PROTECTED) != 0
|
||||
restoredIsProtected := (controlRestored & windows.SE_DACL_PROTECTED) != 0
|
||||
test.Equals(t, originalIsProtected, restoredIsProtected, "DACL protection flag was not restored correctly. Inheritance state is wrong.")
|
||||
}
|
||||
|
||||
// TestRestoreSecurityDescriptorInheritanceLowPrivilege tests that the low-privilege restore
|
||||
// path (setNamedSecurityInfoLow) correctly handles inheritance flags. This test doesn't require
|
||||
// admin privileges and focuses on DACL restoration only.
|
||||
func TestRestoreSecurityDescriptorInheritanceLowPrivilege(t *testing.T) {
|
||||
tempDir := t.TempDir()
|
||||
|
||||
// 1. Create a test directory
|
||||
testDir := filepath.Join(tempDir, "testdir")
|
||||
err := os.Mkdir(testDir, 0755)
|
||||
test.OK(t, err)
|
||||
|
||||
// 2. Get its security descriptor (which will have some default ACL)
|
||||
sdBytesOriginal, err := getSecurityDescriptor(testDir)
|
||||
test.OK(t, err)
|
||||
|
||||
// Verify we can get the control flags
|
||||
sdOriginal, err := securityDescriptorBytesToStruct(*sdBytesOriginal)
|
||||
test.OK(t, err)
|
||||
controlOriginal, _, err := sdOriginal.Control()
|
||||
test.OK(t, err)
|
||||
|
||||
// 3. Test both protected and unprotected scenarios by modifying the control flags
|
||||
testCases := []struct {
|
||||
name string
|
||||
shouldBeProtected bool
|
||||
}{
|
||||
{"unprotected_inheritance_enabled", false},
|
||||
{"protected_inheritance_disabled", true},
|
||||
}
|
||||
|
||||
for _, tc := range testCases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
// Create a copy of the SD bytes to modify
|
||||
sdBytesTest := make([]byte, len(*sdBytesOriginal))
|
||||
copy(sdBytesTest, *sdBytesOriginal)
|
||||
|
||||
// Modify the control flags to simulate protected/unprotected DACL
|
||||
sdTest, err := securityDescriptorBytesToStruct(sdBytesTest)
|
||||
test.OK(t, err)
|
||||
|
||||
// Get the DACL from the test SD
|
||||
dacl, _, err := sdTest.DACL()
|
||||
test.OK(t, err)
|
||||
|
||||
// Determine which control flag to use based on test case
|
||||
var controlToUse windows.SECURITY_DESCRIPTOR_CONTROL
|
||||
if tc.shouldBeProtected {
|
||||
controlToUse = controlOriginal | windows.SE_DACL_PROTECTED
|
||||
} else {
|
||||
controlToUse = controlOriginal &^ windows.SE_DACL_PROTECTED
|
||||
}
|
||||
|
||||
// 4. Call setNamedSecurityInfoLow directly to test the low-privilege path
|
||||
restoreTarget := filepath.Join(tempDir, "restore_"+tc.name)
|
||||
err = os.Mkdir(restoreTarget, 0755)
|
||||
test.OK(t, err)
|
||||
|
||||
// This directly tests the low-privilege restore function
|
||||
err = setNamedSecurityInfoLow(restoreTarget, dacl, controlToUse)
|
||||
test.OK(t, err)
|
||||
|
||||
// 5. Get the security descriptor of the restored directory
|
||||
sdBytesRestored, err := getSecurityDescriptor(restoreTarget)
|
||||
test.OK(t, err)
|
||||
|
||||
// 6. Verify that the control flags were correctly restored
|
||||
sdRestored, err := securityDescriptorBytesToStruct(*sdBytesRestored)
|
||||
test.OK(t, err)
|
||||
controlRestored, _, err := sdRestored.Control()
|
||||
test.OK(t, err) // Check if the protection flag matches what we requested
|
||||
restoredIsProtected := (controlRestored & windows.SE_DACL_PROTECTED) != 0
|
||||
if tc.shouldBeProtected != restoredIsProtected {
|
||||
t.Errorf("DACL protection flag was not restored correctly in low-privilege path. Expected protected=%v, got protected=%v",
|
||||
tc.shouldBeProtected, restoredIsProtected)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func getNode(name string, fileType data.NodeType, genericAttributes map[data.GenericAttributeType]json.RawMessage) data.Node {
|
||||
return data.Node{
|
||||
Name: name,
|
||||
|
|
|
|||
|
|
@ -12,14 +12,17 @@ import (
|
|||
|
||||
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
|
||||
// Flags for backup with admin permissions. Includes protection flags for GET operations.
|
||||
var highBackupSecurityFlags 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
|
||||
|
||||
// Flags for restore with admin permissions. Base flags without protection flags for SET operations.
|
||||
var highRestoreSecurityFlags 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
|
||||
|
||||
// Flags for backup without admin permissions. If there are no admin permissions, only the current user's owner, group and DACL will be backed up.
|
||||
var lowBackupSecurityFlags windows.SECURITY_INFORMATION = windows.OWNER_SECURITY_INFORMATION | windows.GROUP_SECURITY_INFORMATION | windows.DACL_SECURITY_INFORMATION | windows.LABEL_SECURITY_INFORMATION | windows.ATTRIBUTE_SECURITY_INFORMATION | windows.SCOPE_SECURITY_INFORMATION | windows.PROTECTED_DACL_SECURITY_INFORMATION | windows.UNPROTECTED_DACL_SECURITY_INFORMATION
|
||||
|
||||
// Flags for restore without admin permissions. If there are no admin permissions, only the DACL from the SD can be restored and owner and group will be set based on the current user.
|
||||
var lowRestoreSecurityFlags windows.SECURITY_INFORMATION = windows.DACL_SECURITY_INFORMATION | windows.ATTRIBUTE_SECURITY_INFORMATION | windows.PROTECTED_DACL_SECURITY_INFORMATION
|
||||
var lowRestoreSecurityFlags windows.SECURITY_INFORMATION = windows.DACL_SECURITY_INFORMATION | windows.ATTRIBUTE_SECURITY_INFORMATION
|
||||
|
||||
// getSecurityDescriptor takes the path of the file and returns the SecurityDescriptor for the file.
|
||||
// This needs admin permissions or SeBackupPrivilege for getting the full SD.
|
||||
|
|
@ -95,15 +98,21 @@ func setSecurityDescriptor(filePath string, securityDescriptor *[]byte) error {
|
|||
sacl = nil
|
||||
}
|
||||
|
||||
// Get the control flags from the original security descriptor
|
||||
control, _, err := sd.Control()
|
||||
if err != nil {
|
||||
// This is unlikely to fail if the sd is valid, but handle it.
|
||||
return fmt.Errorf("could not get security descriptor control flags: %w", err)
|
||||
}
|
||||
// store original value to avoid unrelated changes in the error check
|
||||
useLowerPrivileges := lowerPrivileges.Load()
|
||||
if useLowerPrivileges {
|
||||
err = setNamedSecurityInfoLow(filePath, dacl)
|
||||
err = setNamedSecurityInfoLow(filePath, dacl, control)
|
||||
} else {
|
||||
err = setNamedSecurityInfoHigh(filePath, owner, group, dacl, sacl)
|
||||
err = setNamedSecurityInfoHigh(filePath, owner, group, dacl, sacl, control)
|
||||
// See corresponding fallback in getSecurityDescriptor for an explanation
|
||||
if err != nil && isAccessDeniedError(err) {
|
||||
err = setNamedSecurityInfoLow(filePath, dacl)
|
||||
err = setNamedSecurityInfoLow(filePath, dacl, control)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -121,7 +130,7 @@ func setSecurityDescriptor(filePath string, securityDescriptor *[]byte) error {
|
|||
|
||||
// getNamedSecurityInfoHigh gets the higher level SecurityDescriptor which requires admin permissions.
|
||||
func getNamedSecurityInfoHigh(filePath string) (*windows.SECURITY_DESCRIPTOR, error) {
|
||||
return windows.GetNamedSecurityInfo(fixpath(filePath), windows.SE_FILE_OBJECT, highSecurityFlags)
|
||||
return windows.GetNamedSecurityInfo(fixpath(filePath), windows.SE_FILE_OBJECT, highBackupSecurityFlags)
|
||||
}
|
||||
|
||||
// getNamedSecurityInfoLow gets the lower level SecurityDescriptor which requires no admin permissions.
|
||||
|
|
@ -130,13 +139,40 @@ func getNamedSecurityInfoLow(filePath string) (*windows.SECURITY_DESCRIPTOR, err
|
|||
}
|
||||
|
||||
// setNamedSecurityInfoHigh sets the higher level SecurityDescriptor which requires admin permissions.
|
||||
func setNamedSecurityInfoHigh(filePath string, owner *windows.SID, group *windows.SID, dacl *windows.ACL, sacl *windows.ACL) error {
|
||||
return windows.SetNamedSecurityInfo(fixpath(filePath), windows.SE_FILE_OBJECT, highSecurityFlags, owner, group, dacl, sacl)
|
||||
func setNamedSecurityInfoHigh(filePath string, owner *windows.SID, group *windows.SID, dacl *windows.ACL, sacl *windows.ACL, control windows.SECURITY_DESCRIPTOR_CONTROL) error {
|
||||
securityInfo := highRestoreSecurityFlags
|
||||
|
||||
// Check if the original DACL was protected from inheritance and add the correct flag.
|
||||
if control&windows.SE_DACL_PROTECTED != 0 {
|
||||
securityInfo |= windows.PROTECTED_DACL_SECURITY_INFORMATION
|
||||
} else {
|
||||
// Explicitly state that it is NOT protected. This ensures inheritance is re-enabled correctly.
|
||||
securityInfo |= windows.UNPROTECTED_DACL_SECURITY_INFORMATION
|
||||
}
|
||||
|
||||
// Do the same for the SACL for completeness.
|
||||
if control&windows.SE_SACL_PROTECTED != 0 {
|
||||
securityInfo |= windows.PROTECTED_SACL_SECURITY_INFORMATION
|
||||
} else {
|
||||
securityInfo |= windows.UNPROTECTED_SACL_SECURITY_INFORMATION
|
||||
}
|
||||
|
||||
return windows.SetNamedSecurityInfo(fixpath(filePath), windows.SE_FILE_OBJECT, securityInfo, owner, group, dacl, sacl)
|
||||
}
|
||||
|
||||
// setNamedSecurityInfoLow sets the lower level SecurityDescriptor which requires no admin permissions.
|
||||
func setNamedSecurityInfoLow(filePath string, dacl *windows.ACL) error {
|
||||
return windows.SetNamedSecurityInfo(fixpath(filePath), windows.SE_FILE_OBJECT, lowRestoreSecurityFlags, nil, nil, dacl, nil)
|
||||
func setNamedSecurityInfoLow(filePath string, dacl *windows.ACL, control windows.SECURITY_DESCRIPTOR_CONTROL) error {
|
||||
securityInfo := lowRestoreSecurityFlags
|
||||
|
||||
// Check if the original DACL was protected from inheritance and add the correct flag.
|
||||
if control&windows.SE_DACL_PROTECTED != 0 {
|
||||
securityInfo |= windows.PROTECTED_DACL_SECURITY_INFORMATION
|
||||
} else {
|
||||
// Explicitly state that it is NOT protected. This ensures inheritance is re-enabled correctly.
|
||||
securityInfo |= windows.UNPROTECTED_DACL_SECURITY_INFORMATION
|
||||
}
|
||||
|
||||
return windows.SetNamedSecurityInfo(fixpath(filePath), windows.SE_FILE_OBJECT, securityInfo, nil, nil, dacl, nil)
|
||||
}
|
||||
|
||||
// isHandlePrivilegeNotHeldError checks if the error is ERROR_PRIVILEGE_NOT_HELD
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue