Browse Source

fix(owner): pass on what a later command raises at the RestoreOwnerError record

InvokeAsOwner (and Set-NTFSSecurityDescriptor's WriteChangesAsOwner) take
ownership of an item, run the action, and in a finally restore the
previous owner, writing a non-terminating RestoreOwnerError through
WriteError when that restore fails. WriteError runs the later commands
of the pipeline while it writes, so in the rare case that a later
command throws exactly when it sees that RestoreOwnerError record (for
example `... 2>&1 | ForEach-Object { if ($_.FullyQualifiedErrorId -like
'RestoreOwnerError,*') { throw } else { $_ } }`), that exception used to
leave InvokeAsOwner through the finally and reach the handler around the
call, which reported it as the item's own error (AddAceError,
ReadSecurityError, ModifySdError, and so on) and moved on to the next
path. The caller never saw the exception.

Thirteen handlers now check IsFromLaterCommand as the first statement of
their catch, the same pattern SetSecurityDescriptor.cs already used in
its second handler and GetSecurityDescriptor.cs already used: Add/Clear
and Get-NTFSAccess, Get-NTFSOrphanedAccess, Get-NTFSEffectiveAccess,
Clear-NTFSAudit, Get-FileHash2, the six inheritance cmdlets, and the
owner-retry of Set-NTFSSecurityDescriptor. The four add/remove access and
audit cmdlets have the same flaw and belong to another work item.

Tests\PipelineControl.Tests.ps1 adds one table-driven It, elevated only,
that denies the current user the right a cmdlet's action needs, gives
the item an owner only the Restore privilege can set back (which the
test turns off), and asserts that a later command's exception reaches
the caller, no item-error record was written for it, and the next path
is untouched. Six of the thirteen handlers are pinned this way (the
other seven need the current user to both fail and succeed the same
access check at once, which Windows does not allow; see the comment
above the Describe block and the probes under ..\probes-wi2\ for the
empirical evidence). The four add and remove cmdlets, which have the same
flaw, follow in the next commit of this item.

Get-NTFSInheritance, the one of the thirteen that writes its result in a
finally, writes it after the try instead, so that the object does not go to
the later command that has just thrown.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: AI Assistant <ai@example.com>
pull/131/head
Raimund Andree 2 weeks ago
parent
commit
62cf026e6e
  1. 6
      NTFSSecurity/AccessCmdlets/ClearAccess.cs
  2. 6
      NTFSSecurity/AccessCmdlets/GetAccess.cs
  3. 6
      NTFSSecurity/AccessCmdlets/GetEffectiveAccess.cs
  4. 6
      NTFSSecurity/AccessCmdlets/GetOrphanedAccess.cs
  5. 6
      NTFSSecurity/AuditCmdlets/ClearAudit.cs
  6. 6
      NTFSSecurity/InheritanceCmdlets/DisableAccessInheritance.cs
  7. 6
      NTFSSecurity/InheritanceCmdlets/DisableAuditInheritance.cs
  8. 6
      NTFSSecurity/InheritanceCmdlets/EnableAccessInheritance.cs
  9. 6
      NTFSSecurity/InheritanceCmdlets/EnableAuditInheritance.cs
  10. 12
      NTFSSecurity/InheritanceCmdlets/GetInheritance.cs
  11. 6
      NTFSSecurity/InheritanceCmdlets/SetInheritance.cs
  12. 6
      NTFSSecurity/MiscCmdlets/GetFileHash2.cs
  13. 6
      NTFSSecurity/SecurityDescriptorCmdlets/SetSecurityDescriptor.cs
  14. 161
      Tests/PipelineControl.Tests.ps1

6
NTFSSecurity/AccessCmdlets/ClearAccess.cs

@ -86,6 +86,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, "ClearAclError", ErrorCategory.WriteError, path));
}
}

6
NTFSSecurity/AccessCmdlets/GetAccess.cs

@ -112,6 +112,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, "ReadSecurityError", ErrorCategory.WriteError, path));
continue;
}

6
NTFSSecurity/AccessCmdlets/GetEffectiveAccess.cs

@ -141,6 +141,12 @@ namespace NTFSSecurity
}
catch (Exception ex2)
{
// Not what a later command raises, for example when it takes the RestoreOwnerError record.
if (IsFromLaterCommand(ex2))
{
throw;
}
this.WriteError(new ErrorRecord(ex2, "ReadSecurityError", ErrorCategory.WriteError, path));
continue;
}

6
NTFSSecurity/AccessCmdlets/GetOrphanedAccess.cs

@ -55,6 +55,12 @@ namespace NTFSSecurity
}
catch (Exception ex2)
{
// Not what a later command raises, for example when it takes the RestoreOwnerError record.
if (IsFromLaterCommand(ex2))
{
throw;
}
// A read error; before 5.0.0-rc6, it was reported as an AddAceError.
this.WriteError(new ErrorRecord(ex2, "ReadSecurityError", ErrorCategory.ReadError, path));
continue;

6
NTFSSecurity/AuditCmdlets/ClearAudit.cs

@ -86,6 +86,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, "ClearAclError", ErrorCategory.WriteError, path));
}
}

