From a0a97d971e91f47e2f540c0cc912083f03c4de11 Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Wed, 21 Oct 2026 11:48:16 +0000 Subject: [PATCH] fix(security2): honor the account list of RemoveFileSystemAccessRuleAll and RemoveFileSystemAuditRuleAll Both methods built a LINQ query that filtered the explicit entries down to the ones whose account was in the given list, but never used the result: the loop right below removed every explicit entry of the descriptor regardless of the list, including an empty one. The cmdlets never pass a list, so only a .NET caller of Security2.dll that does is affected. A non-null list now removes only the explicit entries whose account is one of the listed accounts (compared like the rest of the class does, by IdentityReference2 equality of the entry's SID); null still removes every explicit entry, and an empty list now removes none. Inherited entries are still never touched, as before. Both the FileSystemSecurity2 overload (in-memory only, until a caller writes the descriptor) and the FileSystemInfo overload (which also writes) are fixed, for access and audit entries alike. Public signatures are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- ...essRule2.RemoveFileSystemAccessRulesAll.cs | 14 +- ...AuditRule2.RemoveFileSystemAuditRuleAll.cs | 14 +- Tests/ObjectApis.Tests.ps1 | 165 ++++++++++++++++++ 3 files changed, 183 insertions(+), 10 deletions(-) diff --git a/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.RemoveFileSystemAccessRulesAll.cs b/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.RemoveFileSystemAccessRulesAll.cs index bcc1c27..45cd7e0 100644 --- a/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.RemoveFileSystemAccessRulesAll.cs +++ b/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.RemoveFileSystemAccessRulesAll.cs @@ -8,16 +8,20 @@ namespace Security2 { public partial class FileSystemAccessRule2 { + /// + /// Removes the explicit access entries of the descriptor; with a non-null , only + /// the entries whose account is in the list (compared like IdentityReference2.Equals), leaving the rest of + /// the descriptor untouched. Before 5.0.0, the list was never applied: the filtered result was discarded, and + /// every explicit entry was removed regardless of the list, including with an empty list. + /// public static void RemoveFileSystemAccessRuleAll(FileSystemSecurity2 sd, List accounts = null) { var acl = sd.SecurityDescriptor.GetAccessRules(true, false, typeof(SecurityIdentifier)); - if (accounts != null) - { - acl.OfType().Where(ace => (accounts.Where(account => account == (IdentityReference2)ace.IdentityReference).Count() > 1)); - } + var entriesToRemove = accounts == null ? acl.OfType() : + acl.OfType().Where(ace => accounts.Any(account => account == (IdentityReference2)ace.IdentityReference)); - foreach (FileSystemAccessRule ace in acl) + foreach (FileSystemAccessRule ace in entriesToRemove.ToList()) { sd.SecurityDescriptor.RemoveAccessRuleSpecific(ace); } diff --git a/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.RemoveFileSystemAuditRuleAll.cs b/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.RemoveFileSystemAuditRuleAll.cs index ee721f0..f1872df 100644 --- a/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.RemoveFileSystemAuditRuleAll.cs +++ b/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.RemoveFileSystemAuditRuleAll.cs @@ -8,16 +8,20 @@ namespace Security2 { public partial class FileSystemAuditRule2 { + /// + /// Removes the explicit audit entries of the descriptor; with a non-null , only + /// the entries whose account is in the list (compared like IdentityReference2.Equals), leaving the rest of + /// the descriptor untouched. Before 5.0.0, the list was never applied: the filtered result was discarded, and + /// every explicit entry was removed regardless of the list, including with an empty list. + /// public static void RemoveFileSystemAuditRuleAll(FileSystemSecurity2 sd, List accounts = null) { var acl = sd.SecurityDescriptor.GetAuditRules(true, false, typeof(SecurityIdentifier)); - if (accounts != null) - { - acl.OfType().Where(ace => (accounts.Where(account => account == (IdentityReference2)ace.IdentityReference).Count() > 1)); - } + var entriesToRemove = accounts == null ? acl.OfType() : + acl.OfType().Where(ace => accounts.Any(account => account == (IdentityReference2)ace.IdentityReference)); - foreach (FileSystemAuditRule ace in acl) + foreach (FileSystemAuditRule ace in entriesToRemove.ToList()) { sd.SecurityDescriptor.RemoveAuditRuleSpecific(ace); } diff --git a/Tests/ObjectApis.Tests.ps1 b/Tests/ObjectApis.Tests.ps1 index eafb0e1..e2fc353 100644 --- a/Tests/ObjectApis.Tests.ps1 +++ b/Tests/ObjectApis.Tests.ps1 @@ -890,6 +890,171 @@ Describe 'The FileSystemSecurity2 conversions from FileSecurity and DirectorySec } } +# Before 5.0.0, the account list of both methods was built but its filter result was never assigned anywhere, so a +# non-null list removed every explicit entry regardless of what it named; only a .NET caller that passes a list is +# affected, since the cmdlets never do. +Describe 'RemoveFileSystemAccessRuleAll and RemoveFileSystemAuditRuleAll honor the account list' { + BeforeAll { + $allow = [System.Security.AccessControl.AccessControlType]::Allow + $success = [System.Security.AccessControl.AuditFlags]::Success + $accountOne = $identity + $accountTwo = [Security2.IdentityReference2] 'S-1-5-32-545' + $accountThree = [Security2.IdentityReference2] 'S-1-5-32-544' + $accountsByLabel = @{ One = $accountOne; Two = $accountTwo; Three = $accountThree } + + function New-AccessSecurityFor { + [Diagnostics.CodeAnalysis.SuppressMessageAttribute( + 'PSUseShouldProcessForStateChangingFunctions', '', Justification = 'Builds an in-memory descriptor only.' + )] + param ([Security2.IdentityReference2[]] $Accounts) + + $raw = New-Object -TypeName 'System.Security.AccessControl.FileSecurity' + foreach ($account in $Accounts) { + $raw.AddAccessRule((New-Object -TypeName 'System.Security.AccessControl.FileSystemAccessRule' -ArgumentList ( + [System.Security.Principal.SecurityIdentifier] $account, + [System.Security.AccessControl.FileSystemRights]::ReadData, $allow + ))) + } + $raw + } + + function New-AuditSecurityFor { + [Diagnostics.CodeAnalysis.SuppressMessageAttribute( + 'PSUseShouldProcessForStateChangingFunctions', '', Justification = 'Builds an in-memory descriptor only.' + )] + param ([Security2.IdentityReference2[]] $Accounts) + + $raw = New-Object -TypeName 'System.Security.AccessControl.FileSecurity' + foreach ($account in $Accounts) { + $raw.AddAuditRule((New-Object -TypeName 'System.Security.AccessControl.FileSystemAuditRule' -ArgumentList ( + [System.Security.Principal.SecurityIdentifier] $account, + [System.Security.AccessControl.FileSystemRights]::ReadData, $success + ))) + } + $raw + } + + function New-IdentityList { + [Diagnostics.CodeAnalysis.SuppressMessageAttribute( + 'PSUseShouldProcessForStateChangingFunctions', '', Justification = 'Test helper that only creates a list.' + )] + param ([Security2.IdentityReference2[]] $Accounts) + + $list = New-Object -TypeName 'System.Collections.Generic.List[Security2.IdentityReference2]' + foreach ($account in $Accounts) { + $list.Add($account) + } + , $list + } + + function Get-AccessSddl { + param ([System.Security.AccessControl.FileSecurity] $Raw) + + $Raw.GetSecurityDescriptorSddlForm([System.Security.AccessControl.AccessControlSections]::Access) + } + + function Get-AuditSddl { + param ([System.Security.AccessControl.FileSecurity] $Raw) + + $Raw.GetSecurityDescriptorSddlForm([System.Security.AccessControl.AccessControlSections]::Audit) + } + + # Builds the expected result by removing the given accounts with the .NET method directly, instead of with + # a freshly built descriptor that only holds the remaining accounts: once an entry was added and removed + # again, the SACL stays present but empty, while a SACL that never held an entry is absent, and the two give + # a different SDDL for the same set of remaining entries. + function Get-ExpectedAccessSddl { + param ([Security2.IdentityReference2[]] $AccountsToRemove) + + $raw = New-AccessSecurityFor -Accounts $accountOne, $accountTwo, $accountThree + foreach ($account in $AccountsToRemove) { + $raw.RemoveAccessRuleSpecific((New-Object -TypeName 'System.Security.AccessControl.FileSystemAccessRule' -ArgumentList ( + [System.Security.Principal.SecurityIdentifier] $account, + [System.Security.AccessControl.FileSystemRights]::ReadData, $allow + ))) + } + Get-AccessSddl $raw + } + + function Get-ExpectedAuditSddl { + param ([Security2.IdentityReference2[]] $AccountsToRemove) + + $raw = New-AuditSecurityFor -Accounts $accountOne, $accountTwo, $accountThree + foreach ($account in $AccountsToRemove) { + $raw.RemoveAuditRuleSpecific((New-Object -TypeName 'System.Security.AccessControl.FileSystemAuditRule' -ArgumentList ( + [System.Security.Principal.SecurityIdentifier] $account, + [System.Security.AccessControl.FileSystemRights]::ReadData, $success + ))) + } + Get-AuditSddl $raw + } + } + + It 'Should honor the account list when it removes access entries from a descriptor with three accounts, in memory, for ' -ForEach @( + @{ Scenario = 'a list with one account'; AccountsToRemove = @('One') } + @{ Scenario = 'a list with two accounts'; AccountsToRemove = @('One', 'Two') } + @{ Scenario = 'a null list'; AccountsToRemove = $null } + @{ Scenario = 'an empty list'; AccountsToRemove = @() } + ) { + $raw = New-AccessSecurityFor -Accounts $accountOne, $accountTwo, $accountThree + $toRemove = if ($null -eq $AccountsToRemove) { $accountOne, $accountTwo, $accountThree } else { $AccountsToRemove.ForEach({ $accountsByLabel[$_] }) } + $expected = Get-ExpectedAccessSddl -AccountsToRemove $toRemove + $accounts = if ($null -eq $AccountsToRemove) { $null } else { New-IdentityList -Accounts $AccountsToRemove.ForEach({ $accountsByLabel[$_] }) } + + [Security2.FileSystemAccessRule2]::RemoveFileSystemAccessRuleAll(([Security2.FileSystemSecurity2] $raw), $accounts) + + Get-AccessSddl $raw | Should -BeExactly $expected + } + + It 'Should honor the account list when it removes audit entries from a descriptor with three accounts, in memory, for ' -ForEach @( + @{ Scenario = 'a list with one account'; AccountsToRemove = @('One') } + @{ Scenario = 'a list with two accounts'; AccountsToRemove = @('One', 'Two') } + @{ Scenario = 'a null list'; AccountsToRemove = $null } + @{ Scenario = 'an empty list'; AccountsToRemove = @() } + ) { + $raw = New-AuditSecurityFor -Accounts $accountOne, $accountTwo, $accountThree + $toRemove = if ($null -eq $AccountsToRemove) { $accountOne, $accountTwo, $accountThree } else { $AccountsToRemove.ForEach({ $accountsByLabel[$_] }) } + $expected = Get-ExpectedAuditSddl -AccountsToRemove $toRemove + $accounts = if ($null -eq $AccountsToRemove) { $null } else { New-IdentityList -Accounts $AccountsToRemove.ForEach({ $accountsByLabel[$_] }) } + + [Security2.FileSystemAuditRule2]::RemoveFileSystemAuditRuleAll(([Security2.FileSystemSecurity2] $raw), $accounts) + + Get-AuditSddl $raw | Should -BeExactly $expected + } + + It 'Should remove only the listed account''s explicit access entries of a sandbox item, read back with .NET' { + $path = New-TestSandboxItem -Sandbox $sandbox -Name 'RemoveAllAccessByItem' + $item = New-Object -TypeName 'Alphaleonis.Win32.Filesystem.FileInfo' -ArgumentList $path + foreach ($account in $accountOne, $accountTwo, $accountThree) { + [void] [Security2.FileSystemAccessRule2]::AddFileSystemAccessRule( + $item, $account, [Security2.FileSystemRights2]::ReadData, $allow, + [System.Security.AccessControl.InheritanceFlags]::None, [System.Security.AccessControl.PropagationFlags]::None + ) + } + + [Security2.FileSystemAccessRule2]::RemoveFileSystemAccessRuleAll($item, (New-IdentityList -Accounts $accountOne)) + + $remaining = @((Get-Acl -LiteralPath $path).GetAccessRules($true, $false, [System.Security.Principal.SecurityIdentifier])) + @($remaining.IdentityReference.Value | Sort-Object) | Should -Be @(($accountTwo.Sid, $accountThree.Sid) | Sort-Object) + } + + It 'Should remove only the listed account''s explicit audit entries of a sandbox item, read back with .NET' -Skip:(-not $holdsSecurityPrivilege) { + $path = New-TestSandboxItem -Sandbox $sandbox -Name 'RemoveAllAuditByItem' + $item = New-Object -TypeName 'Alphaleonis.Win32.Filesystem.FileInfo' -ArgumentList $path + foreach ($account in $accountOne, $accountTwo, $accountThree) { + [void] [Security2.FileSystemAuditRule2]::AddFileSystemAuditRule( + $item, $account, [Security2.FileSystemRights2]::ReadData, $success, + [System.Security.AccessControl.InheritanceFlags]::None, [System.Security.AccessControl.PropagationFlags]::None + ) + } + + [Security2.FileSystemAuditRule2]::RemoveFileSystemAuditRuleAll($item, (New-IdentityList -Accounts $accountOne)) + + $remaining = @((Get-Acl -LiteralPath $path -Audit).GetAuditRules($true, $false, [System.Security.Principal.SecurityIdentifier])) + @($remaining.IdentityReference.Value | Sort-Object) | Should -Be @(($accountTwo.Sid, $accountThree.Sid) | Sort-Object) + } +} + Describe 'Generic access rights and identity errors' { It 'Should map the generic mask to the file system rights ' -ForEach @( @{ Mask = '80000000'; Expected = '00120089' }