From ac9e57e349d4db9e61737813e44d9bfa3fc86da8 Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Mon, 5 Oct 2026 00:15:00 +0200 Subject: [PATCH] fix: report unreadable audit entries in Get-NTFSAudit Defect 4, both parts: - Get-NTFSAudit kept the entries of the previous item and wrote them in a finally block, so a path whose security descriptor failed to read returned the previous item's entries again. Each item now starts empty, and entries are written only after a successful read. - Without the Security privilege, the cmdlet read the descriptor without its SACL and returned nothing, like an item without audit entries. It now reads the SACL alone, so a missing privilege is a ReadSecurityError ("A required privilege is not held by the client"). A descriptor from Get-NTFSSecurityDescriptor that was read without the SACL gets the same error; FileSystemSecurity2 now records which sections it read (internal, visible to NTFSSecurity). Tests/Audit.Tests.ps1 (new): 3 tests. The repeat test needs the Security privilege to add an audit entry and runs in CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- CHANGELOG.md | 3 + Docs/Cmdlets/Get-NTFSAudit.md | 6 +- NTFSSecurity/AuditCmdlets/GetAudit.cs | 50 ++++++---- NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml | 5 +- Security2/FileSystem/FileSystemSecurity2.cs | 12 +++ Security2/Properties/AssemblyInfo.cs | 3 + Tests/Audit.Tests.ps1 | 100 +++++++++++++++++++ 7 files changed, 154 insertions(+), 25 deletions(-) create mode 100644 Tests/Audit.Tests.ps1 diff --git a/CHANGELOG.md b/CHANGELOG.md index b9ceb6e..f69ca96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,5 +59,8 @@ The format is based on `-Path` pointed to a file; it now returns the file, like `Get-ChildItem` - Fix `Get-FileHash2`, which stopped at a folder in `-Path` and didn't hash 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 [Unreleased]: https://github.com/raandree/NTFSSecurity/compare/4.2.6...HEAD diff --git a/Docs/Cmdlets/Get-NTFSAudit.md b/Docs/Cmdlets/Get-NTFSAudit.md index 03ec754..b010e7f 100644 --- a/Docs/Cmdlets/Get-NTFSAudit.md +++ b/Docs/Cmdlets/Get-NTFSAudit.md @@ -173,16 +173,18 @@ You can pass an account name or a SID string to `-Account`, which the cmdlet con ### Security2.FileSystemAuditRule2 -The cmdlet returns one object per audit entry, with the audited account, the audited access rights, the audit flags, the inheritance and propagation flags, the `IsInherited` flag, and the `InheritedFrom` path. When an item has no audit entries, or when the SACL cannot be read, the cmdlet returns nothing for that item. +The cmdlet returns one object per audit entry, with the audited account, the audited access rights, the audit flags, the inheritance and propagation flags, the `IsInherited` flag, and the `InheritedFrom` path. When an item has no audit entries, the cmdlet returns nothing for that item; when its SACL cannot be read, the cmdlet writes an error. ## NOTES 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. -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 falls back to reading the security descriptor without its SACL; it then returns no audit entries and reports no error, which looks the same as an item that is not audited at all. +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. +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. + ## RELATED LINKS [Add-NTFSAudit](Add-NTFSAudit.md) diff --git a/NTFSSecurity/AuditCmdlets/GetAudit.cs b/NTFSSecurity/AuditCmdlets/GetAudit.cs index 394417e..6531274 100644 --- a/NTFSSecurity/AuditCmdlets/GetAudit.cs +++ b/NTFSSecurity/AuditCmdlets/GetAudit.cs @@ -79,13 +79,13 @@ namespace NTFSSecurity protected override void ProcessRecord() { - IEnumerable acl = null; - FileSystemInfo item = null; - if (ParameterSetName == "Path") { foreach (var path in paths) { + FileSystemInfo item = null; + IEnumerable acl = null; + try { item = GetFileSystemInfo2(path); @@ -98,7 +98,7 @@ namespace NTFSSecurity try { - acl = FileSystemAuditRule2.GetFileSystemAuditRules(item, !excludeExplicit, !excludeInherited, getInheritedFrom); + acl = GetAuditRules(item); } catch (UnauthorizedAccessException) { @@ -108,7 +108,7 @@ namespace NTFSSecurity var previousOwner = ownerInfo.Owner; FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User); - acl = FileSystemAuditRule2.GetFileSystemAuditRules(item, !excludeExplicit, !excludeInherited, getInheritedFrom); + acl = GetAuditRules(item); FileSystemOwner.SetOwner(item, previousOwner); } catch (Exception ex2) @@ -122,34 +122,42 @@ namespace NTFSSecurity WriteError(new ErrorRecord(ex, "ReadSecurityError", ErrorCategory.OpenError, path)); continue; } - finally - { - if (acl != null) - { - if (account != null) - { - acl = acl.Where(ace => ace.Account == account); - } - acl.ForEach(ace => WriteObject(ace)); - } - } + WriteAuditRules(acl); } } else { foreach (var sd in securityDescriptors) { - acl = FileSystemAuditRule2.GetFileSystemAuditRules(sd, !excludeExplicit, !excludeInherited, getInheritedFrom); - - if (account != null) + if (!sd.HasAuditSection) { - acl = acl.Where(ace => ace.Account == account); + var ex = new InvalidOperationException(string.Format( + "The security descriptor of '{0}' doesn't contain the audit entries, because it was read without the Security privilege.", sd.FullName)); + WriteError(new ErrorRecord(ex, "ReadSecurityError", ErrorCategory.InvalidData, sd)); + continue; } - acl.ForEach(ace => WriteObject(ace)); + WriteAuditRules(FileSystemAuditRule2.GetFileSystemAuditRules(sd, !excludeExplicit, !excludeInherited, getInheritedFrom)); } } } + + private IEnumerable GetAuditRules(FileSystemInfo item) + { + // Reading only the SACL fails without the Security privilege, instead of returning no entries. + var sd = new FileSystemSecurity2(item, System.Security.AccessControl.AccessControlSections.Audit); + return FileSystemAuditRule2.GetFileSystemAuditRules(sd, !excludeExplicit, !excludeInherited, getInheritedFrom); + } + + private void WriteAuditRules(IEnumerable acl) + { + if (account != null) + { + acl = acl.Where(ace => ace.Account == account); + } + + acl.ForEach(ace => WriteObject(ace)); + } } } \ No newline at end of file diff --git a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml index 7e44f86..c93e5ab 100644 --- a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml +++ b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml @@ -4832,15 +4832,16 @@ PS C:\> Disable-Privileges Security2.FileSystemAuditRule2 - The cmdlet returns one object per audit entry, with the audited account, the audited access rights, the audit flags, the inheritance and propagation flags, the `IsInherited` flag, and the `InheritedFrom` path. When an item has no audit entries, or when the SACL cannot be read, the cmdlet returns nothing for that item. + The cmdlet returns one object per audit entry, with the audited account, the audited access rights, the audit flags, the inheritance and propagation flags, the `IsInherited` flag, and the `InheritedFrom` path. When an item has no audit entries, the cmdlet returns nothing for that item; when its SACL cannot be read, the cmdlet writes an error. 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. - 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 falls back to reading the security descriptor without its SACL; it then returns no audit entries and reports no error, which looks the same as an item that is not audited at all. + 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. + 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. diff --git a/Security2/FileSystem/FileSystemSecurity2.cs b/Security2/FileSystem/FileSystemSecurity2.cs index cd5eb08..fda0c72 100644 --- a/Security2/FileSystem/FileSystemSecurity2.cs +++ b/Security2/FileSystem/FileSystemSecurity2.cs @@ -53,16 +53,19 @@ namespace Security2 try { sd = ((FileInfo)this.item).GetAccessControl(AccessControlSections.All); + sections = AccessControlSections.All; } catch { try { sd = ((FileInfo)this.item).GetAccessControl(AccessControlSections.Access | AccessControlSections.Owner | AccessControlSections.Group); + sections = AccessControlSections.Access | AccessControlSections.Owner | AccessControlSections.Group; } catch { sd = ((FileInfo)this.item).GetAccessControl(AccessControlSections.Access); + sections = AccessControlSections.Access; } } @@ -74,21 +77,30 @@ namespace Security2 try { sd = ((DirectoryInfo)this.item).GetAccessControl(AccessControlSections.All); + sections = AccessControlSections.All; } catch { try { sd = ((DirectoryInfo)this.item).GetAccessControl(AccessControlSections.Access | AccessControlSections.Owner | AccessControlSections.Group); + sections = AccessControlSections.Access | AccessControlSections.Owner | AccessControlSections.Group; } catch { sd = ((DirectoryInfo)this.item).GetAccessControl(AccessControlSections.Access); + sections = AccessControlSections.Access; } } } } + // Without the Security privilege, the security descriptor is read without its SACL. + internal bool HasAuditSection + { + get { return (sections & AccessControlSections.Audit) == AccessControlSections.Audit; } + } + public FileSystemSecurity SecurityDescriptor { get diff --git a/Security2/Properties/AssemblyInfo.cs b/Security2/Properties/AssemblyInfo.cs index 89a37d6..5f5bd6d 100644 --- a/Security2/Properties/AssemblyInfo.cs +++ b/Security2/Properties/AssemblyInfo.cs @@ -19,6 +19,9 @@ using System.Runtime.InteropServices; // COM, set the ComVisible attribute to true on that type. [assembly: ComVisible(false)] +// Lets the cmdlets tell whether a security descriptor was read with its SACL. +[assembly: InternalsVisibleTo("NTFSSecurity")] + // The following GUID is for the ID of the typelib if this project is exposed to COM [assembly: Guid("d89dc40a-9b43-4bce-972d-b995df8d2820")] diff --git a/Tests/Audit.Tests.ps1 b/Tests/Audit.Tests.ps1 new file mode 100644 index 0000000..59293c2 --- /dev/null +++ b/Tests/Audit.Tests.ps1 @@ -0,0 +1,100 @@ +<# + Tests the audit cmdlets of the module built in NTFSSecurity\bin\Release on files in a sandbox folder. Reading + and changing audit entries needs the Security privilege; tests that need it skip without it and run in CI, + whose runners are elevated. +#> +[Diagnostics.CodeAnalysis.SuppressMessageAttribute( + 'PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Pester shares variables between blocks.' +)] +param () + +BeforeDiscovery { + Import-Module -Name (Join-Path -Path $PSScriptRoot -ChildPath 'TestHelpers.psm1') -Force + $canReadAudit = Test-PrivilegeHeld -Name 'SeSecurityPrivilege' +} + +BeforeAll { + Import-Module -Name (Join-Path -Path $PSScriptRoot -ChildPath 'TestHelpers.psm1') -Force + $modulePath = Join-Path -Path $PSScriptRoot -ChildPath '..\NTFSSecurity\bin\Release\NTFSSecurity.psd1' + Import-Module -Name $modulePath -Force -ErrorAction Stop + $sandbox = New-TestSandbox -Name 'Audit' + Push-Location -LiteralPath $sandbox + + function New-SandboxItem { + param ( + [string] $Name, + [switch] $Directory + ) + + $path = Join-Path -Path $sandbox -ChildPath ('{0}-{1}' -f $Name, [guid]::NewGuid().ToString('N').Substring(0, 8)) + Assert-TestSandboxPath -Sandbox $sandbox -Path $path + if ($Directory) { + New-Item -ItemType Directory -Path $path | Out-Null + } + else { + Set-Content -LiteralPath $path -Value 'Audit test' + } + $path + } + + # Denies the owner, the current account, to read the security descriptor of the item. + function Deny-ReadPermission { + param ([string] $Path) + + Assert-TestSandboxPath -Sandbox $sandbox -Path $Path + $acl = Get-Acl -LiteralPath $Path + $ownerRights = New-Object -TypeName 'System.Security.Principal.SecurityIdentifier' -ArgumentList 'S-1-3-4' + $rule = New-Object -TypeName 'System.Security.AccessControl.FileSystemAccessRule' -ArgumentList ( + $ownerRights, [System.Security.AccessControl.FileSystemRights]::ReadPermissions, [System.Security.AccessControl.AccessControlType]::Deny + ) + $acl.AddAccessRule($rule) + Set-Acl -LiteralPath $Path -AclObject $acl + } +} + +AfterAll { + Pop-Location + Remove-TestSandbox -Sandbox $sandbox + Remove-Module -Name NTFSSecurity -Force -ErrorAction SilentlyContinue +} + +Describe 'Get-NTFSAudit' { + Context 'When the audit entries cannot be read' { + It 'Should write an error without the Security privilege instead of returning nothing' -Skip:$canReadAudit { + $file = New-SandboxItem -Name 'NoPrivilege' + + $entries = @(Get-NTFSAudit -Path $file -ErrorVariable auditErrors -ErrorAction SilentlyContinue) + + $entries | Should -BeNullOrEmpty + $auditErrors | Should -HaveCount 1 + $auditErrors[0].FullyQualifiedErrorId | Should -BeLike 'ReadSecurityError,*' + } + + It 'Should write an error for a security descriptor that was read without the audit entries' { + $file = New-SandboxItem -Name 'AccessOnly' + $sd = New-Object -TypeName 'Security2.FileSystemSecurity2' -ArgumentList ( + (Get-Item2 -Path $file), [System.Security.AccessControl.AccessControlSections]::Access + ) + + $entries = @(Get-NTFSAudit -SecurityDescriptor $sd -ErrorVariable auditErrors -ErrorAction SilentlyContinue) + + $entries | Should -BeNullOrEmpty + $auditErrors | Should -HaveCount 1 + $auditErrors[0].FullyQualifiedErrorId | Should -BeLike 'ReadSecurityError,*' + } + } + + 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. + It 'Should return the entries of the first item once' -Skip:(-not $canReadAudit) { + $folder = New-SandboxItem -Name 'Audited' -Directory + $denied = New-SandboxItem -Name 'Denied' + Add-NTFSAudit -Path $folder -Account 'Everyone' -AccessRights Delete -AuditFlags Success + Deny-ReadPermission -Path $denied + + $entries = @(Get-NTFSAudit -Path $folder, $denied -ExcludeInherited -ErrorAction SilentlyContinue) + + @($entries | Where-Object -Property FullName -EQ -Value $folder) | Should -HaveCount 1 + } + } +}