6
NTFSSecurity/InheritanceCmdlets/DisableAccessInheritance.cs

@ -90,6 +90,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, "ModifySdError", ErrorCategory.WriteError, path));
continue;
}

6
NTFSSecurity/InheritanceCmdlets/DisableAuditInheritance.cs

@ -96,6 +96,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, "ModifySdError", ErrorCategory.WriteError, path));
continue;
}

6
NTFSSecurity/InheritanceCmdlets/EnableAccessInheritance.cs

@ -90,6 +90,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, "ModifySdError", ErrorCategory.WriteError, path));
continue;
}

6
NTFSSecurity/InheritanceCmdlets/EnableAuditInheritance.cs

@ -95,6 +95,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, "ModifySdError", ErrorCategory.WriteError, path));
continue;
}

12
NTFSSecurity/InheritanceCmdlets/GetInheritance.cs

@ -80,6 +80,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, "ReadSecurityError", ErrorCategory.WriteError, path));
continue;
}
@ -89,15 +95,15 @@ namespace NTFSSecurity
WriteError(new ErrorRecord(ex, "ReadSecurityError", ErrorCategory.OpenError, path));
continue;
}
finally
{
// After the try, not in a finally: what a later command raises while the retry restores the owner
// leaves the loop through the check above, and the object must not go to that command then.
if (inheritanceInfo != null)
{
WriteObject(inheritanceInfo);
}
}
}
}
else
{
foreach (var sd in securityDescriptors)

6
NTFSSecurity/InheritanceCmdlets/SetInheritance.cs

@ -98,6 +98,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, "ModifySdError", ErrorCategory.WriteError, path));
continue;
}

6
NTFSSecurity/MiscCmdlets/GetFileHash2.cs

@ -103,6 +103,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, "GetHashError", ErrorCategory.WriteError, path));
continue;
}

6
NTFSSecurity/SecurityDescriptorCmdlets/SetSecurityDescriptor.cs

@ -61,6 +61,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, "WriteSdError", ErrorCategory.WriteError, sd.Item));
continue;
}

161
Tests/PipelineControl.Tests.ps1

