Browse Source

fix: resolve the review of the group A changes

Security review of this branch, Major findings:

- M4: Get-NTFSAudit took ownership of an item whose SACL it couldn't
  read. Ownership grants no access to the SACL, so the retry always
  failed, and it left the owner changed. The cmdlet now writes a
  ReadSecurityError with the category PermissionDenied, as Get-NTFSOwner
  does since defect 9; the page says so.
- M1: Remove-TestSandbox reset the ACLs recursively before it removed the
  links. Measured: icacls /reset /T did not follow the junction (the
  target's explicit entry stayed), but the links now go first anyway; a
  folder that denies listing gets a reset without /T. New test: the ACL
  of a junction target stays unchanged.
- m4: Assert-TestSandboxPath now rejects a path below a link, which can
  point outside the sandbox (new test, failed before).
- M5: the Inherits column reads one ACL per displayed item; the
  Get-ChildItem2 page names the cost and how to avoid it.
- M2: the comment of the CI-only Get-NTFSAudit repeat test states what it
  guards; Access.Tests.ps1 guards the same loop fix without elevation.
- M3, the stale hash of Get-FileHash2, is fixed on ai/defects-c.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: AI Assistant <ai@example.com>
pull/100/head
Raimund Andree 7 days ago
parent
commit
95332b12ca
  1. 2
      .memory-bank/progress.md
  2. 4
      CHANGELOG.md
  3. 2
      Docs/Cmdlets/Get-ChildItem2.md
  4. 2
      Docs/Cmdlets/Get-NTFSAudit.md
  5. 19
      NTFSSecurity/AuditCmdlets/GetAudit.cs
  6. 4
      NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml
  7. 5
      Tests/Audit.Tests.ps1
  8. 27
      Tests/TestHelpers.Tests.ps1
  9. 24
      Tests/TestHelpers.psm1

2
.memory-bank/progress.md

@ -122,6 +122,8 @@ Numbered as agreed with the maintainer; each is documented on its page.
`BeginProcessing` and leave them enabled.
- (13) Format view `Children2`: the `Inherits` column uses
`IsInheritanceBlocked`, so it always shows `True` for `Get-ChildItem2`.
- Review of group A: five Major findings fixed in the last commit; the
Minor ones are listed in the PR description.
#### B: Ignored parameters and parameter sets

4
CHANGELOG.md

@ -61,7 +61,9 @@ The format is based on
the files that followed it; folders are now skipped
- Fix `Get-NTFSAudit`, which returned nothing without the Security privilege
instead of an error, and which returned the entries of the previous item
again after a path whose security descriptor it couldn't read
again after a path whose security descriptor it couldn't read; it also no
longer takes ownership of an item whose audit entries it can't read, which
didn't help and could leave the owner changed
- Fix `Get-NTFSAccess`, which returned the entries of the previous item again
after a path whose ACL it couldn't read
- Fix `Add-NTFSAudit`, whose `-Account` and `-AccessRights` parameters were

2
Docs/Cmdlets/Get-ChildItem2.md

@ -301,7 +301,7 @@ The cmdlet returns this object for every folder it finds. Depending on the modul
The module defines the alias `dir2` for this cmdlet.
The default table view shows the `Mode`, `Inherits`, `LastWriteTime`, `Size(M)`, and `Name` columns. `Inherits` is `False` for an item whose access inheritance is disabled. Before 5.0.0, the column showed `True` for every item.
The default table view shows the `Mode`, `Inherits`, `LastWriteTime`, `Size(M)`, and `Name` columns. `Inherits` is `False` for an item whose access inheritance is disabled. Reading that value costs one access to the ACL of each displayed item, which slows down the display of large listings; to avoid it, select the properties you need, for example with `Format-Table -Property Mode, LastWriteTime, Length, Name`. Objects that you pipe to another command are not affected. Before 5.0.0, the column showed `True` for every item.
The `PrivateData` section of the module manifest `NTFSSecurity.psd1` contains two settings that this cmdlet reads when it starts. `GetFileSystemModeProperty` adds the calculated `Mode` property to every item. `IdentifyHardLinks` adds the `HardLinkCount` property to every file, which requires an extra call into the file system for each file and therefore slows down large listings noticeably. Set either value to `$false` in the manifest and import the module again if you prefer the faster enumeration over the additional properties.

2
Docs/Cmdlets/Get-NTFSAudit.md

@ -181,7 +181,7 @@ When the module setting `EnablePrivileges` is `$true` (the default in the `Priva
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 the security descriptor cannot be read because access is denied, the cmdlet takes ownership of the item, reads the descriptor again, and restores the previous owner. If the second attempt fails as well, the cmdlet writes an error, and the ownership change is not rolled back.
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.

19
NTFSSecurity/AuditCmdlets/GetAudit.cs

@ -100,22 +100,11 @@ namespace NTFSSecurity
{
acl = GetAuditRules(item);
}
catch (UnauthorizedAccessException)
catch (UnauthorizedAccessException ex)
{
try
{
var ownerInfo = FileSystemOwner.GetOwner(item);
var previousOwner = ownerInfo.Owner;
FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User);
acl = GetAuditRules(item);
FileSystemOwner.SetOwner(item, previousOwner);
}
catch (Exception ex2)
{
WriteError(new ErrorRecord(ex2, "ReadSecurityError", ErrorCategory.WriteError, path));
continue;
}
// Taking ownership grants no access to the SACL, so it wouldn't help, and it would change the owner.
WriteError(new ErrorRecord(ex, "ReadSecurityError", ErrorCategory.PermissionDenied, path));
continue;
}
catch (Exception ex)
{

4
NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml

@ -3841,7 +3841,7 @@ PS C:\&gt; Disable-Privileges</dev:code>
<maml:alert>
<maml:para>`Get-ChildItem2` enumerates the file system through the AlphaFS library (`Alphaleonis.Win32.Filesystem`), which is why it returns items whose path exceeds the 260-character `MAX_PATH` limit that the built-in `Get-ChildItem` cmdlet is bound to. The objects are AlphaFS objects, not `System.IO` objects, and the other NTFSSecurity cmdlets accept them directly because their `-Path` parameters have the alias `FullName`.</maml:para>
<maml:para>The module defines the alias `dir2` for this cmdlet.</maml:para>
<maml:para>The default table view shows the `Mode`, `Inherits`, `LastWriteTime`, `Size(M)`, and `Name` columns. `Inherits` is `False` for an item whose access inheritance is disabled. Before 5.0.0, the column showed `True` for every item.</maml:para>
<maml:para>The default table view shows the `Mode`, `Inherits`, `LastWriteTime`, `Size(M)`, and `Name` columns. `Inherits` is `False` for an item whose access inheritance is disabled. Reading that value costs one access to the ACL of each displayed item, which slows down the display of large listings; to avoid it, select the properties you need, for example with `Format-Table -Property Mode, LastWriteTime, Length, Name`. Objects that you pipe to another command are not affected. Before 5.0.0, the column showed `True` for every item.</maml:para>
<maml:para>The `PrivateData` section of the module manifest `NTFSSecurity.psd1` contains two settings that this cmdlet reads when it starts. `GetFileSystemModeProperty` adds the calculated `Mode` property to every item. `IdentifyHardLinks` adds the `HardLinkCount` property to every file, which requires an extra call into the file system for each file and therefore slows down large listings noticeably. Set either value to `$false` in the manifest and import the module again if you prefer the faster enumeration over the additional properties.</maml:para>
<maml:para>A folder that cannot be read produces a non-terminating error with the ID `DirUnauthorizedAccessError` for an access denial or `DirUnspecifiedError` for any other failure, and a path that does not exist produces the error `FileNotFound`. In each case the cmdlet continues with the next path. Failures that occur while `-Recurse` collects the subfolders of a folder are reported as verbose messages only, not as errors.</maml:para>
<maml:para>Before 5.0.0, a `-Path` value that points to a file stopped the cmdlet with an `InvalidCastException`.</maml:para>
@ -4847,7 +4847,7 @@ PS C:\&gt; Disable-Privileges</dev:code>
<maml:alert>
<maml:para>When the module setting `EnablePrivileges` is `$true` (the default in the `PrivateData` section of NTFSSecurity.psd1), this cmdlet tries to enable the Backup, Restore, Take Ownership, and Security privileges while it runs and disables the privileges it enabled when it finishes. These privileges are only available in an elevated session of an account that holds them, such as a member of the local Administrators group. If a privilege cannot be enabled, the cmdlet continues without it and writes a debug message.</maml:para>
<maml:para>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.</maml:para>
<maml:para>If the security descriptor cannot be read because access is denied, the cmdlet takes ownership of the item, reads the descriptor again, and restores the previous owner. If the second attempt fails as well, the cmdlet writes an error, and the ownership change is not rolled back.</maml:para>
<maml:para>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.</maml:para>
<maml:para>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.</maml:para>
</maml:alert>
</maml:alertSet>

5
Tests/Audit.Tests.ps1

@ -54,7 +54,10 @@ Describe 'Get-NTFSAudit' {
}
Context 'When a path fails after a path with audit entries' {
# Before 5.0.0, the cmdlet wrote the entries of the previous item again for the failing path.
# Before 5.0.0, the cmdlet wrote the entries of the previous item again for the failing path. In CI, the deny
# entry made the original implementation, which also read the DACL, fail for the second path. The current one
# reads only the SACL, which the deny entry doesn't block; Access.Tests.ps1 guards the same loop fix in
# Get-NTFSAccess with a read that fails without elevation.
It 'Should return the entries of the first item once' -Skip:(-not $canReadAudit) {
$folder = New-TestSandboxItem -Sandbox $sandbox -Name 'Audited' -Directory
$denied = New-TestSandboxItem -Sandbox $sandbox -Name 'Denied'

27
Tests/TestHelpers.Tests.ps1

@ -67,6 +67,21 @@ Describe 'Test helpers' {
{ Assert-TestSandboxPath -Sandbox $env:TEMP -Path (Join-Path -Path $env:TEMP -ChildPath 'File.txt') } |
Should -Throw -ExpectedMessage '*is not a test sandbox*'
}
It 'Should reject a path below a link, which can point outside the sandbox' {
$otherSandbox = New-TestSandbox -Name 'Helpers'
try {
$link = Join-Path -Path $sandbox -ChildPath 'Link'
Assert-TestSandboxPath -Sandbox $sandbox -Path $link
New-Item -ItemType Junction -Path $link -Value $otherSandbox | Out-Null
{ Assert-TestSandboxPath -Sandbox $sandbox -Path (Join-Path -Path $link -ChildPath 'File.txt') } |
Should -Throw -ExpectedMessage '*is a link*'
}
finally {
Remove-TestSandbox -Sandbox $otherSandbox
}
}
}
Context 'Remove-TestSandbox' {
@ -80,6 +95,14 @@ Describe 'Test helpers' {
Assert-TestSandboxPath -Sandbox $otherSandbox -Path $target
New-Item -ItemType Directory -Path $target | Out-Null
Set-Content -LiteralPath (Join-Path -Path $target -ChildPath 'Keep.txt') -Value 'Keep'
# An explicit entry that a reset through the link would remove
$targetAcl = Get-Acl -LiteralPath $target
$targetAcl.AddAccessRule((New-Object -TypeName 'System.Security.AccessControl.FileSystemAccessRule' -ArgumentList (
(New-Object -TypeName 'System.Security.Principal.SecurityIdentifier' -ArgumentList 'S-1-1-0'),
[System.Security.AccessControl.FileSystemRights]::ReadData, [System.Security.AccessControl.AccessControlType]::Allow
)))
Set-Acl -LiteralPath $target -AclObject $targetAcl
$targetSddl = (Get-Acl -LiteralPath $target).Sddl
Assert-TestSandboxPath -Sandbox $sandbox -Path $link, $locked
New-Item -ItemType Junction -Path $link -Value $target | Out-Null
New-Item -ItemType Directory -Path $locked | Out-Null
@ -101,6 +124,10 @@ Describe 'Test helpers' {
It 'Should remove a junction without removing the files of its target' {
Join-Path -Path $target -ChildPath 'Keep.txt' | Should -Exist
}
It 'Should not change the ACL of the target of a junction' {
(Get-Acl -LiteralPath $target).Sddl | Should -BeExactly $targetSddl
}
}
Context 'Test-IsElevated and Test-PrivilegeHeld' {

24
Tests/TestHelpers.psm1

@ -61,6 +61,16 @@ function Assert-TestSandboxPath {
if (-not ($fullName + '\').StartsWith($sandboxPrefix, [StringComparison]::OrdinalIgnoreCase)) {
throw "Refusing to change '$fullName', which is outside the test sandbox '$sandboxFullName'."
}
# A link inside the sandbox can point outside of it, so no folder of the path may be a link.
$parent = [IO.Path]::GetDirectoryName($fullName)
while ($parent -and $parent.Length -gt $sandboxFullName.Length) {
if ([IO.Directory]::Exists($parent) -and
([IO.File]::GetAttributes($parent) -band [IO.FileAttributes]::ReparsePoint)) {
throw "Refusing to change '$fullName', because its folder '$parent' is a link."
}
$parent = [IO.Path]::GetDirectoryName($parent)
}
}
}
}
@ -90,12 +100,20 @@ function Remove-TestSandbox {
# the caller uses ErrorAction Stop. Such items can still be deleted through the rights on their folder.
$ErrorActionPreference = 'Continue'
# Windows PowerShell 5.1 follows directory links when it removes a folder recursively, so the links go first.
& icacls.exe $Sandbox /reset /T /C /Q *> $null
# Windows PowerShell 5.1 and icacls /T follow directory links, so the links go first. A folder that denies
# listing its content gets its own ACL reset, without /T, before it is listed.
$pending = New-Object -TypeName 'System.Collections.Generic.Stack[string]'
$pending.Push($Sandbox)
while ($pending.Count -gt 0) {
foreach ($entry in [IO.Directory]::GetFileSystemEntries($pending.Pop())) {
$folder = $pending.Pop()
try {
$entries = [IO.Directory]::GetFileSystemEntries($folder)
}
catch {
& icacls.exe $folder /reset /C /Q *> $null
$entries = [IO.Directory]::GetFileSystemEntries($folder)
}
foreach ($entry in $entries) {
$attributes = [IO.File]::GetAttributes($entry)
if ($attributes -band [IO.FileAttributes]::ReparsePoint) {
if ($attributes -band [IO.FileAttributes]::Directory) {

Loading…
Cancel
Save