From c7a03836449ea8e26f333d9a308393738dc39247 Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Fri, 9 Oct 2026 14:19:32 +0000 Subject: [PATCH] fix: skip setting back an owner that did not change Clear-NTFSAccess -DisableInheritance and Set-NTFSSecurityDescriptor took ownership of an item that the user owned already, left a DACL without the right to set an owner, and then reported a RestoreOwnerError for setting the same owner back. Skip the restore when the previous owner is the current user. The guards fail without the fix in all four configurations and also cover the owner that cannot be set back without the Restore privilege, the missing path of Get-ChildItem2, and the descriptor write that retries as owner. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- CHANGELOG.md | 5 +++ NTFSSecurity/BaseCmdlets.cs | 20 ++++++---- .../SetSecurityDescriptor.cs | 9 +++-- Tests/PathErrors.Tests.ps1 | 38 +++++++++++++++++++ Tests/SecurityDescriptor.Tests.ps1 | 38 +++++++++++++++++++ 5 files changed, 100 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c79c5c..f97915f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -113,6 +113,11 @@ The format is based on - Compare boxed privilege output values by their privilege and attributes; the object overload rejected privilege values and recursively compared an attributes enum instead +- Fix `Clear-NTFSAccess -DisableInheritance` and `Set-NTFSSecurityDescriptor`, + which reported a `RestoreOwnerError` for an owner that had not changed: + after they took ownership of an item that the user owned already and left a + DACL without the right to set an owner, they failed to set the same owner + back - Fix `Get-Help`, which showed only the syntax: ship the help file `en-US\NTFSSecurity.dll-Help.xml` generated from the cmdlet documentation, diff --git a/NTFSSecurity/BaseCmdlets.cs b/NTFSSecurity/BaseCmdlets.cs index 9dd30f1..07e7040 100644 --- a/NTFSSecurity/BaseCmdlets.cs +++ b/NTFSSecurity/BaseCmdlets.cs @@ -192,6 +192,8 @@ namespace NTFSSecurity /// /// Takes ownership of the item, runs the action, and restores the previous owner on every exit path. /// A failure to restore the owner is written as a RestoreOwnerError and doesn't hide an error of the action. + /// An owner that the action did not change, because the current user owned the item already, isn't set again: + /// an action such as clearing the DACL can leave nobody the right to do so. /// /// The file or folder to take ownership of. /// The path the user specified, used as the error target. @@ -199,8 +201,9 @@ namespace NTFSSecurity protected void InvokeAsOwner(Alphaleonis.Win32.Filesystem.FileSystemInfo item, string path, Action action) { var previousOwner = FileSystemOwner.GetOwner(item).Owner; + IdentityReference2 currentUser = System.Security.Principal.WindowsIdentity.GetCurrent().User; - FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); + FileSystemOwner.SetOwner(item, currentUser); try { @@ -208,13 +211,16 @@ namespace NTFSSecurity } finally { - try + if (previousOwner != currentUser) { - FileSystemOwner.SetOwner(item, previousOwner); - } - catch (Exception ex) - { - WriteError(new ErrorRecord(ex, "RestoreOwnerError", ErrorCategory.WriteError, path)); + try + { + FileSystemOwner.SetOwner(item, previousOwner); + } + catch (Exception ex) + { + WriteError(new ErrorRecord(ex, "RestoreOwnerError", ErrorCategory.WriteError, path)); + } } } } diff --git a/NTFSSecurity/SecurityDescriptorCmdlets/SetSecurityDescriptor.cs b/NTFSSecurity/SecurityDescriptorCmdlets/SetSecurityDescriptor.cs index 9b6f299..b3a6100 100644 --- a/NTFSSecurity/SecurityDescriptorCmdlets/SetSecurityDescriptor.cs +++ b/NTFSSecurity/SecurityDescriptorCmdlets/SetSecurityDescriptor.cs @@ -88,13 +88,16 @@ namespace NTFSSecurity } // Like InvokeAsOwner, takes ownership for the write and sets the previous owner back on every exit path, but not - // after a successful write of a descriptor that sets the owner itself, which would undo that owner. + // after a successful write of a descriptor that sets the owner itself, which would undo that owner, and not when the + // current user owned the item already, so that nothing changed and a descriptor that leaves nobody the right to set + // an owner can't make it fail. private void WriteChangesAsOwner(FileSystemSecurity2 sd) { var setsOwner = (sd.ChangedSections & AccessControlSections.Owner) == AccessControlSections.Owner; var previousOwner = FileSystemOwner.GetOwner(sd.Item).Owner; + IdentityReference2 currentUser = System.Security.Principal.WindowsIdentity.GetCurrent().User; - FileSystemOwner.SetOwner(sd.Item, System.Security.Principal.WindowsIdentity.GetCurrent().User); + FileSystemOwner.SetOwner(sd.Item, currentUser); var written = false; try @@ -104,7 +107,7 @@ namespace NTFSSecurity } finally { - if (!(written && setsOwner)) + if (!(written && setsOwner) && previousOwner != currentUser) { try { diff --git a/Tests/PathErrors.Tests.ps1 b/Tests/PathErrors.Tests.ps1 index eb613bd..96fcee5 100644 --- a/Tests/PathErrors.Tests.ps1 +++ b/Tests/PathErrors.Tests.ps1 @@ -35,6 +35,7 @@ BeforeDiscovery { @{ Command = 'Set-NTFSOwner'; Parameters = @{ Account = $currentUser }; ErrorId = 'ReadFileError'; Output = $false } @{ Command = 'Get-NTFSSecurityDescriptor'; Parameters = @{}; ErrorId = 'ReadFileError'; Output = $true } @{ Command = 'Get-Item2'; Parameters = @{}; ErrorId = 'FileNotFound'; Output = $true } + @{ Command = 'Get-ChildItem2'; Parameters = @{}; ErrorId = 'FileNotFound'; Output = $true } @{ Command = 'Get-FileHash2'; Parameters = @{}; ErrorId = 'ReadFileError'; Output = $true } @{ Command = 'Get-NTFSHardLink'; Parameters = @{}; ErrorId = 'FileNotFound'; Output = $true } ) @@ -259,6 +260,43 @@ Describe 'An item whose owner may not change its permissions' { @($acl.GetAccessRules($true, $false, $sidType)) | Should -BeNullOrEmpty } + # With the DACL cleared and protected, nobody keeps the right to set an owner, so setting the previous owner back + # fails. The user owned the item already, so there is no owner to set back. + It 'Clear-NTFSAccess -DisableInheritance should take ownership, clear and protect the DACL, and not set an unchanged owner back' { + $user = [Security.Principal.WindowsIdentity]::GetCurrent().User.Value + Set-TestOwner -Sandbox $sandbox -Path $file -Sid $user + Add-NTFSAccess -Path $file -Account 'S-1-1-0' -AccessRights ReadData + Add-TestDenyRule -Sandbox $sandbox -Path $file -Rights @{ 'S-1-3-4' = 'ChangePermissions' } + + Clear-NTFSAccess -Path $file -DisableInheritance -ErrorVariable changeErrors -ErrorAction SilentlyContinue + + $changeErrors | Should -BeNullOrEmpty + $acl = Get-TestAcl -Path $file + $acl.GetOwner($sidType).Value | Should -Be $user + $acl.AreAccessRulesProtected | Should -BeTrue + @($acl.GetAccessRules($true, $true, $sidType)) | Should -BeNullOrEmpty + } + + # Windows lets the owner of an item set another owner only with the Restore privilege, which the tests turn off, or + # with the right in the DACL, which the cleared DACL no longer holds. The cmdlet reports the owner it cannot set back. + It 'Clear-NTFSAccess -DisableInheritance should report RestoreOwnerError for a previous owner that it cannot set back' -Skip:(-not $holdsRestorePrivilege) { + $user = [Security.Principal.WindowsIdentity]::GetCurrent().User.Value + $owner | Should -Not -Be $user + Add-NTFSAccess -Path $file -Account 'S-1-1-0' -AccessRights ReadData + Add-TestDenyRule -Sandbox $sandbox -Path $file -Rights @{ 'S-1-3-4' = 'ChangePermissions' } + + Clear-NTFSAccess -Path $file -DisableInheritance -ErrorVariable changeErrors -ErrorAction SilentlyContinue + + $changeErrors | Should -HaveCount 1 + $changeErrors[0].FullyQualifiedErrorId | Should -BeLike 'RestoreOwnerError,*' + $changeErrors[0].CategoryInfo.Category | Should -Be 'WriteError' + $changeErrors[0].TargetObject | Should -Be $file + $acl = Get-TestAcl -Path $file + $acl.GetOwner($sidType).Value | Should -Be $user + $acl.AreAccessRulesProtected | Should -BeTrue + @($acl.GetAccessRules($true, $true, $sidType)) | Should -BeNullOrEmpty + } + It 'Disable-NTFSAccessInheritance should take ownership, protect the DACL, and set the owner back' { Add-TestDenyRule -Sandbox $sandbox -Path $file -Rights @{ 'S-1-3-4' = 'ChangePermissions' } diff --git a/Tests/SecurityDescriptor.Tests.ps1 b/Tests/SecurityDescriptor.Tests.ps1 index d60cb25..b77a470 100644 --- a/Tests/SecurityDescriptor.Tests.ps1 +++ b/Tests/SecurityDescriptor.Tests.ps1 @@ -225,6 +225,44 @@ Describe 'Set-NTFSSecurityDescriptor' { $result[0].FullName | Should -Be $file $result[0].SecurityDescriptor.GetOwner($sidType).Value | Should -Be 'S-1-5-32-544' } + + # With a cleared, protected DACL, nobody keeps the right to set an owner, so setting the previous owner back would + # fail. The user owned the item already, so there is no owner to set back. + It 'Should not report an owner that did not change when the write that took ownership leaves an empty DACL' { + $file = New-TestSandboxItem -Sandbox $sandbox -Name 'RetryUnchangedOwner' + Set-TestOwner -Sandbox $sandbox -Path $file -Sid $currentUser + Add-TestDenyRule -Sandbox $sandbox -Path $file -Rights @{ 'S-1-3-4' = 'ChangePermissions' } + $sd = Get-NTFSSecurityDescriptor -Path $file + Clear-NTFSAccess -SecurityDescriptor $sd -DisableInheritance -ErrorAction Stop + + Set-NTFSSecurityDescriptor -SecurityDescriptor $sd -ErrorVariable setErrors -ErrorAction SilentlyContinue + + $setErrors | Should -BeNullOrEmpty + $acl = Get-Acl -LiteralPath $file + $acl.GetOwner($sidType).Value | Should -Be $currentUser + $acl.AreAccessRulesProtected | Should -BeTrue + @($acl.GetAccessRules($true, $true, $sidType)) | Should -BeNullOrEmpty + } + + # Without the Restore privilege, the user can't set an owner such as TrustedInstaller back. The cmdlet reports it + # after it wrote the descriptor. + It 'Should report RestoreOwnerError for a previous owner that it cannot set back after the write' -Skip:(-not $canAssignAnyOwner) { + $file = New-TestSandboxItem -Sandbox $sandbox -Name 'RetryRestoreDenied' + Add-TestDenyRule -Sandbox $sandbox -Path $file -Rights @{ $currentUser = 'ChangePermissions' } + Set-TestOwner -Sandbox $sandbox -Path $file -Sid $trustedInstaller + Get-RestorePrivilegeState | Should -Be 'Disabled' + $sd = Get-NTFSSecurityDescriptor -Path $file + Add-NTFSAccess -SecurityDescriptor $sd -Account 'Everyone' -AccessRights ReadData + + Set-NTFSSecurityDescriptor -SecurityDescriptor $sd -ErrorVariable setErrors -ErrorAction SilentlyContinue + + $setErrors | Should -HaveCount 1 + $setErrors[0].FullyQualifiedErrorId | Should -BeLike 'RestoreOwnerError,*' + $setErrors[0].CategoryInfo.Category | Should -Be 'WriteError' + $setErrors[0].TargetObject.FullName | Should -Be $file + @(Get-EveryoneRule -Path $file) | Should -HaveCount 1 + (Get-Acl -LiteralPath $file).GetOwner($sidType).Value | Should -Be $currentUser + } } Context 'A descriptor that cannot be written' {