diff --git a/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.GetFileSystemAccessRules.cs b/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.GetFileSystemAccessRules.cs index 6e4f65f..0408646 100644 --- a/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.GetFileSystemAccessRules.cs +++ b/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.GetFileSystemAccessRules.cs @@ -1,5 +1,4 @@ using Alphaleonis.Win32.Filesystem; -using System; using System.Collections.Generic; using System.Security.AccessControl; using System.Security.Principal; @@ -20,17 +19,15 @@ namespace Security2 List aceList = new List(); List inheritedFrom = null; - if (getInheritedFrom) - { - // The source of an inherited entry is found from the path of the item that the descriptor was read - // from; a descriptor that a caller built directly, such as the conversion from FileSecurity, has none. - if (sd.Item == null) - { - throw new InvalidOperationException( - "Cannot find the source of an inherited entry: this security descriptor has no item, so there " + - "is no path to search from. Pass getInheritedFrom as false, or read the entries from an item."); - } + // The source of an inherited entry is found from the path of the item that the descriptor was read from; + // a descriptor that a caller built directly, such as the conversion from FileSecurity, has none. Such an + // entry gets no source, the same as one whose source Windows could not name: a cmdlet caller has no + // parameter to turn getInheritedFrom off (the module setting GetInheritedFrom is true by default), so an + // exception here would stop a cmdlet that a descriptor without an item was never meant to reach. + var findInheritedFrom = getInheritedFrom && sd.Item != null; + if (findInheritedFrom) + { inheritedFrom = Win32.GetInheritedFrom(sd.Item, sd.SecurityDescriptor, false); } @@ -43,7 +40,7 @@ namespace Security2 foreach (FileSystemAccessRule ace in acl) { - var source = getInheritedFrom && aceCounter < inheritedFrom.Count ? inheritedFrom[aceCounter] : null; + var source = findInheritedFrom && aceCounter < inheritedFrom.Count ? inheritedFrom[aceCounter] : null; aceCounter++; if (ace.IsInherited ? !includeInherited : !includeExplicit) { @@ -52,7 +49,7 @@ namespace Security2 // sd.FullName, not sd.Item.FullName: null without an item instead of a NullReferenceException. var ace2 = new FileSystemAccessRule2(ace) { FullName = sd.FullName, InheritanceEnabled = !sd.SecurityDescriptor.AreAccessRulesProtected }; - if (getInheritedFrom && inheritedFrom.Count > 0) + if (findInheritedFrom && inheritedFrom.Count > 0) { // Windows names a folder with a trailing backslash; the text for an unknown parent has none. ace2.inheritedFrom = string.IsNullOrEmpty(source) ? "" : source.TrimEnd('\\'); diff --git a/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.GetFileSystemAuditRules.cs b/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.GetFileSystemAuditRules.cs index 2d3d27c..5df90c9 100644 --- a/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.GetFileSystemAuditRules.cs +++ b/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.GetFileSystemAuditRules.cs @@ -1,5 +1,4 @@ using Alphaleonis.Win32.Filesystem; -using System; using System.Collections.Generic; using System.Security.AccessControl; using System.Security.Principal; @@ -20,17 +19,15 @@ namespace Security2 List aceList = new List(); List inheritedFrom = null; - if (getInheritedFrom) - { - // The source of an inherited entry is found from the path of the item that the descriptor was read - // from; a descriptor that a caller built directly, such as the conversion from FileSecurity, has none. - if (sd.Item == null) - { - throw new InvalidOperationException( - "Cannot find the source of an inherited entry: this security descriptor has no item, so there " + - "is no path to search from. Pass getInheritedFrom as false, or read the entries from an item."); - } + // The source of an inherited entry is found from the path of the item that the descriptor was read from; + // a descriptor that a caller built directly, such as the conversion from FileSecurity, has none. Such an + // entry gets no source, the same as one whose source Windows could not name: a cmdlet caller has no + // parameter to turn getInheritedFrom off (the module setting GetInheritedFrom is true by default), so an + // exception here would stop a cmdlet that a descriptor without an item was never meant to reach. + var findInheritedFrom = getInheritedFrom && sd.Item != null; + if (findInheritedFrom) + { inheritedFrom = Win32.GetInheritedFrom(sd.Item, sd.SecurityDescriptor, true); } @@ -43,7 +40,7 @@ namespace Security2 foreach (FileSystemAuditRule ace in acl) { - var source = getInheritedFrom && aceCounter < inheritedFrom.Count ? inheritedFrom[aceCounter] : null; + var source = findInheritedFrom && aceCounter < inheritedFrom.Count ? inheritedFrom[aceCounter] : null; aceCounter++; if (ace.IsInherited ? !includeInherited : !includeExplicit) { @@ -52,7 +49,7 @@ namespace Security2 // sd.FullName, not sd.Item.FullName: null without an item instead of a NullReferenceException. var ace2 = new FileSystemAuditRule2(ace) { FullName = sd.FullName, InheritanceEnabled = !sd.SecurityDescriptor.AreAuditRulesProtected }; - if (getInheritedFrom && inheritedFrom.Count > 0) + if (findInheritedFrom && inheritedFrom.Count > 0) { // Windows names a folder with a trailing backslash; the text for an unknown parent has none. ace2.inheritedFrom = string.IsNullOrEmpty(source) ? "" : source.TrimEnd('\\'); diff --git a/Security2/FileSystem/FileSystemSecurity2.cs b/Security2/FileSystem/FileSystemSecurity2.cs index 9e29088..6e5b71e 100644 --- a/Security2/FileSystem/FileSystemSecurity2.cs +++ b/Security2/FileSystem/FileSystemSecurity2.cs @@ -43,13 +43,20 @@ namespace Security2 } // Wraps a descriptor that a caller created directly, such as the conversions from FileSecurity and - // DirectorySecurity do; the object has no item. The descriptor is treated as if every section had been read, - // since there was no partial read: WriteChanges compares against it as a whole. + // DirectorySecurity do; the object has no item. Access, Owner, and Group are always part of such a + // descriptor; Audit is part of it only when the descriptor actually carries a SACL (the same test as + // HasSystemAcl), since there was no read to tell sections apart otherwise. Without this, a descriptor that + // Get-Acl returned without -Audit reported HasAuditSection as true, so TestAuditSection (#109) let the audit + // cmdlets go on with no audit data at all, instead of reporting that the descriptor has none. private FileSystemSecurity2(FileSystemSecurity sd) { this.sd = sd; isFile = sd is FileSecurity; - sections = AccessControlSections.All; + sections = AccessControlSections.Access | AccessControlSections.Owner | AccessControlSections.Group; + if (new RawSecurityDescriptor(sd.GetSecurityDescriptorBinaryForm(), 0).SystemAcl != null) + { + sections |= AccessControlSections.Audit; + } RememberSections(); } diff --git a/Tests/ObjectApis.Tests.ps1 b/Tests/ObjectApis.Tests.ps1 index 9f13b07..2312e85 100644 --- a/Tests/ObjectApis.Tests.ps1 +++ b/Tests/ObjectApis.Tests.ps1 @@ -856,8 +856,13 @@ Describe 'The FileSystemSecurity2 conversions from FileSecurity and DirectorySec [object]::ReferenceEquals($wrapped.SecurityDescriptor, $raw) | Should -BeTrue $wrapped.IsFile | Should -Be $IsFile [object]::ReferenceEquals(($wrapped -as $targetType), $raw) | Should -BeTrue - $wrapped.FullName | Should -Be $null - $wrapped.Name | Should -Be $null + # A plain property read turns a throwing getter into $null (see Invoke-PropertyGetter above), so these two + # getters are invoked through reflection: a null FullName/Name is pinned as the getter's own return value, + # not as whatever a getter that happened to throw would also look like from here. + { Invoke-PropertyGetter -InputObject $wrapped -PropertyName 'FullName' } | Should -Not -Throw + { Invoke-PropertyGetter -InputObject $wrapped -PropertyName 'Name' } | Should -Not -Throw + (Invoke-PropertyGetter -InputObject $wrapped -PropertyName 'FullName') | Should -Be $null + (Invoke-PropertyGetter -InputObject $wrapped -PropertyName 'Name') | Should -Be $null } It 'Should compare two conversions of the same descriptor as equal, and of different descriptors as unequal' -ForEach @( @@ -941,14 +946,17 @@ Describe 'The FileSystemSecurity2 conversions from FileSecurity and DirectorySec $found[0].Account.Sid | Should -BeExactly 'S-1-1-0' } - It 'Should throw an InvalidOperationException, not a NullReferenceException, for the source of an inherited entry of a descriptor without an item' -ForEach @( + # Before this fix, a descriptor without an item made getInheritedFrom throw an InvalidOperationException, which a + # cmdlet caller has no parameter to avoid (the module setting GetInheritedFrom is $true by default): getInheritedFrom + # is now treated as false for such a descriptor instead, the same as an entry whose source Windows cannot name. + It 'Should return the entries with no source, not throw, when getInheritedFrom is requested for a descriptor without an item' -ForEach @( @{ RuleType = 'access' } @{ RuleType = 'audit' } ) { if ($RuleType -eq 'access') { $sd = [Security2.FileSystemSecurity2] (New-RawFileSystemSecurity -Kind 'FileSecurity') - $failure = { [Security2.FileSystemAccessRule2]::GetFileSystemAccessRules($sd, $true, $true, $true) } | - Should -Throw -PassThru + { [Security2.FileSystemAccessRule2]::GetFileSystemAccessRules($sd, $true, $true, $true) } | Should -Not -Throw + $found = @([Security2.FileSystemAccessRule2]::GetFileSystemAccessRules($sd, $true, $true, $true)) } else { $raw = New-RawFileSystemSecurity -Kind 'FileSecurity' @@ -957,12 +965,12 @@ Describe 'The FileSystemSecurity2 conversions from FileSecurity and DirectorySec [System.Security.AccessControl.AuditFlags]::Success ))) $sd = [Security2.FileSystemSecurity2] $raw - $failure = { [Security2.FileSystemAuditRule2]::GetFileSystemAuditRules($sd, $true, $true, $true) } | - Should -Throw -PassThru + { [Security2.FileSystemAuditRule2]::GetFileSystemAuditRules($sd, $true, $true, $true) } | Should -Not -Throw + $found = @([Security2.FileSystemAuditRule2]::GetFileSystemAuditRules($sd, $true, $true, $true)) } - $failure.Exception.GetBaseException() | Should -BeOfType [System.InvalidOperationException] - $failure.Exception.GetBaseException().Message | Should -BeLike '*item*' + $found | Should -HaveCount 1 + $found[0].InheritedFrom | Should -Be $null } It 'Should return an owner object with a null FullName and an empty ToString for a without an item' -ForEach @( @@ -1018,9 +1026,62 @@ 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. +# Review finding: PowerShell's binder uses the fixed conversion of 3.1 for every -SecurityDescriptor parameter, so a +# descriptor that Get-Acl returns, never meant for these cmdlets, now binds where it used to be a binding error. These +# pin the four cmdlets the review named; the one-line probes of the others are in the final report, not here. +Describe 'Cmdlets with a security descriptor that Get-Acl returns' { + BeforeAll { + $plainFile = New-TestSandboxItem -Sandbox $sandbox -Name 'GetAclPlain' + Add-NTFSAccess -Path $plainFile -Account 'S-1-1-0' -AccessRights ReadData -AppliesTo ThisFolderOnly + } + + It 'Should return the explicit access entries and no error for Get-NTFSAccess -SecurityDescriptor (Get-Acl ...)' { + $sd = Get-Acl -LiteralPath $plainFile + $expected = @($sd.GetAccessRules($true, $false, [System.Security.Principal.SecurityIdentifier])) + + $result = @(Get-NTFSAccess -SecurityDescriptor $sd -ExcludeInherited -ErrorVariable accessErrors -ErrorAction SilentlyContinue) + + $accessErrors | Should -BeNullOrEmpty + ($result.Account.Sid | Sort-Object) | Should -Be ($expected.IdentityReference.Value | Sort-Object) + } + + It 'Should write the #109 ReadSecurityError, not an empty result, for Get-NTFSAudit -SecurityDescriptor (Get-Acl ...) without -Audit' { + $sd = Get-Acl -LiteralPath $plainFile + + $result = @(Get-NTFSAudit -SecurityDescriptor $sd -ErrorVariable auditErrors -ErrorAction SilentlyContinue) + + $result | Should -BeNullOrEmpty + $auditErrors | Should -HaveCount 1 + $auditErrors[0].FullyQualifiedErrorId | Should -BeLike 'ReadSecurityError,*' + $auditErrors[0].CategoryInfo.Category | Should -Be 'InvalidData' + $auditErrors[0].Exception.Message | Should -BeLike "*doesn't contain the audit entries*" + } + + It 'Should give a null AuditInheritanceEnabled and the right AccessInheritanceEnabled for Get-NTFSInheritance -SecurityDescriptor (Get-Acl ...)' { + $sd = Get-Acl -LiteralPath $plainFile + + $result = Get-NTFSInheritance -SecurityDescriptor $sd -ErrorAction Stop + + $result.AuditInheritanceEnabled | Should -Be $null + $result.AccessInheritanceEnabled | Should -Be (-not $sd.AreAccessRulesProtected) + } + + It 'Should write a non-terminating error naming the missing item for Set-NTFSSecurityDescriptor -SecurityDescriptor (Get-Acl ...), and change nothing' { + $sd = Get-Acl -LiteralPath $plainFile + $sd.AddAccessRule((New-Object -TypeName 'System.Security.AccessControl.FileSystemAccessRule' -ArgumentList ( + $sid, [System.Security.AccessControl.FileSystemRights]::WriteData, + [System.Security.AccessControl.AccessControlType]::Allow + ))) + $before = (Get-Acl -LiteralPath $plainFile).Sddl + + $null = Set-NTFSSecurityDescriptor -SecurityDescriptor $sd -ErrorVariable sdErrors -ErrorAction SilentlyContinue + + $sdErrors | Should -HaveCount 1 + $sdErrors[0].FullyQualifiedErrorId | Should -BeLike 'WriteSdError,*' + $sdErrors[0].Exception.Message | Should -BeLike '*no item*' + (Get-Acl -LiteralPath $plainFile).Sddl | Should -BeExactly $before + } +} Describe 'RemoveFileSystemAccessRuleAll and RemoveFileSystemAuditRuleAll honor the account list' { BeforeAll { $allow = [System.Security.AccessControl.AccessControlType]::Allow