Browse Source

fix(security2): keep a descriptor without an item usable by the cmdlets that take a security descriptor

PowerShell's parameter binder uses the conversion that 3.1 fixed 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. Two more consequences of that surfaced:

With the default module setting GetInheritedFrom ($true), Get-NTFSAccess,
Get-NTFSAudit, Get-NTFSOrphanedAccess, and Get-NTFSOrphanedAudit stopped
with the InvalidOperationException that a previous fix added for a
descriptor without an item; a cmdlet user has no parameter to avoid it.
GetFileSystemAccessRules and GetFileSystemAuditRules now treat
getInheritedFrom as false for such a descriptor instead of throwing: the
entries come back with no source, the same as an entry whose source
Windows cannot name.

The private constructor that wraps a descriptor directly marked every
section, including Audit, as read; Get-Acl never reads Audit unless
-Audit is given. This made HasAuditSection wrongly report true, so the
existing #109 check let Get-NTFSAudit go on with no audit data instead
of reporting that the descriptor has none, and let Get-NTFSInheritance
report AuditInheritanceEnabled as a real value instead of null
(unknown). The constructor now includes Audit in the sections only when
the descriptor's binary form actually carries a SACL, the same test
HasSystemAcl already uses.

Get-NTFSOwner, Get-NTFSSimpleAccess, Get-NTFSOrphanedAccess,
Get-NTFSEffectiveAccess, and Add-/Remove-NTFSAccess with such a
descriptor already worked; Set-NTFSSecurityDescriptor already reported
a clear, non-terminating error. A review finding on the earlier fix is
also addressed: two assertions read a property through reflection
instead of a plain PowerShell property access, which silently turns a
throwing getter into $null without a trace and so could not tell a
getter that throws from one that legitimately returns null.

Every existing behavior for an item that is not null is unchanged.
Public signatures are unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: AI Assistant <ai@example.com>
pull/131/head
Raimund Andree 1 week ago
parent
commit
e060c24f4d
  1. 23
      Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.GetFileSystemAccessRules.cs
  2. 23
      Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.GetFileSystemAuditRules.cs
  3. 13
      Security2/FileSystem/FileSystemSecurity2.cs
  4. 85
      Tests/ObjectApis.Tests.ps1

23
Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.GetFileSystemAccessRules.cs

