From 578042f09900dd10d49bbce6530d2c723c9f7f1a Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Thu, 8 Oct 2026 09:06:08 +0000 Subject: [PATCH] fix: disable the privileges by their current state when a cmdlet ends A cmdlet that enables the Backup, Restore, Take Ownership, and Security privileges decided which ones to disable on the states that it had read when it enabled them, and stopped at the first one that failed. When another command in the pipeline, such as Disable-Privileges, had disabled one of them, the cmdlet stopped with "Priviledge already disabled" and left the privileges after that one enabled. After an early stop, which Dispose handles since this branch, it left them enabled without any message, because PowerShell ignores exceptions thrown in Dispose; and Disable-Privileges threw that exception in Dispose on every call while the privileges were disabled. 4.2.6 already decided on the old states. DisablePrivilege now reads the current state, and the cleanup tries every privilege: in EndProcessing, a privilege that it can't disable gives a warning; in Dispose, it is ignored. The early-stop tests now pin EnablePrivileges and check that the cmdlet had enabled the privileges, so that they can't pass without testing anything. Found by the security review of fcb370e..00c3646 (findings 1 to 3). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- CHANGELOG.md | 5 ++ Docs/Concepts.md | 8 ++- NTFSSecurity/BaseCmdlets.cs | 35 +++++++++--- Tests/Privileges.Tests.ps1 | 106 +++++++++++++++++++++++++++++++++++- 4 files changed, 141 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 770d467..67f741b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -277,5 +277,10 @@ The format is based on Security privileges, which left them enabled in the session when a later command, such as `Select-Object -First`, or a terminating error stopped the pipeline early; they now disable them also then +- Fix the cmdlets that enable the privileges, which stopped with the error + "Priviledge already disabled" and left the other privileges enabled when + another command in the pipeline, such as `Disable-Privileges`, had + disabled one of them; a privilege that they can't disable now gives a + warning, and they still disable the others [Unreleased]: https://github.com/raandree/NTFSSecurity/compare/4.2.6...HEAD diff --git a/Docs/Concepts.md b/Docs/Concepts.md index d196d8e..680c0c3 100644 --- a/Docs/Concepts.md +++ b/Docs/Concepts.md @@ -189,13 +189,17 @@ session runs elevated (**Run as administrator**). The access, audit, inheritance, owner, and security descriptor cmdlets enable these privileges automatically while they run and disable the ones they enabled when they finish, also when a later command such as -`Select-Object -First` or a terminating error stops the pipeline early. If a +`Select-Object -First` or a terminating error stops the pipeline early. A +privilege that another command in the pipeline, such as `Disable-Privileges`, +has disabled in the meantime stays disabled, and a privilege that a cmdlet +can't disable when it finishes gives a warning. If a privilege cannot be enabled, the cmdlet continues without it. You can turn this behavior off with the `EnablePrivileges` module setting. Before 5.0.0, the inheritance cmdlets were an exception: they always tried to enable the privileges, and when `EnablePrivileges` was `$false`, they left them enabled. And before 5.0.0, every cmdlet left the privileges enabled in the session when -the pipeline stopped early. +the pipeline stopped early, and stopped with the error "Priviledge already +disabled" when another command in the pipeline had disabled one of them. `Enable-Privileges` enables the four privileges for the current PowerShell process until you run `Disable-Privileges` or close the session. diff --git a/NTFSSecurity/BaseCmdlets.cs b/NTFSSecurity/BaseCmdlets.cs index 9a300be..e2f2263 100644 --- a/NTFSSecurity/BaseCmdlets.cs +++ b/NTFSSecurity/BaseCmdlets.cs @@ -214,33 +214,48 @@ namespace NTFSSecurity //disable all privileges that have been enabled by this cmdlet WriteVerbose(string.Format("Disabeling all {0} enabled privileges...", enabledPrivileges.Count)); - foreach (var privilege in DisableEnabledPrivileges()) + var failed = new Dictionary(); + foreach (var privilege in DisableEnabledPrivileges(failed)) { WriteVerbose(string.Format("\t{0} disabled", privilege)); } + foreach (var failure in failed) + { + WriteDebug(string.Format("Could not disable privilege {0}. The error was: {1}", failure.Key, failure.Value.Message)); + WriteWarning(string.Format("The privilege '{0}' could not be disabled.", failure.Key)); + } WriteVerbose(string.Format("...finished")); } } // PowerShell calls Dispose also when a later command or a terminating error stops the pipeline, and then // skips EndProcessing. Before 5.0.0-rc6, the privileges that the cmdlet had enabled stayed enabled in the - // session in that case. + // session in that case. PowerShell ignores an exception from Dispose and no stream is open anymore, so a + // privilege that can't be disabled goes unreported here. public void Dispose() { - DisableEnabledPrivileges(); + DisableEnabledPrivileges(new Dictionary()); GC.SuppressFinalize(this); } - // Disables the privileges that this cmdlet enabled, once, and returns their names. - private List DisableEnabledPrivileges() + // Disables the privileges that this cmdlet enabled, once, and returns their names. A privilege that can't be + // disabled goes to failed with its error and doesn't keep the others enabled. + private List DisableEnabledPrivileges(Dictionary failed) { var disabled = new List(); if (!KeepEnabledPrivileges) { foreach (var privilege in enabledPrivileges) { - DisablePrivilege((Privilege)Enum.Parse(typeof(Privilege), privilege)); - disabled.Add(privilege); + try + { + DisablePrivilege((Privilege)Enum.Parse(typeof(Privilege), privilege)); + disabled.Add(privilege); + } + catch (Exception ex) + { + failed[privilege] = ex; + } } } @@ -270,8 +285,10 @@ namespace NTFSSecurity public void DisablePrivilege(Privilege privilege) { - //if the privilege is enabled - if (privileges.Single(p => p.Privilege == privilege).PrivilegeState == PrivilegeState.Enabled) + // The current state, not the one that the cmdlet read when it enabled the privileges: another command, also + // one in the same pipeline, can have disabled the privilege since then. Before 5.0.0-rc6, the cmdlet then + // failed with "Priviledge already disabled" and left the privileges after this one enabled. + if (privControl.GetPrivileges().Any(p => p.Privilege == privilege && p.PrivilegeState == PrivilegeState.Enabled)) privControl.DisablePrivilege(privilege); } diff --git a/Tests/Privileges.Tests.ps1 b/Tests/Privileges.Tests.ps1 index c688cf2..e75e18f 100644 --- a/Tests/Privileges.Tests.ps1 +++ b/Tests/Privileges.Tests.ps1 @@ -29,6 +29,13 @@ BeforeAll { function Get-BackupPrivilegeState { (Get-Privileges | Where-Object -Property Privilege -EQ -Value 'Backup').PrivilegeState } + + function Get-EnabledFileSystemPrivilege { + # The names of the privileges that the cmdlets enable, as far as they are enabled now + @(Get-Privileges | Where-Object -FilterScript { + $_.Privilege -in 'TakeOwnership', 'Restore', 'Backup', 'Security' -and $_.PrivilegeState -eq 'Enabled' + } | ForEach-Object -Process { $_.Privilege.ToString() }) + } } AfterAll { @@ -122,10 +129,16 @@ Describe 'Inheritance cmdlets' { Describe 'Privileges when the pipeline stops early' { BeforeAll { + # The cmdlets enable the privileges only with this setting; without it, these tests would prove nothing. + $privateData['EnablePrivileges'] = $true $files = 1..3 | ForEach-Object -Process { New-TestSandboxItem -Sandbox $sandbox -Name "Stopped$_" } $missing = Join-Path -Path $sandbox -ChildPath 'StoppedMissing.txt' } + AfterAll { + $privateData['EnablePrivileges'] = $enablePrivileges + } + BeforeEach { Disable-Privileges -ErrorAction SilentlyContinue -WarningAction SilentlyContinue } @@ -141,16 +154,24 @@ Describe 'Privileges when the pipeline stops early' { It 'Should disable the privileges after Select-Object -First stops the pipeline' -Skip:(-not $holdsPrivileges) { Get-BackupPrivilegeState | Should -Be 'Disabled' - Get-NTFSOwner -Path $files | Select-Object -First 1 | Out-Null + $stateWhileRunning = Get-NTFSOwner -Path $files | ForEach-Object -Process { Get-BackupPrivilegeState } | + Select-Object -First 1 + $stateWhileRunning | Should -Be 'Enabled' Get-BackupPrivilegeState | Should -Be 'Disabled' } It 'Should disable the privileges after a terminating error' -Skip:(-not $holdsPrivileges) { Get-BackupPrivilegeState | Should -Be 'Disabled' + $statesWhileRunning = New-Object -TypeName 'System.Collections.Generic.List[string]' - { Get-NTFSAccess -Path $missing, $files[0] -ErrorAction Stop } | Should -Throw + { + Get-NTFSAccess -Path $files[0], $missing -ErrorAction Stop | + ForEach-Object -Process { $statesWhileRunning.Add((Get-BackupPrivilegeState)) } + } | Should -Throw + $statesWhileRunning | Should -Not -BeNullOrEmpty + $statesWhileRunning | Should -Not -Contain 'Disabled' Get-BackupPrivilegeState | Should -Be 'Disabled' } @@ -160,3 +181,84 @@ Describe 'Privileges when the pipeline stops early' { Get-BackupPrivilegeState | Should -Be 'Enabled' } } + +Describe 'Privileges that another command in the pipeline changes' { + BeforeAll { + $privateData['EnablePrivileges'] = $true + $files = 1..3 | ForEach-Object -Process { New-TestSandboxItem -Sandbox $sandbox -Name "Changed$_" } + + function Disable-TakeOwnershipOnce { + # Passes the objects on and disables the Take Ownership privilege when the first one passes, as another + # command in the pipeline can. Records the state of the Backup privilege at that moment. + param ( + [Parameter(ValueFromPipeline)] + [object] + $InputObject, + + [Parameter(Mandatory)] + [AllowEmptyCollection()] + [System.Collections.Generic.List[string]] + $BackupState + ) + + begin { + $first = $true + } + + process { + if ($first) { + $BackupState.Add((Get-BackupPrivilegeState)) + $null = [ProcessPrivileges.ProcessExtensions]::DisablePrivilege( + [System.Diagnostics.Process]::GetCurrentProcess(), [ProcessPrivileges.Privilege]::TakeOwnership + ) + $first = $false + } + + $InputObject + } + } + } + + AfterAll { + $privateData['EnablePrivileges'] = $enablePrivileges + } + + BeforeEach { + Disable-Privileges -ErrorAction SilentlyContinue -WarningAction SilentlyContinue + } + + AfterEach { + Disable-Privileges -ErrorAction SilentlyContinue -WarningAction SilentlyContinue + } + + # Before 5.0.0-rc6, a cmdlet decided which privileges to disable on the states that it had read when it enabled + # them. A privilege that another command had disabled since then stopped it with "Priviledge already disabled", + # and the privileges after that one in its list stayed enabled. + It 'Should disable the other privileges when another command disabled one of them' -Skip:(-not $holdsPrivileges) { + $backupState = New-Object -TypeName 'System.Collections.Generic.List[string]' + + { Get-NTFSOwner -Path $files | Disable-TakeOwnershipOnce -BackupState $backupState | Out-Null } | Should -Not -Throw + + $backupState | Should -Be 'Enabled' + Get-EnabledFileSystemPrivilege | Should -BeNullOrEmpty + } + + It 'Should disable the other privileges when another command disabled one of them and the pipeline stops early' -Skip:(-not $holdsPrivileges) { + $backupState = New-Object -TypeName 'System.Collections.Generic.List[string]' + + Get-NTFSOwner -Path $files | Disable-TakeOwnershipOnce -BackupState $backupState | Select-Object -First 1 | Out-Null + + $backupState | Should -Be 'Enabled' + Get-EnabledFileSystemPrivilege | Should -BeNullOrEmpty + } + + It 'Should not fail when Disable-Privileges runs inside the pipeline' -Skip:(-not $holdsPrivileges) { + { + Get-NTFSOwner -Path $files | + ForEach-Object -Process { Disable-Privileges -WarningAction SilentlyContinue; $_ } | + Out-Null + } | Should -Not -Throw + + Get-EnabledFileSystemPrivilege | Should -BeNullOrEmpty + } +}