@ -636,3 +636,164 @@ namespace NtfsSecurityTests { public class StandInForBreak : System.Management.A
$isEnd.Invoke($null, @($Exception)) | Should -BeFalse
}
}
# InvokeAsOwner (and Set-NTFSSecurityDescriptor's WriteChangesAsOwner) take ownership, run the action, and in a
# finally restore the previous owner, writing a RestoreOwnerError through WriteError when that restore fails.
# WriteError runs the later commands of the pipeline and notes what they raise (IsFromLaterCommand, above); the catch
# around the InvokeAsOwner/WriteChangesAsOwner call must pass that on instead of reporting it as the item's own error
# 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,
# 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.
#
# NOT DONE for the other seven (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).
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'
)
}
BeforeAll {
# An owner that the user can assign only with the Restore privilege, which EnablePrivileges = $false below
# keeps disabled, matching the existing RestoreOwnerError tests of the other files.
$trustedInstaller = 'S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464'
$enablePrivileges = $privateData['EnablePrivileges']
$privateData['EnablePrivileges'] = $false
$laterCommandRestoreOwnerCases = @{
'Clear-NTFSAccess' = @{
Prepare = {
$blocked = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterClearBlocked'
$next = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterClearNext'
Set-TestOwner -Sandbox $sandbox -Path $blocked -Sid 'S-1-5-32-544'
Add-NTFSAccess -Path $blocked, $next -Account 'S-1-1-0' -AccessRights ReadData -ErrorAction Stop
Add-TestDenyRule -Sandbox $sandbox -Path $blocked -Rights @{ 'S-1-3-4' = 'ChangePermissions' }
@{ Blocked = $blocked; Next = $next }
}
Run = { param ($Context) Clear-NTFSAccess -Path $Context.Blocked, $Context.Next -DisableInheritance -ErrorAction Continue }
ItemErrorId = 'ClearAclError'
Untouched = { param ($Context, $Emitted) (Test-ExplicitEntry -Path $Context.Next -Account 'S-1-1-0') -and -not (Get-Acl -LiteralPath $Context.Next).AreAccessRulesProtected -and $Emitted.Count -eq 0 }
}
'Disable-NTFSAccessInheritance' = @{
Prepare = {
$blocked = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterDisableBlocked'
$next = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterDisableNext'
Add-TestDenyRule -Sandbox $sandbox -Path $blocked -Rights @{ $currentUser = 'ChangePermissions' }
Set-TestOwner -Sandbox $sandbox -Path $blocked -Sid $trustedInstaller
@{ Blocked = $blocked; Next = $next }
}
Run = { param ($Context) Disable-NTFSAccessInheritance -Path $Context.Blocked, $Context.Next -ErrorAction Continue }
ItemErrorId = 'ModifySdError'
Untouched = { param ($Context, $Emitted) -not (Get-Acl -LiteralPath $Context.Next).AreAccessRulesProtected -and $Emitted.Count -eq 0 }
}
'Enable-NTFSAccessInheritance' = @{
Prepare = {
$blocked = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterEnableBlocked'
$next = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterEnableNext'
Disable-NTFSAccessInheritance -Path $blocked, $next -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) Enable-NTFSAccessInheritance -Path $Context.Blocked, $Context.Next -ErrorAction Continue }
ItemErrorId = 'ModifySdError'
Untouched = { param ($Context, $Emitted) (Get-Acl -LiteralPath $Context.Next).AreAccessRulesProtected -and $Emitted.Count -eq 0 }
}
'Set-NTFSInheritance' = @{
Prepare = {
$blocked = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterSetInheritanceBlocked'
$next = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterSetInheritanceNext'
Add-TestDenyRule -Sandbox $sandbox -Path $blocked -Rights @{ $currentUser = 'ChangePermissions' }
Set-TestOwner -Sandbox $sandbox -Path $blocked -Sid $trustedInstaller
@{ Blocked = $blocked; Next = $next }
}
Run = { param ($Context) Set-NTFSInheritance -Path $Context.Blocked, $Context.Next -AccessInheritanceEnabled $false -ErrorAction Continue }
ItemErrorId = 'ModifySdError'
Untouched = { param ($Context, $Emitted) -not (Get-Acl -LiteralPath $Context.Next).AreAccessRulesProtected -and $Emitted.Count -eq 0 }
}
'Set-NTFSSecurityDescriptor' = @{
Prepare = {
$blocked = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterSdBlocked'
$next = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterSdNext'
$descriptors = @(Get-NTFSSecurityDescriptor -Path $blocked, $next -ErrorAction Stop)
Add-NTFSAccess -SecurityDescriptor $descriptors -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
@{ Descriptors = $descriptors; Next = $next }
}
Run = { param ($Context) Set-NTFSSecurityDescriptor -SecurityDescriptor $Context.Descriptors -ErrorAction Continue }
ItemErrorId = 'WriteSdError'
Untouched = { param ($Context, $Emitted) -not (Test-ExplicitEntry -Path $Context.Next -Account 'S-1-1-0') -and $Emitted.Count -eq 0 }
}
'Get-FileHash2' = @{
Prepare = {
$blocked = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterHashBlocked'
$next = New-TestSandboxItem -Sandbox $sandbox -Name 'LaterHashNext'
Add-TestDenyRule -Sandbox $sandbox -Path $blocked -Rights @{ 'S-1-1-0' = 'ReadData' }
Set-TestOwner -Sandbox $sandbox -Path $blocked -Sid $trustedInstaller
@{ Blocked = $blocked; Next = $next }
}
Run = { param ($Context) Get-FileHash2 -Path $Context.Blocked, $Context.Next -ErrorAction Continue }
ItemErrorId = 'GetHashError'
# Get-FileHash2 has no ACL state of its own to read back, so the oracle is that the next path still
# exists, untouched, and that the pipeline produced no hash at all: the next path's hash would be the
# only object a later-command block ever sees, since neither -PassThru nor an output object exists
# for the blocked path in either state of the fix. Not @($Emitted): PowerShell's array operator
# raises ArgumentException for an empty generic List(Of T).
Untouched = { param ($Context, $Emitted) (Test-Path -LiteralPath $Context.Next) -and $Emitted.Count -eq 0 }
}
}
}
AfterAll {
$privateData['EnablePrivileges'] = $enablePrivileges
}
It '<_> should pass on what a later command raises at the RestoreOwnerError record instead of reporting it as the item''s own error, and leave the next path untouched' -Skip:(-not $holdsRestorePrivilege) -ForEach $laterCommandRestoreOwnerNames {
$case = $laterCommandRestoreOwnerCases[$_]
$context = & $case.Prepare
$Error.Clear()
$caught = $null
$emitted = New-Object -TypeName 'System.Collections.Generic.List[object]'
try {
& $case.Run $context 2>&1 | ForEach-Object -Process {
if ($_ -is [System.Management.Automation.ErrorRecord] -and $_.FullyQualifiedErrorId -like 'RestoreOwnerError,*') {
throw 'later'
}
$emitted.Add($_)
$_
}
}
catch {
$caught = $_
}
$caught.Exception.Message | Should -BeLike '*later*'
@($Error | Where-Object -FilterScript { $_.FullyQualifiedErrorId -like ('{0},*' -f $case.ItemErrorId) }) | Should -BeNullOrEmpty
(& $case.Untouched $context $emitted) | Should -BeTrue
}
# Without a later command, the existing behavior stays: RestoreOwnerError is reported and the cmdlet goes on to
# the next path, already pinned by the tests named above for all six rows; not duplicated here.
}

Loading…
Cancel
Save