@ -1,5 +1,4 @@
using Alphaleonis.Win32.Filesystem; using Alphaleonis.Win32.Filesystem;
using System;
using System.Collections.Generic; using System.Collections.Generic;
using System.Security.AccessControl; using System.Security.AccessControl;
using System.Security.Principal; using System.Security.Principal;
@ -20,17 +19,15 @@ namespace Security2
List<FileSystemAccessRule2> aceList = new List<FileSystemAccessRule2>(); List<FileSystemAccessRule2> aceList = new List<FileSystemAccessRule2>();
List<string> inheritedFrom = null; List<string> 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. Such an
// The source of an inherited entry is found from the path of the item that the descriptor was read // entry gets no source, the same as one whose source Windows could not name: a cmdlet caller has no
// from; a descriptor that a caller built directly, such as the conversion from FileSecurity, has none. // parameter to turn getInheritedFrom off (the module setting GetInheritedFrom is true by default), so an
if (sd.Item == null) // exception here would stop a cmdlet that a descriptor without an item was never meant to reach.
{ var findInheritedFrom = getInheritedFrom && 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.");
}
if (findInheritedFrom)
{
inheritedFrom = Win32.GetInheritedFrom(sd.Item, sd.SecurityDescriptor, false); inheritedFrom = Win32.GetInheritedFrom(sd.Item, sd.SecurityDescriptor, false);
} }
@ -43,7 +40,7 @@ namespace Security2
foreach (FileSystemAccessRule ace in acl) foreach (FileSystemAccessRule ace in acl)
{ {
var source = getInheritedFrom && aceCounter < inheritedFrom.Count ? inheritedFrom[aceCounter] : null; var source = findInheritedFrom && aceCounter < inheritedFrom.Count ? inheritedFrom[aceCounter] : null;
aceCounter++; aceCounter++;
if (ace.IsInherited ? !includeInherited : !includeExplicit) 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. // 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 }; 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. // Windows names a folder with a trailing backslash; the text for an unknown parent has none.
ace2.inheritedFrom = string.IsNullOrEmpty(source) ? "" : source.TrimEnd('\\'); ace2.inheritedFrom = string.IsNullOrEmpty(source) ? "" : source.TrimEnd('\\');

23
Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.GetFileSystemAuditRules.cs

@ -1,5 +1,4 @@
using Alphaleonis.Win32.Filesystem; using Alphaleonis.Win32.Filesystem;
using System;
using System.Collections.Generic; using System.Collections.Generic;
using System.Security.AccessControl; using System.Security.AccessControl;
using System.Security.Principal; using System.Security.Principal;
@ -20,17 +19,15 @@ namespace Security2
List<FileSystemAuditRule2> aceList = new List<FileSystemAuditRule2>(); List<FileSystemAuditRule2> aceList = new List<FileSystemAuditRule2>();
List<string> inheritedFrom = null; List<string> 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. Such an
// The source of an inherited entry is found from the path of the item that the descriptor was read // entry gets no source, the same as one whose source Windows could not name: a cmdlet caller has no
// from; a descriptor that a caller built directly, such as the conversion from FileSecurity, has none. // parameter to turn getInheritedFrom off (the module setting GetInheritedFrom is true by default), so an
if (sd.Item == null) // exception here would stop a cmdlet that a descriptor without an item was never meant to reach.
{ var findInheritedFrom = getInheritedFrom && 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.");
}
if (findInheritedFrom)
{
inheritedFrom = Win32.GetInheritedFrom(sd.Item, sd.SecurityDescriptor, true); inheritedFrom = Win32.GetInheritedFrom(sd.Item, sd.SecurityDescriptor, true);
} }
@ -43,7 +40,7 @@ namespace Security2
foreach (FileSystemAuditRule ace in acl) foreach (FileSystemAuditRule ace in acl)
{ {
var source = getInheritedFrom && aceCounter < inheritedFrom.Count ? inheritedFrom[aceCounter] : null; var source = findInheritedFrom && aceCounter < inheritedFrom.Count ? inheritedFrom[aceCounter] : null;
aceCounter++; aceCounter++;
if (ace.IsInherited ? !includeInherited : !includeExplicit) 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. // 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 }; 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. // Windows names a folder with a trailing backslash; the text for an unknown parent has none.
ace2.inheritedFrom = string.IsNullOrEmpty(source) ? "" : source.TrimEnd('\\'); ace2.inheritedFrom = string.IsNullOrEmpty(source) ? "" : source.TrimEnd('\\');

13
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 // 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, // DirectorySecurity do; the object has no item. Access, Owner, and Group are always part of such a
// since there was no partial read: WriteChanges compares against it as a whole. // 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) private FileSystemSecurity2(FileSystemSecurity sd)
{ {
this.sd = sd; this.sd = sd;
isFile = sd is FileSecurity; 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(); RememberSections();
} }

85
Tests/ObjectApis.Tests.ps1

@ -856,8 +856,13 @@ Describe 'The FileSystemSecurity2 conversions from FileSecurity and DirectorySec
[object]::ReferenceEquals($wrapped.SecurityDescriptor, $raw) | Should -BeTrue [object]::ReferenceEquals($wrapped.SecurityDescriptor, $raw) | Should -BeTrue
$wrapped.IsFile | Should -Be $IsFile $wrapped.IsFile | Should -Be $IsFile
[object]::ReferenceEquals(($wrapped -as $targetType), $raw) | Should -BeTrue [object]::ReferenceEquals(($wrapped -as $targetType), $raw) | Should -BeTrue
$wrapped.FullName | Should -Be $null # A plain property read turns a throwing getter into $null (see Invoke-PropertyGetter above), so these two
$wrapped.Name | Should -Be $null # 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 <Kind> conversions of the same descriptor as equal, and of different descriptors as unequal' -ForEach @( It 'Should compare two <Kind> 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' $found[0].Account.Sid | Should -BeExactly 'S-1-1-0'
} }
It 'Should throw an InvalidOperationException, not a NullReferenceException, for the source of an inherited <RuleType> 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 <RuleType> entries with no source, not throw, when getInheritedFrom is requested for a descriptor without an item' -ForEach @(
@{ RuleType = 'access' } @{ RuleType = 'access' }
@{ RuleType = 'audit' } @{ RuleType = 'audit' }
) { ) {
if ($RuleType -eq 'access') { if ($RuleType -eq 'access') {
$sd = [Security2.FileSystemSecurity2] (New-RawFileSystemSecurity -Kind 'FileSecurity') $sd = [Security2.FileSystemSecurity2] (New-RawFileSystemSecurity -Kind 'FileSecurity')
$failure = { [Security2.FileSystemAccessRule2]::GetFileSystemAccessRules($sd, $true, $true, $true) } | { [Security2.FileSystemAccessRule2]::GetFileSystemAccessRules($sd, $true, $true, $true) } | Should -Not -Throw
Should -Throw -PassThru $found = @([Security2.FileSystemAccessRule2]::GetFileSystemAccessRules($sd, $true, $true, $true))
} }
else { else {
$raw = New-RawFileSystemSecurity -Kind 'FileSecurity' $raw = New-RawFileSystemSecurity -Kind 'FileSecurity'
@ -957,12 +965,12 @@ Describe 'The FileSystemSecurity2 conversions from FileSecurity and DirectorySec
[System.Security.AccessControl.AuditFlags]::Success [System.Security.AccessControl.AuditFlags]::Success
))) )))
$sd = [Security2.FileSystemSecurity2] $raw $sd = [Security2.FileSystemSecurity2] $raw
$failure = { [Security2.FileSystemAuditRule2]::GetFileSystemAuditRules($sd, $true, $true, $true) } | { [Security2.FileSystemAuditRule2]::GetFileSystemAuditRules($sd, $true, $true, $true) } | Should -Not -Throw
Should -Throw -PassThru $found = @([Security2.FileSystemAuditRule2]::GetFileSystemAuditRules($sd, $true, $true, $true))
} }
$failure.Exception.GetBaseException() | Should -BeOfType [System.InvalidOperationException] $found | Should -HaveCount 1
$failure.Exception.GetBaseException().Message | Should -BeLike '*item*' $found[0].InheritedFrom | Should -Be $null
} }
It 'Should return an owner object with a null FullName and an empty ToString for a <Kind> without an item' -ForEach @( It 'Should return an owner object with a null FullName and an empty ToString for a <Kind> 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 # Review finding: PowerShell's binder uses the fixed conversion of 3.1 for every -SecurityDescriptor parameter, so a
# non-null list removed every explicit entry regardless of what it named; only a .NET caller that passes a list is # descriptor that Get-Acl returns, never meant for these cmdlets, now binds where it used to be a binding error. These
# affected, since the cmdlets never do. # 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' { Describe 'RemoveFileSystemAccessRuleAll and RemoveFileSystemAuditRuleAll honor the account list' {
BeforeAll { BeforeAll {
$allow = [System.Security.AccessControl.AccessControlType]::Allow $allow = [System.Security.AccessControl.AccessControlType]::Allow

Loading…
Cancel
Save