diff --git a/.memory-bank/progress.md b/.memory-bank/progress.md index a6f4ec4..d492941 100644 --- a/.memory-bank/progress.md +++ b/.memory-bank/progress.md @@ -186,6 +186,8 @@ Numbered as agreed with the maintainer; each is documented on its page. location of nine cmdlets, found by the review), #88 (an object passed by position), #17 (an entry with `GenericAll`); `Docs/FAQ.md` answers the recurring questions. +- Review of #17: generic rights are removed the way .NET removes other + rights, an exact match as it is, otherwise without `Synchronize`. - Open bugs: #5 (`-Attributes` matches all, `Get-ChildItem` any; needs a decision), #34 and #67 (writes owner and group), #41 (drive root), #82 (`Size` type data), #90 (trailing space). diff --git a/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.RemoveFileSystemAccessRules.cs b/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.RemoveFileSystemAccessRules.cs index f217ed6..71d171e 100644 --- a/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.RemoveFileSystemAccessRules.cs +++ b/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.RemoveFileSystemAccessRules.cs @@ -1,39 +1,55 @@ using Alphaleonis.Win32.Filesystem; using System.Collections.Generic; +using System.Linq; using System.Security.AccessControl; +using System.Security.Principal; namespace Security2 { public partial class FileSystemAccessRule2 { - // Generic rights, such as GENERIC_ALL, appear in inherit-only entries. FileSystemSecurity.RemoveAccessRule - // rebuilds a rule that doesn't match exactly and rejects generic rights then, so such a rule is removed - // through ModifyAccessRule, which works on the access mask (#17). - private static bool HasGenericRights(FileSystemRights2 rights) + // Rights that FileSystemAccessRule.AccessMaskFromRights rejects, such as the generic rights, which Windows + // keeps in the inherit-only entries of folders (#17). + private static bool HasUnsupportedRights(int accessMask) { - return ((long)rights & 0xF0000000L) != 0; + return accessMask < 0 || accessMask > (int)FileSystemRights.FullControl; } + // FileSystemSecurity.RemoveAccessRule removes a rule that matches an entry exactly as it is, and otherwise + // rebuilds it without the Synchronize right, which fails for unsupported rights. For those, do the same + // without rebuilding the rule. private static void RemoveRule(FileSystemSecurity sd, FileSystemAccessRule ace, bool removeSpecific) { - if (HasGenericRights((FileSystemRights2)(int)ace.FileSystemRights)) - { - bool modified; - sd.ModifyAccessRule(removeSpecific ? AccessControlModification.RemoveSpecific : AccessControlModification.Remove, ace, out modified); - } - else if (removeSpecific) - { - sd.RemoveAccessRuleSpecific(ace); - } - else + var accessMask = (int)ace.FileSystemRights; + + if (!HasUnsupportedRights(accessMask)) { - sd.RemoveAccessRule(ace); + if (removeSpecific) + sd.RemoveAccessRuleSpecific(ace); + else + sd.RemoveAccessRule(ace); + + return; } - } + var sid = (SecurityIdentifier)ace.IdentityReference.Translate(typeof(SecurityIdentifier)); + var exactMatch = sd.GetAccessRules(true, true, typeof(SecurityIdentifier)) + .OfType() + .Any(rule => (int)rule.FileSystemRights == accessMask && + rule.IdentityReference == sid && + rule.AccessControlType == ace.AccessControlType); + + var ruleToRemove = exactMatch ? ace : (FileSystemAccessRule)sd.AccessRuleFactory(sid, + accessMask & ~(int)FileSystemRights.Synchronize, false, ace.InheritanceFlags, ace.PropagationFlags, ace.AccessControlType); + + // Like RemoveAccessRule, ignore whether an entry was changed: removing an entry that doesn't exist is not + // an error. + bool modified; + sd.ModifyAccessRule(removeSpecific ? AccessControlModification.RemoveSpecific : AccessControlModification.Remove, ruleToRemove, out modified); + } public static void RemoveFileSystemAccessRule(FileSystemInfo item, IdentityReference2 account, FileSystemRights2 rights, AccessControlType type, InheritanceFlags inheritanceFlags, PropagationFlags propagationFlags, bool removeSpecific = false) { - if (type == AccessControlType.Allow && !HasGenericRights(rights)) + if (type == AccessControlType.Allow) rights = rights | FileSystemRights2.Synchronize; FileSystemAccessRule ace = null; @@ -122,18 +138,11 @@ namespace Security2 public static FileSystemAccessRule2 RemoveFileSystemAccessRule(FileSystemSecurity2 sd, IdentityReference2 account, FileSystemRights2 rights, AccessControlType type, InheritanceFlags inheritanceFlags, PropagationFlags propagationFlags, bool removeSpecific = false) { - if (type == AccessControlType.Allow && !HasGenericRights(rights)) + if (type == AccessControlType.Allow) rights = rights | FileSystemRights2.Synchronize; var ace = (FileSystemAccessRule)sd.SecurityDescriptor.AccessRuleFactory(account, (int)rights, false, inheritanceFlags, propagationFlags, type); - if (sd.IsFile) - { - RemoveRule(((FileSecurity)sd.SecurityDescriptor), ace, removeSpecific); - } - else - { - RemoveRule(((DirectorySecurity)sd.SecurityDescriptor), ace, removeSpecific); - } + RemoveRule(sd.SecurityDescriptor, ace, removeSpecific); return ace; } diff --git a/Tests/Access.Tests.ps1 b/Tests/Access.Tests.ps1 index a3e1f62..d89bd74 100644 --- a/Tests/Access.Tests.ps1 +++ b/Tests/Access.Tests.ps1 @@ -208,6 +208,73 @@ Describe 'Remove-NTFSAccess' { } } + Context 'With a generic right' { + # Before 5.0.0, removing an entry with a generic right such as GENERIC_ALL failed with "The value '269484032' is + # not valid", because .NET rebuilds the rule and rejects generic rights (#17). Windows keeps generic rights in + # inherit-only entries of folders. + BeforeAll { + $guests = [System.Security.Principal.SecurityIdentifier]'S-1-5-32-546' + + function New-GenericRightFolder { + [Diagnostics.CodeAnalysis.SuppressMessageAttribute( + 'PSUseShouldProcessForStateChangingFunctions', '', Justification = 'Test helper that only writes to the sandbox.' + )] + param ([string] $Entry) + + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'Generic' -Directory + Assert-TestSandboxPath -Sandbox $sandbox -Path $folder + $acl = Get-Acl -LiteralPath $folder + $acl.SetSecurityDescriptorSddlForm(($acl.Sddl -replace 'D:(?[A-Z]*)', ('D:${flags}' + $Entry))) + Set-Acl -LiteralPath $folder -AclObject $acl + $folder + } + + function Get-GuestsRule { + param ([string] $Path) + + (Get-Acl -LiteralPath $Path).GetAccessRules($true, $false, [System.Security.Principal.SecurityIdentifier]) | + Where-Object -Property IdentityReference -EQ -Value $guests + } + } + + It 'Should remove an inherit-only entry that Get-NTFSAccess returned, and nothing else' -ForEach @( + @{ Case = 'Allow'; Entry = '(A;OICIIO;GA;;;BG)'; Specific = $false } + @{ Case = 'Allow with -RemoveSpecific'; Entry = '(A;OICIIO;GA;;;BG)'; Specific = $true } + @{ Case = 'Deny'; Entry = '(D;OICIIO;GA;;;BG)'; Specific = $false } + ) { + $folder = New-GenericRightFolder -Entry $Entry + $before = (Get-Acl -LiteralPath $folder).Sddl + $before | Should -Match ([regex]::Escape($Entry)) + + Get-NTFSAccess -Path $folder -Account 'S-1-5-32-546' -ExcludeInherited | + Remove-NTFSAccess -RemoveSpecific:$Specific -ErrorVariable removeErrors -ErrorAction SilentlyContinue + + $removeErrors | Should -BeNullOrEmpty + (Get-Acl -LiteralPath $folder).Sddl | Should -BeExactly $before.Replace($Entry, '') + } + + It 'Should remove only the requested generic right from an entry with two' { + $folder = New-GenericRightFolder -Entry '(A;OICIIO;0x90000000;;;BG)' + + Remove-NTFSAccess -Path $folder -Account 'S-1-5-32-546' -AccessRights GenericRead -InheritanceFlags ContainerInherit, ObjectInherit -PropagationFlags InheritOnly -ErrorVariable removeErrors -ErrorAction SilentlyContinue + + $removeErrors | Should -BeNullOrEmpty + $rule = Get-GuestsRule -Path $folder + $rule | Should -HaveCount 1 + [int] $rule.FileSystemRights | Should -Be 0x10000000 + } + + # A rule that matches the entry exactly is removed as it is, with the Synchronize right that the module adds + # to an Allow rule; otherwise an entry with only Synchronize would be left behind. + It 'Should remove an entry with GenericAll and Synchronize when -AccessRights names GenericAll' { + $folder = New-GenericRightFolder -Entry '(A;OICIIO;0x10100000;;;BG)' + + Remove-NTFSAccess -Path $folder -Account 'S-1-5-32-546' -AccessRights GenericAll -InheritanceFlags ContainerInherit, ObjectInherit -PropagationFlags InheritOnly -ErrorVariable removeErrors -ErrorAction SilentlyContinue + + $removeErrors | Should -BeNullOrEmpty + Get-GuestsRule -Path $folder | Should -BeNullOrEmpty + } + } Context 'With -RemoveSpecific' { BeforeEach { $removeFolder = New-TestSandboxItem -Sandbox $sandbox -Name 'RemoveSpecific' -Directory @@ -259,25 +326,6 @@ Describe 'Add-NTFSAccess' { } } -Describe 'Remove-NTFSAccess with generic rights' { - # Before 5.0.0, removing an entry with a generic right such as GENERIC_ALL failed with "The value '269484032' is not - # valid", because .NET rebuilds the rule and rejects generic rights (#17). Windows keeps generic rights in - # inherit-only entries of folders. - It 'Should remove an inherit-only GenericAll entry that Get-NTFSAccess returned' { - $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'Generic' -Directory - Assert-TestSandboxPath -Sandbox $sandbox -Path $folder - $acl = Get-Acl -LiteralPath $folder - $acl.SetSecurityDescriptorSddlForm(($acl.Sddl -replace 'D:(?[A-Z]*)', 'D:${flags}(A;OICIIO;GA;;;S-1-5-32-546)')) - Set-Acl -LiteralPath $folder -AclObject $acl - (Get-Acl -LiteralPath $folder).Sddl | Should -Match '\(A;OICIIO;GA;;;BG\)' - - Get-NTFSAccess -Path $folder -Account 'S-1-5-32-546' -ExcludeInherited | - Remove-NTFSAccess -ErrorVariable removeErrors -ErrorAction SilentlyContinue - - $removeErrors | Should -BeNullOrEmpty - (Get-Acl -LiteralPath $folder).Sddl | Should -Not -Match ';;;BG\)' - } -} Describe 'Security descriptor parameter sets' { BeforeAll { $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'ParameterSets' -Directory