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 <ai@example.com>

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

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.

NTFSSecurity/BaseCmdlets.cs

@@ -214,34 +214,49 @@ 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<string, Exception>();
+                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<string, Exception>());
             GC.SuppressFinalize(this);
         }
 
-        // Disables the privileges that this cmdlet enabled, once, and returns their names.
-        private List<string> 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<string> DisableEnabledPrivileges(Dictionary<string, Exception> failed)
         {
             var disabled = new List<string>();
             if (!KeepEnabledPrivileges)
             {
                 foreach (var privilege in enabledPrivileges)
+                {
+                    try
                     {
                         DisablePrivilege((Privilege)Enum.Parse(typeof(Privilege), privilege));
                         disabled.Add(privilege);
                     }
+                    catch (Exception ex)
+                    {
+                        failed[privilege] = ex;
+                    }
+                }
             }
 
             enabledPrivileges.Clear();
@@ -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);
         }
 

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