diff --git a/CHANGELOG.md b/CHANGELOG.md index 2fbe06e..137c644 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -316,5 +316,16 @@ The format is based on `.\`, such as `.gitignore`: the cmdlets dropped its first two characters and read, changed, or removed the item with the shorter name, such as `itignore`, when one existed +- Fix comparing the objects of the access, audit, and security descriptor + cmdlets: `-eq` and `-contains`, and in PowerShell 7 also + `Select-Object -Unique` and `Compare-Object`, stopped with an + `InvalidCastException`, and a security descriptor as the key of a + hashtable with a `NullReferenceException`. Two objects are now equal when + they hold the same entry or descriptor, as in .NET +- Fix `InheritedFrom` of `Get-NTFSAccess` and `Get-NTFSAudit`: with + `-ExcludeExplicit`, each inherited entry showed the source of another + entry, and `Get-NTFSAccess -SecurityDescriptor` stopped with an + `ArgumentOutOfRangeException` for a descriptor with audit entries, such + as one that `Get-NTFSSecurityDescriptor` reads in an elevated session [Unreleased]: https://github.com/raandree/NTFSSecurity/compare/4.2.6...HEAD diff --git a/Docs/Cmdlets/Get-NTFSAccess.md b/Docs/Cmdlets/Get-NTFSAccess.md index 26ff37b..1b092c6 100644 --- a/Docs/Cmdlets/Get-NTFSAccess.md +++ b/Docs/Cmdlets/Get-NTFSAccess.md @@ -186,6 +186,8 @@ Entries whose account cannot be translated into a name are returned with their S Before 5.0.0, after a path whose ACL could not be read, the cmdlet returned the entries of the previous item again. +Before 5.0.0-rc6, with `-ExcludeExplicit`, each inherited entry showed the `InheritedFrom` path of another entry, and for a security descriptor with audit entries, such as one that `Get-NTFSSecurityDescriptor` reads in an elevated session, the cmdlet stopped with an `ArgumentOutOfRangeException`. + For the root of a drive, such as `C:\`, or of a volume, such as `\\?\Volume{GUID}\`, the cmdlets that read and change security use the root folder of the volume, like Explorer, `icacls`, and `Get-Acl`. Before 5.0.0, they read and changed the security descriptor of the drive itself, a device object with other entries. ## RELATED LINKS diff --git a/Docs/Cmdlets/Get-NTFSAudit.md b/Docs/Cmdlets/Get-NTFSAudit.md index 9294d25..bfbb742 100644 --- a/Docs/Cmdlets/Get-NTFSAudit.md +++ b/Docs/Cmdlets/Get-NTFSAudit.md @@ -185,6 +185,8 @@ If reading the audit entries is denied, the cmdlet writes a `ReadSecurityError` Before 5.0.0, the cmdlet returned no entries and no error without the Security privilege, and after a path whose security descriptor could not be read, it returned the entries of the previous item again. The `InheritanceEnabled` property of the entries also reported whether the access entries were inherited instead of the audit entries. +Before 5.0.0-rc6, with `-ExcludeExplicit`, each inherited entry showed the `InheritedFrom` path of another entry. + ## RELATED LINKS [Add-NTFSAudit](Add-NTFSAudit.md) diff --git a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml index 9a59a89..f8e25a2 100644 --- a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml +++ b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml @@ -4588,6 +4588,7 @@ PS C:\> Disable-Privileges If the ACL of an item cannot be read because access is denied, the cmdlet tries once more after making the current account the owner of the item, and restores the previous owner afterwards. Reading the owner, which that fallback needs first, requires the same Read Permissions right as reading the ACL, so the cmdlet then writes a non-terminating `ReadSecurityError` and continues with the next item. Entries whose account cannot be translated into a name are returned with their SID. Use `Get-NTFSOrphanedAccess` to list only those entries. Before 5.0.0, after a path whose ACL could not be read, the cmdlet returned the entries of the previous item again. + Before 5.0.0-rc6, with `-ExcludeExplicit`, each inherited entry showed the `InheritedFrom` path of another entry, and for a security descriptor with audit entries, such as one that `Get-NTFSSecurityDescriptor` reads in an elevated session, the cmdlet stopped with an `ArgumentOutOfRangeException`. For the root of a drive, such as `C:`, or of a volume, such as `\?\Volume{GUID}`, the cmdlets that read and change security use the root folder of the volume, like Explorer, `icacls`, and `Get-Acl`. Before 5.0.0, they read and changed the security descriptor of the drive itself, a device object with other entries. @@ -4871,6 +4872,7 @@ PS C:\> Disable-Privileges Reading the SACL requires the Security privilege (`SeSecurityPrivilege`, "Manage auditing and security log"), so run this cmdlet in an elevated session of an account that holds that privilege. Without it, the cmdlet writes the non-terminating error `ReadSecurityError` for each item, which reports "A required privilege is not held by the client". `Get-NTFSSecurityDescriptor` reads a security descriptor without its SACL when the privilege is missing; for such a descriptor, the cmdlet writes a `ReadSecurityError` as well. If reading the audit entries is denied, the cmdlet writes a `ReadSecurityError` with the category `PermissionDenied`. It doesn't take ownership of the item, because ownership grants no access to the SACL. Before 5.0.0, the cmdlet returned no entries and no error without the Security privilege, and after a path whose security descriptor could not be read, it returned the entries of the previous item again. The `InheritanceEnabled` property of the entries also reported whether the access entries were inherited instead of the audit entries. + Before 5.0.0-rc6, with `-ExcludeExplicit`, each inherited entry showed the `InheritedFrom` path of another entry. diff --git a/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.GetFileSystemAccessRules.cs b/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.GetFileSystemAccessRules.cs index 35f3bec..eb0c35c 100644 --- a/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.GetFileSystemAccessRules.cs +++ b/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.GetFileSystemAccessRules.cs @@ -21,21 +21,29 @@ namespace Security2 if (getInheritedFrom) { - inheritedFrom = Win32.GetInheritedFrom(sd.Item, sd.SecurityDescriptor); + inheritedFrom = Win32.GetInheritedFrom(sd.Item, sd.SecurityDescriptor, false); } + // All entries, so that each entry gets the source at its index in the DACL; before 5.0.0-rc6, the entries of + // -ExcludeExplicit got the sources of other entries. The filter follows. var aceCounter = 0; var acl = !sd.IsFile ? - ((DirectorySecurity)sd.SecurityDescriptor).GetAccessRules(includeExplicit, includeInherited, typeof(SecurityIdentifier)) : - ((FileSecurity)sd.SecurityDescriptor).GetAccessRules(includeExplicit, includeInherited, typeof(SecurityIdentifier)); + ((DirectorySecurity)sd.SecurityDescriptor).GetAccessRules(true, true, typeof(SecurityIdentifier)) : + ((FileSecurity)sd.SecurityDescriptor).GetAccessRules(true, true, typeof(SecurityIdentifier)); foreach (FileSystemAccessRule ace in acl) { + var source = getInheritedFrom && aceCounter < inheritedFrom.Count ? inheritedFrom[aceCounter] : null; + aceCounter++; + if (ace.IsInherited ? !includeInherited : !includeExplicit) + { + continue; + } + var ace2 = new FileSystemAccessRule2(ace) { FullName = sd.Item.FullName, InheritanceEnabled = !sd.SecurityDescriptor.AreAccessRulesProtected }; if (getInheritedFrom && inheritedFrom.Count > 0) { - ace2.inheritedFrom = string.IsNullOrEmpty(inheritedFrom[aceCounter]) ? "" : inheritedFrom[aceCounter].Substring(0, inheritedFrom[aceCounter].Length - 1); - aceCounter++; + ace2.inheritedFrom = string.IsNullOrEmpty(source) ? "" : source.Substring(0, source.Length - 1); } aceList.Add(ace2); diff --git a/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.cs b/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.cs index b48e41d..7cf84a4 100644 --- a/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.cs +++ b/Security2/FileSystem/FileSystemAccessRule2 Class/FileSystemAccessRule2.cs @@ -48,10 +48,13 @@ namespace Security2 return new FileSystemAccessRule2(ace); } - //REQUIRED BECAUSE OF CONVERSION OPERATORS + // Like the entries of .NET, two objects are equal when they wrap the same entry. A cast instead of "as" + // threw an InvalidCastException for any other type, also for -eq in PowerShell. public override bool Equals(object obj) { - return fileSystemAccessRule == (FileSystemAccessRule)obj; + var other = obj as FileSystemAccessRule2; + var rule = other != null ? other.fileSystemAccessRule : obj as FileSystemAccessRule; + return rule != null && ReferenceEquals(fileSystemAccessRule, rule); } public override int GetHashCode() { diff --git a/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.GetFileSystemAuditRules.cs b/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.GetFileSystemAuditRules.cs index 9fc525c..772c509 100644 --- a/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.GetFileSystemAuditRules.cs +++ b/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.GetFileSystemAuditRules.cs @@ -21,21 +21,29 @@ namespace Security2 if (getInheritedFrom) { - inheritedFrom = Win32.GetInheritedFrom(sd.Item, sd.SecurityDescriptor); + inheritedFrom = Win32.GetInheritedFrom(sd.Item, sd.SecurityDescriptor, true); } + // All entries, so that each entry gets the source at its index in the SACL; before 5.0.0-rc6, the entries of + // -ExcludeExplicit got the sources of other entries. The filter follows. var aceCounter = 0; var acl = !sd.IsFile ? - ((DirectorySecurity)sd.SecurityDescriptor).GetAuditRules(includeExplicit, includeInherited, typeof(SecurityIdentifier)) : - ((FileSecurity)sd.SecurityDescriptor).GetAuditRules(includeExplicit, includeInherited, typeof(SecurityIdentifier)); + ((DirectorySecurity)sd.SecurityDescriptor).GetAuditRules(true, true, typeof(SecurityIdentifier)) : + ((FileSecurity)sd.SecurityDescriptor).GetAuditRules(true, true, typeof(SecurityIdentifier)); foreach (FileSystemAuditRule ace in acl) { + var source = getInheritedFrom && aceCounter < inheritedFrom.Count ? inheritedFrom[aceCounter] : null; + aceCounter++; + if (ace.IsInherited ? !includeInherited : !includeExplicit) + { + continue; + } + var ace2 = new FileSystemAuditRule2(ace) { FullName = sd.Item.FullName, InheritanceEnabled = !sd.SecurityDescriptor.AreAuditRulesProtected }; - if (getInheritedFrom) + if (getInheritedFrom && inheritedFrom.Count > 0) { - ace2.inheritedFrom = string.IsNullOrEmpty(inheritedFrom[aceCounter]) ? "" : inheritedFrom[aceCounter].Substring(0, inheritedFrom[aceCounter].Length - 1); - aceCounter++; + ace2.inheritedFrom = string.IsNullOrEmpty(source) ? "" : source.Substring(0, source.Length - 1); } aceList.Add(ace2); diff --git a/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.cs b/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.cs index 7f951e4..1253d90 100644 --- a/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.cs +++ b/Security2/FileSystem/FileSystemAuditRule2 Class/FileSystemAuditRule2.cs @@ -48,10 +48,13 @@ namespace Security2 { return new FileSystemAuditRule2(ace); } - //REQUIRED BECAUSE OF CONVERSION OPERATORS + // Like the entries of .NET, two objects are equal when they wrap the same entry. A cast instead of "as" + // threw an InvalidCastException for any other type, also for -eq in PowerShell. public override bool Equals(object obj) { - return this.fileSystemAuditRule == (FileSystemAuditRule)obj; + var other = obj as FileSystemAuditRule2; + var rule = other != null ? other.fileSystemAuditRule : obj as FileSystemAuditRule; + return rule != null && ReferenceEquals(fileSystemAuditRule, rule); } public override int GetHashCode() { diff --git a/Security2/FileSystem/FileSystemSecurity2.cs b/Security2/FileSystem/FileSystemSecurity2.cs index 76c590d..39e5c8c 100644 --- a/Security2/FileSystem/FileSystemSecurity2.cs +++ b/Security2/FileSystem/FileSystemSecurity2.cs @@ -254,14 +254,18 @@ namespace Security2 return new FileSystemSecurity2(new DirectoryInfo("")); } - //REQUIRED BECAUSE OF CONVERSION OPERATORS + // Like the descriptors of .NET, two objects are equal when they wrap the same descriptor. The descriptor is in + // sd; fileSecurityDescriptor is never set, so its hash code threw a NullReferenceException, and the cast an + // InvalidCastException for any other type. public override bool Equals(object obj) { - return this.fileSecurityDescriptor == (FileSecurity)obj; + var other = obj as FileSystemSecurity2; + var descriptor = other != null ? other.sd : obj as FileSystemSecurity; + return descriptor != null && ReferenceEquals(sd, descriptor); } public override int GetHashCode() { - return fileSecurityDescriptor.GetHashCode(); + return sd != null ? sd.GetHashCode() : 0; } #endregion diff --git a/Security2/Win32/Lib.cs b/Security2/Win32/Lib.cs index 9df1595..46e42aa 100644 --- a/Security2/Win32/Lib.cs +++ b/Security2/Win32/Lib.cs @@ -19,44 +19,21 @@ namespace Security2 IntPtr pErrorSecObj = IntPtr.Zero; #region GetInheritedFrom - public static List GetInheritedFrom(FileSystemInfo item, ObjectSecurity sd) + // Returns the source of each entry of the DACL, or of the SACL for audit entries, in the order of the ACL. Before + // 5.0.0-rc6, a descriptor with a SACL returned the sources of the audit entries also for the access entries. + public static List GetInheritedFrom(FileSystemInfo item, ObjectSecurity sd, bool auditEntries) { var inheritedFrom = new List(); var sdBytes = sd.GetSecurityDescriptorBinaryForm(); - byte[] aclBytes = null; var rawSd = new RawSecurityDescriptor(sdBytes, 0); + var acl = auditEntries ? rawSd.SystemAcl : rawSd.DiscretionaryAcl; - var aceCount = 0; - - if (rawSd.SystemAcl != null) - { - aceCount = rawSd.SystemAcl.Count; - aclBytes = new byte[rawSd.SystemAcl.BinaryLength]; - rawSd.SystemAcl.GetBinaryForm(aclBytes, 0); - - try - { - inheritedFrom = GetInheritedFrom(item.FullName, - aclBytes, - aceCount, - item is DirectoryInfo ? true : false, - SECURITY_INFORMATION.SACL_SECURITY_INFORMATION); - } - catch - { - inheritedFrom = new List(); - for (int i = 0; i < aceCount; i++) - { - inheritedFrom.Add("unknown parent"); - } - } - } - else if (rawSd.DiscretionaryAcl != null) + if (acl != null) { - aceCount = rawSd.DiscretionaryAcl.Count; - aclBytes = new byte[rawSd.DiscretionaryAcl.BinaryLength]; - rawSd.DiscretionaryAcl.GetBinaryForm(aclBytes, 0); + var aceCount = acl.Count; + var aclBytes = new byte[acl.BinaryLength]; + acl.GetBinaryForm(aclBytes, 0); try { @@ -64,7 +41,7 @@ namespace Security2 aclBytes, aceCount, item is DirectoryInfo ? true : false, - SECURITY_INFORMATION.DACL_SECURITY_INFORMATION); + auditEntries ? SECURITY_INFORMATION.SACL_SECURITY_INFORMATION : SECURITY_INFORMATION.DACL_SECURITY_INFORMATION); } catch { diff --git a/Tests/Access.Tests.ps1 b/Tests/Access.Tests.ps1 index 097ffcc..deafdc6 100644 --- a/Tests/Access.Tests.ps1 +++ b/Tests/Access.Tests.ps1 @@ -784,3 +784,74 @@ Describe 'Clear-NTFSAccess' { } } } + +# Before 5.0.0-rc6, comparing an entry with anything threw an InvalidCastException, so -eq, -contains, and in PowerShell 7 +# also Select-Object -Unique and Compare-Object failed for the output of Get-NTFSAccess. Like the entries of .NET, two +# objects are equal when they wrap the same entry. +Describe 'Comparing access entries' { + BeforeAll { + $compareFile = New-TestSandboxItem -Sandbox $sandbox -Name 'Compare' + Add-NTFSAccess -Path $compareFile -Account 'S-1-1-0' -AccessRights ReadData + $entries = @(Get-NTFSAccess -Path $compareFile) + } + + It 'Should find an entry equal to itself and not to another entry' { + $entries.Count | Should -BeGreaterThan 1 + + $entries[0] -eq $entries[0] | Should -BeTrue + $entries[0] -eq $entries[1] | Should -BeFalse + $entries -contains $entries[1] | Should -BeTrue + $entries[0].Equals('S-1-1-0') | Should -BeFalse + $entries[0].Equals($null) | Should -BeFalse + } + + It 'Should work with Select-Object -Unique and Compare-Object' { + @(@($entries) + $entries[0] | Select-Object -Unique) | Should -HaveCount $entries.Count + Compare-Object -ReferenceObject $entries -DifferenceObject $entries | Should -BeNullOrEmpty + } +} + +# Before 5.0.0-rc6, -ExcludeExplicit gave each inherited entry the source of another entry, and Get-NTFSAccess stopped +# with an ArgumentOutOfRangeException for a security descriptor with audit entries, because it took the sources of the +# audit entries for the access entries. +Describe 'InheritedFrom of access entries' { + BeforeAll { + $inheritedFromFile = New-TestSandboxItem -Sandbox $sandbox -Name 'InheritedFrom' + Add-NTFSAccess -Path $inheritedFromFile -Account 'S-1-1-0' -AccessRights ReadData + $expectedSource = @{} + foreach ($entry in Get-NTFSAccess -Path $inheritedFromFile) { + if ($entry.IsInherited) { + $expectedSource["$($entry.Account.Sid)"] = $entry.InheritedFrom + } + } + } + + It 'Should name the folder that each inherited entry comes from' { + $expectedSource.Count | Should -BeGreaterThan 0 + $expectedSource.Values | ForEach-Object -Process { $_ | Should -Not -BeNullOrEmpty } + } + + It 'Should name the same folders with -ExcludeExplicit' { + $result = @(Get-NTFSAccess -Path $inheritedFromFile -ExcludeExplicit) + + $result | Should -HaveCount $expectedSource.Count + foreach ($entry in $result) { + $entry.InheritedFrom | Should -Be $expectedSource["$($entry.Account.Sid)"] + } + } + + It 'Should read a security descriptor with audit entries and name the same folders' -Skip:(-not $holdsSecurityPrivilege) { + $file = New-TestSandboxItem -Sandbox $sandbox -Name 'InheritedFromAudit' + Add-NTFSAccess -Path $file -Account 'S-1-1-0' -AccessRights ReadData + Add-NTFSAudit -Path $file -Account 'S-1-1-0' -AccessRights ReadData -InheritanceFlags None -PropagationFlags None + $sd = Get-NTFSSecurityDescriptor -Path $file + @($sd.SecurityDescriptor.GetAuditRules($true, $true, $sidType)) | Should -Not -BeNullOrEmpty + + $result = @(Get-NTFSAccess -SecurityDescriptor $sd -ErrorAction Stop) + + $result | Should -HaveCount @(Get-NTFSAccess -Path $file).Count + foreach ($entry in @($result | Where-Object -FilterScript { $_.IsInherited })) { + $entry.InheritedFrom | Should -Be $expectedSource["$($entry.Account.Sid)"] + } + } +} diff --git a/Tests/Audit.Tests.ps1 b/Tests/Audit.Tests.ps1 index d7dae3a..fc99457 100644 --- a/Tests/Audit.Tests.ps1 +++ b/Tests/Audit.Tests.ps1 @@ -462,3 +462,37 @@ Describe 'Clear-NTFSAudit' { } } } + +# Before 5.0.0-rc6, comparing an audit entry with anything threw an InvalidCastException. +Describe 'Comparing audit entries' { + It 'Should find an entry equal to itself and not to a string' -Skip:(-not $canReadAudit) { + $file = New-TestSandboxItem -Sandbox $sandbox -Name 'Compare' + Add-NTFSAudit -Path $file -Account 'S-1-1-0' -AccessRights ReadData -InheritanceFlags None -PropagationFlags None + $entries = @(Get-NTFSAudit -Path $file -ExcludeInherited) + $entries | Should -HaveCount 1 + + $entries[0] -eq $entries[0] | Should -BeTrue + $entries -contains $entries[0] | Should -BeTrue + $entries[0].Equals('S-1-1-0') | Should -BeFalse + } +} + +# Before 5.0.0-rc6, -ExcludeExplicit gave each inherited audit entry the source of another entry. +Describe 'InheritedFrom of audit entries' { + It 'Should name the folder that an inherited entry comes from, also with -ExcludeExplicit' -Skip:(-not $canReadAudit) { + $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'InheritedFrom' -Directory + Add-NTFSAudit -Path $folder -Account 'S-1-1-0' -AccessRights ReadData -InheritanceFlags 'ContainerInherit, ObjectInherit' -PropagationFlags None + $file = Join-Path -Path $folder -ChildPath 'File.txt' + Assert-TestSandboxPath -Sandbox $sandbox -Path $file + Set-Content -LiteralPath $file -Value 'File' + Add-NTFSAudit -Path $file -Account 'S-1-5-32-546' -AccessRights Delete -InheritanceFlags None -PropagationFlags None + + $all = @(Get-NTFSAudit -Path $file) + $inherited = @(Get-NTFSAudit -Path $file -ExcludeExplicit) + + @($all | Where-Object -FilterScript { $_.IsInherited }) | Should -HaveCount 1 + @($all | Where-Object -FilterScript { $_.IsInherited })[0].InheritedFrom | Should -Be $folder + $inherited | Should -HaveCount 1 + $inherited[0].InheritedFrom | Should -Be $folder + } +} diff --git a/Tests/SecurityDescriptor.Tests.ps1 b/Tests/SecurityDescriptor.Tests.ps1 index 3fa8050..5fad078 100644 --- a/Tests/SecurityDescriptor.Tests.ps1 +++ b/Tests/SecurityDescriptor.Tests.ps1 @@ -293,3 +293,22 @@ Describe 'FileSystemSecurity2.Write with another item' { (Get-Acl -LiteralPath $target).GetOwner($sidType).Value | Should -Be $targetOwner } } + +# Before 5.0.0-rc6, comparing a descriptor threw an InvalidCastException, and its hash code a NullReferenceException, +# so -eq and a hashtable with the descriptor as key failed. +Describe 'Comparing security descriptors' { + It 'Should find a descriptor equal to itself and not to another one, and use it as a key' { + $file = New-TestSandboxItem -Sandbox $sandbox -Name 'Compare' + $sd = Get-NTFSSecurityDescriptor -Path $file + $other = Get-NTFSSecurityDescriptor -Path $file + + $sd -eq $sd | Should -BeTrue + $sd -eq $other | Should -BeFalse + $sd.Equals('x') | Should -BeFalse + $table = @{} + $table[$sd] = 'first' + $table[$other] = 'second' + $table[$sd] | Should -Be 'first' + $table.Count | Should -Be 2 + } +}