Browse Source

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 <ai@example.com>
pull/131/head
Raimund Andree 1 week ago
parent
commit
7732fe57ac
  1. 6
      NTFSSecurity/AccessCmdlets/AddAccess.cs
  2. 6
      NTFSSecurity/AccessCmdlets/RemoveAccess.cs
  3. 6
      NTFSSecurity/AuditCmdlets/AddAudit.cs
  4. 6
      NTFSSecurity/AuditCmdlets/RemoveAudit.cs
  5. 51
      Tests/PipelineControl.Tests.ps1

6
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;
}

6
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;
}

6
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;
}

6
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;
}

51
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 }
}
}
}

Loading…
Cancel
Save