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' {