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