Skip to content

Fix: Correctly restore ACL inheritance state#5465

Merged
MichaelEischer merged 4 commits into
restic:masterfrom
aneesh-n:windows_sd_isinherited_fix
Nov 28, 2025
Merged

Fix: Correctly restore ACL inheritance state#5465
MichaelEischer merged 4 commits into
restic:masterfrom
aneesh-n:windows_sd_isinherited_fix

Conversation

@aneesh-n

@aneesh-n aneesh-n commented Aug 4, 2025

Copy link
Copy Markdown
Contributor

What does this PR change? What problem does it solve?

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.

Was the change previously discussed in an issue or on the forum?

Closes #5427

Checklist

  • I have added tests for all code changes.
  • I have added documentation for relevant changes (in the manual).
  • There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
  • I'm done! This pull request is ready for review.

@MichaelEischer MichaelEischer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I just have two questions/suggestion below.

Comment thread internal/fs/sd_windows.go Outdated
// 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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowRestoreSecurityFlags also contains PROTECTED_DACL_SECURITY_INFORMATION. Do we need similar handling there?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Made the fix

Comment thread internal/fs/sd_windows.go Outdated
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, makes sense. Updated

@MichaelEischer MichaelEischer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aneesh-n can you take a look at the two comments above? Feel free to just close those comments if they aren't relevant.

@aneesh-n aneesh-n force-pushed the windows_sd_isinherited_fix branch from f2466e3 to 8267f87 Compare November 16, 2025 20:02
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
@MichaelEischer MichaelEischer force-pushed the windows_sd_isinherited_fix branch from 8267f87 to a620cf6 Compare November 28, 2025 19:14

@MichaelEischer MichaelEischer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks. I've rebased the PR to fix the merge conflict near

var lowerPrivileges atomic.Bool

@MichaelEischer MichaelEischer enabled auto-merge (squash) November 28, 2025 19:16
@MichaelEischer MichaelEischer merged commit b9afdf7 into restic:master Nov 28, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Backup & Restore "IsInherited" Property on Windows

3 participants