Fix: Correctly restore ACL inheritance state#5465
Conversation
MichaelEischer
left a comment
There was a problem hiding this comment.
Thanks! I just have two questions/suggestion below.
| // Remove all protection flags first to ensure a clean state before adding the correct one. | ||
| // The highSecurityFlags variable contains all of them, which is fine for GET operations, | ||
| // but for SET operations, we must specify only one of PROTECTED or UNPROTECTED. | ||
| securityInfo &^= (windows.PROTECTED_DACL_SECURITY_INFORMATION | windows.UNPROTECTED_DACL_SECURITY_INFORMATION) |
There was a problem hiding this comment.
lowRestoreSecurityFlags also contains PROTECTED_DACL_SECURITY_INFORMATION. Do we need similar handling there?
There was a problem hiding this comment.
Thanks. Made the fix
| 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 := highSecurityFlags |
There was a problem hiding this comment.
What about splitting the highSecurityFlags into a backup and restore variant? Then we don't need to drop the flags below. The highBackupSecurityFlags would then be based just add those flags to the highRestoreSecurityFlags.
There was a problem hiding this comment.
Thanks, makes sense. Updated
MichaelEischer
left a comment
There was a problem hiding this comment.
@aneesh-n can you take a look at the two comments above? Feel free to just close those comments if they aren't relevant.
f2466e3 to
8267f87
Compare
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.
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.
…store 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 restic#5427
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 restic#5427
8267f87 to
a620cf6
Compare
MichaelEischer
left a comment
There was a problem hiding this comment.
LGTM. Thanks. I've rebased the PR to fix the merge conflict near
var lowerPrivileges atomic.Bool
What does this PR change? What problem does it solve?
When restoring a file or directory on Windows, the
IsInheritedproperty of its Access Control Entries (ACEs) was always being set toFalse, even if the ACEs were inherited in the original backup.This was caused by the restore process calling the
SetNamedSecurityInfoAPI 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
Controlflags of the saved Security Descriptor during restore. Based on whether theSE_DACL_PROTECTEDflag is present, the code now adds the appropriatePROTECTED_DACL_SECURITY_INFORMATIONorUNPROTECTED_DACL_SECURITY_INFORMATIONflag to theSetNamedSecurityInfoAPI call.By providing this crucial inheritance context, the Windows API can now correctly reconstruct the ACL, ensuring the
IsInheritedstatus of each ACE is preserved as it was at the time of backup.Was the change previously discussed in an issue or on the forum?
Closes #5427
Checklist
changelog/unreleased/that describes the changes for our users (see template).