From 7732fe57ac82f67c9427ac1297de49bb0c1857d5 Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Wed, 21 Oct 2026 12:52:45 +0000 Subject: [PATCH] fix(owner): pass on what a later command raises at the RestoreOwnerError record for the add and remove cmdlets Add-NTFSAccess, Remove-NTFSAccess, Add-NTFSAudit, and Remove-NTFSAudit had the same flaw as the 13 handlers fixed before: when a later command of the pipeline throws exactly at the RestoreOwnerError record that InvokeAsOwner writes after a failed restore of the owner, the catch around the call reported the exception as the item's own AddAceError or RemoveAceError and went on with the next path, so the caller never saw it. The four handlers now check IsFromLaterCommand first, like the others, so all 17 handlers of item 7 of Decision 27 pass the exception on. The table of Tests\PipelineControl.Tests.ps1 gets two rows, for Add-NTFSAccess and Remove-NTFSAccess, which use the scenario of the inheritance cmdlets (a ChangePermissions deny for the user and an owner that only the Restore privilege can set back). The two audit cmdlets can't be pinned this way: Windows lets this elevated session write the SACL whatever the DACL says, so their action never fails to begin with. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- NTFSSecurity/AccessCmdlets/AddAccess.cs | 6 +++ NTFSSecurity/AccessCmdlets/RemoveAccess.cs | 6 +++ NTFSSecurity/AuditCmdlets/AddAudit.cs | 6 +++ NTFSSecurity/AuditCmdlets/RemoveAudit.cs | 6 +++ Tests/PipelineControl.Tests.ps1 | 51 ++++++++++++++++------ 5 files changed, 62 insertions(+), 13 deletions(-) diff --git a/NTFSSecurity/AccessCmdlets/AddAccess.cs b/NTFSSecurity/AccessCmdlets/AddAccess.cs index 550905f..d3f93c3 100644 --- a/NTFSSecurity/AccessCmdlets/AddAccess.cs +++ b/NTFSSecurity/AccessCmdlets/AddAccess.cs @@ -190,6 +190,12 @@ namespace NTFSSecurity } catch (Exception ex2) { + // Not what a later command raises, for example when it takes the RestoreOwnerError record. + if (IsFromLaterCommand(ex2)) + { + throw; + } + WriteError(new ErrorRecord(ex2, "AddAceError", ErrorCategory.WriteError, path)); continue; } diff --git a/NTFSSecurity/AccessCmdlets/RemoveAccess.cs b/NTFSSecurity/AccessCmdlets/RemoveAccess.cs index a9d3a74..cb24b15 100644 --- a/NTFSSecurity/AccessCmdlets/RemoveAccess.cs +++ b/NTFSSecurity/AccessCmdlets/RemoveAccess.cs @@ -201,6 +201,12 @@ namespace NTFSSecurity } catch (Exception ex2) { + // Not what a later command raises, for example when it takes the RestoreOwnerError record. + if (IsFromLaterCommand(ex2)) + { + throw; + } + WriteError(new ErrorRecord(ex2, "RemoveAceError", ErrorCategory.WriteError, path)); continue; } diff --git a/NTFSSecurity/AuditCmdlets/AddAudit.cs b/NTFSSecurity/AuditCmdlets/AddAudit.cs index 560d333..d06f424 100644 --- a/NTFSSecurity/AuditCmdlets/AddAudit.cs +++ b/NTFSSecurity/AuditCmdlets/AddAudit.cs @@ -189,6 +189,12 @@ namespace NTFSSecurity } catch (Exception ex2) { + // Not what a later command raises, for example when it takes the RestoreOwnerError record. + if (IsFromLaterCommand(ex2)) + { + throw; + } + WriteError(new ErrorRecord(ex2, "AddAceError", ErrorCategory.WriteError, path)); continue; } diff --git a/NTFSSecurity/AuditCmdlets/RemoveAudit.cs b/NTFSSecurity/AuditCmdlets/RemoveAudit.cs index b1de294..8aefd04 100644 --- a/NTFSSecurity/AuditCmdlets/RemoveAudit.cs +++ b/NTFSSecurity/AuditCmdlets/RemoveAudit.cs @@ -200,6 +200,12 @@ namespace NTFSSecurity } catch (Exception ex2) { + // Not what a later command raises, for example when it takes the RestoreOwnerError record. + if (IsFromLaterCommand(ex2)) + { + throw; + } + WriteError(new ErrorRecord(ex2, "RemoveAceError", ErrorCategory.WriteError, path)); continue; } diff --git a/Tests/PipelineControl.Tests.ps1 b/Tests/PipelineControl.Tests.ps1 index 89f97bd..01b117f 100644 --- a/Tests/PipelineControl.Tests.ps1 +++ b/Tests/PipelineControl.Tests.ps1 @@ -644,32 +644,32 @@ namespace NtfsSecurityTests { public class StandInForBreak : System.Management.A # and going on, or the caller never sees it. Each row denies the current user the right its own action needs (a # change denial for the writers, a read denial on Everyone for Get-FileHash2, which needs file data, not # permissions) and gives the item an owner that only the Restore privilege can set back, which the tests turn off; -# InvokeAsOwner/WriteChangesAsOwner then takes ownership, runs the action, and fails to restore the owner. Six of -# the thirteen handlers of item 7 can be pinned this way; the scenario and the "without a later command" assertion -# already exist for all six: ParameterSets.Inheritance.Tests.ps1 (Disable/Enable-NTFSAccessInheritance, +# InvokeAsOwner/WriteChangesAsOwner then takes ownership, runs the action, and fails to restore the owner. Eight of +# the seventeen handlers of item 7 can be pinned this way. The scenario and the "without a later command" assertion +# already exist for six of them: ParameterSets.Inheritance.Tests.ps1 (Disable/Enable-NTFSAccessInheritance, # Set-NTFSInheritance), PathErrors.Tests.ps1 (Clear-NTFSAccess -DisableInheritance), SecurityDescriptor.Tests.ps1 -# (Set-NTFSSecurityDescriptor), FileHash.Tests.ps1 (Get-FileHash2), so this table only adds the later-command -# assertion, not a duplicate of the existing one. +# (Set-NTFSSecurityDescriptor), FileHash.Tests.ps1 (Get-FileHash2); Add-NTFSAccess and Remove-NTFSAccess use the same +# scenario as the inheritance cmdlets. So this table only adds the later-command assertion, not a duplicate of the +# existing one. # -# NOT DONE for the other seven (the reason is the same Windows rule in both groups: taking ownership only bypasses a +# NOT DONE for the other nine (the reason is the same Windows rule in both groups: taking ownership only bypasses a # denial of ReadPermissions/ChangePermissions, never of a different right): Get-NTFSAccess, Get-NTFSOrphanedAccess, # Get-NTFSEffectiveAccess, and Get-NTFSInheritance read the security descriptor; InvokeAsOwner's own first statement, # FileSystemOwner.GetOwner, reads it too and needs the same ReadPermissions right the cmdlet's read needs, so a # denial that fails one fails the other and InvokeAsOwner never reaches the owner-restore step (verified: denying # ReadPermissions directly to the user reports ReadSecurityError with the plain UnauthorizedAccessException, not # RestoreOwnerError, and the owner is never taken; probes-wi2\p-restoreowner-read-vs-write.ps1, -# p-read-cmdlets-2.ps1). Clear-NTFSAudit, Disable-NTFSAuditInheritance, and Enable-NTFSAuditInheritance write the -# SACL, which Windows grants to this elevated session regardless of a DACL deny and regardless of the module's -# EnablePrivileges setting, so the action does not fail to begin with (verified with both a ChangePermissions deny -# and EnablePrivileges off: no error, the SACL is written, and the owner is left unchanged; -# probes-wi2\p-audit-restoreowner.ps1, p-audit-restoreowner-2.ps1). The integrator's four rows for -# Add/Remove-NTFSAccess/Audit need only the same four pieces of data (Prepare, Run, ItemErrorId, Untouched). +# p-read-cmdlets-2.ps1). Clear-NTFSAudit, Disable-NTFSAuditInheritance, Enable-NTFSAuditInheritance, Add-NTFSAudit, and +# Remove-NTFSAudit write the SACL, which Windows grants to this elevated session regardless of a DACL deny and +# regardless of the module's EnablePrivileges setting, so the action does not fail to begin with (verified with both +# a ChangePermissions deny and EnablePrivileges off: no error, the SACL is written, and the owner is left unchanged; +# probes-wi2\p-audit-restoreowner.ps1, p-audit-restoreowner-2.ps1). The handlers of all seventeen carry the check. Describe 'A later command that throws at the RestoreOwnerError record' { BeforeDiscovery { $holdsRestorePrivilege = Test-PrivilegeHeld -Name 'SeRestorePrivilege' $laterCommandRestoreOwnerNames = @( 'Clear-NTFSAccess', 'Disable-NTFSAccessInheritance', 'Enable-NTFSAccessInheritance', 'Set-NTFSInheritance', - 'Set-NTFSSecurityDescriptor', 'Get-FileHash2' + 'Set-NTFSSecurityDescriptor', 'Get-FileHash2', 'Add-NTFSAccess', 'Remove-NTFSAccess' ) } @@ -762,6 +762,31 @@ Describe 'A later command that throws at the RestoreOwnerError record' { # raises ArgumentException for an empty generic List(Of T). Untouched = { param ($Context, $Emitted) (Test-Path -LiteralPath $Context.Next) -and $Emitted.Count -eq 0 } } + 'Add-NTFSAccess' = @{ + Prepare = { + $blocked = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterAddBlocked' + $next = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterAddNext' + Add-TestDenyRule -Sandbox $sandbox -Path $blocked -Rights @{ $currentUser = 'ChangePermissions' } + Set-TestOwner -Sandbox $sandbox -Path $blocked -Sid $trustedInstaller + @{ Blocked = $blocked; Next = $next } + } + Run = { param ($Context) Add-NTFSAccess -Path $Context.Blocked, $Context.Next -Account 'S-1-1-0' -AccessRights ReadData -ErrorAction Continue } + ItemErrorId = 'AddAceError' + Untouched = { param ($Context, $Emitted) -not (Test-ExplicitEntry -Path $Context.Next -Account 'S-1-1-0') -and $Emitted.Count -eq 0 } + } + 'Remove-NTFSAccess' = @{ + Prepare = { + $blocked = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterRemoveBlocked' + $next = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterRemoveNext' + Add-NTFSAccess -Path $blocked, $next -Account 'S-1-1-0' -AccessRights ReadData -ErrorAction Stop + Add-TestDenyRule -Sandbox $sandbox -Path $blocked -Rights @{ $currentUser = 'ChangePermissions' } + Set-TestOwner -Sandbox $sandbox -Path $blocked -Sid $trustedInstaller + @{ Blocked = $blocked; Next = $next } + } + Run = { param ($Context) Remove-NTFSAccess -Path $Context.Blocked, $Context.Next -Account 'S-1-1-0' -AccessRights ReadData -ErrorAction Continue } + ItemErrorId = 'RemoveAceError' + Untouched = { param ($Context, $Emitted) (Test-ExplicitEntry -Path $Context.Next -Account 'S-1-1-0') -and $Emitted.Count -eq 0 } + } } }