Browse Source

fix: address the review of 5.0.0-rc4

- Read the root folder for a volume name such as \\?\Volume{GUID}\ too,
  like for a drive letter, and accept only the letters A to Z as a drive
- Report a security descriptor without the audit entries without naming a
  missing Security privilege as the cause, which may not be the reason
- Skip the audit tests that change a descriptor from
  Get-NTFSSecurityDescriptor in a session without the Security privilege
- Assert the absence of the old hint in the Get-NTFSEffectiveAccess test
- Narrow the drive-root note of Get-NTFSAccess to the security cmdlets

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: AI Assistant <ai@example.com>
pull/113/head
Raimund Andree 5 days ago
parent
commit
ce318ceecf
  1. 9
      CHANGELOG.md
  2. 2
      Docs/Cmdlets/Get-NTFSAccess.md
  3. 2
      NTFSSecurity/BaseCmdlets.cs
  4. 2
      NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml
  5. 11
      Security2/FileSystem/FileSystemSecurity2.cs
  6. 13
      Tests/Access.Tests.ps1
  7. 11
      Tests/Audit.Tests.ps1
  8. 11
      Tests/DriveRoot.Tests.ps1

9
CHANGELOG.md

@ -234,10 +234,11 @@ The format is based on
- Fix `Clear-NTFSAudit`, which finished without an error but changed nothing - Fix `Clear-NTFSAudit`, which finished without an error but changed nothing
in a session without the Security privilege; it now writes an error, like in a session without the Security privilege; it now writes an error, like
the other audit cmdlets the other audit cmdlets
- Fix the cmdlets for the root of a drive, such as `C:\`, which read and - Fix the cmdlets for the root of a drive, such as `C:\`, or of a volume,
changed the security descriptor of the drive, a device object, instead of such as `\\?\Volume{GUID}\`, which read and changed the security
that of its root folder, so that `Get-NTFSAccess` showed other entries than descriptor of the drive, a device object, instead of that of its root
Explorer ([#41](https://github.com/raandree/NTFSSecurity/issues/41)) folder, so that `Get-NTFSAccess` showed other entries than Explorer
([#41](https://github.com/raandree/NTFSSecurity/issues/41))
- Fix `Add-NTFSAudit`, `Remove-NTFSAudit`, and `Clear-NTFSAudit` with a - Fix `Add-NTFSAudit`, `Remove-NTFSAudit`, and `Clear-NTFSAudit` with a
security descriptor that was read without the audit entries; they now security descriptor that was read without the audit entries; they now
write an error like `Get-NTFSAudit` instead of changing the missing entries write an error like `Get-NTFSAudit` instead of changing the missing entries

2
Docs/Cmdlets/Get-NTFSAccess.md

@ -186,7 +186,7 @@ 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, after a path whose ACL could not be read, the cmdlet returned the entries of the previous item again.
For the root of a drive, such as `C:\`, the cmdlets of the module read and change 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. 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 ## RELATED LINKS

2
NTFSSecurity/BaseCmdlets.cs

@ -33,7 +33,7 @@ namespace NTFSSecurity
} }
var ex = new InvalidOperationException(string.Format( 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)); "The security descriptor of '{0}' doesn't contain the audit entries. Read it with Get-NTFSSecurityDescriptor in a session that holds the Security privilege.", sd.FullName));
WriteError(new ErrorRecord(ex, "ReadSecurityError", ErrorCategory.InvalidData, sd)); WriteError(new ErrorRecord(ex, "ReadSecurityError", ErrorCategory.InvalidData, sd));
return false; return false;
} }

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

@ -4588,7 +4588,7 @@ PS C:\&gt; Disable-Privileges</dev:code>
<maml:para>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. Changing the owner of an item requires the Take Ownership and Restore privileges, so this fallback only succeeds in an elevated session of an account that holds them.</maml:para> <maml:para>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. Changing the owner of an item requires the Take Ownership and Restore privileges, so this fallback only succeeds in an elevated session of an account that holds them.</maml:para>
<maml:para>Entries whose account cannot be translated into a name are returned with their SID. Use `Get-NTFSOrphanedAccess` to list only those entries.</maml:para> <maml:para>Entries whose account cannot be translated into a name are returned with their SID. Use `Get-NTFSOrphanedAccess` to list only those entries.</maml:para>
<maml:para>Before 5.0.0, after a path whose ACL could not be read, the cmdlet returned the entries of the previous item again.</maml:para> <maml:para>Before 5.0.0, after a path whose ACL could not be read, the cmdlet returned the entries of the previous item again.</maml:para>
<maml:para>For the root of a drive, such as `C:`, the cmdlets of the module read and change 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.</maml:para> <maml:para>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.</maml:para>
</maml:alert> </maml:alert>
</maml:alertSet> </maml:alertSet>
<command:examples> <command:examples>

11
Security2/FileSystem/FileSystemSecurity2.cs

@ -119,7 +119,16 @@ namespace Security2
private static bool TryGetDriveRoot(FileSystemInfo item, out string root) private static bool TryGetDriveRoot(FileSystemInfo item, out string root)
{ {
var fullName = item.FullName.TrimEnd('\\'); var fullName = item.FullName.TrimEnd('\\');
if (fullName.Length == 2 && fullName[1] == ':' && char.IsLetter(fullName[0]))
// A drive letter, such as C:
var isDriveLetter = fullName.Length == 2 && fullName[1] == ':' &&
((fullName[0] >= 'A' && fullName[0] <= 'Z') || (fullName[0] >= 'a' && fullName[0] <= 'z'));
// A volume name, such as \\?\Volume{9f122b1b-858a-49bd-aec4-dfbe8978fe16}, without a folder below it
var isVolumeName = fullName.StartsWith(@"\\?\Volume{", StringComparison.OrdinalIgnoreCase) &&
fullName.EndsWith("}", StringComparison.Ordinal) && fullName.IndexOf('\\', 4) < 0;
if (isDriveLetter || isVolumeName)
{ {
root = fullName + "\\"; root = fullName + "\\";
return true; return true;

13
Tests/Access.Tests.ps1

@ -128,7 +128,7 @@ Describe 'Get-NTFSEffectiveAccess' {
$accessErrors | Should -HaveCount 1 $accessErrors | Should -HaveCount 1
$accessErrors[0].FullyQualifiedErrorId | Should -BeLike 'GetEffectiveAccessError,*' $accessErrors[0].FullyQualifiedErrorId | Should -BeLike 'GetEffectiveAccessError,*'
$accessErrors[0].Exception.Message | Should -Not -BeLike '*privilege*' $accessErrors[0].Exception.Message | Should -Not -BeLike '*Enable-Privileges*'
} }
} }
} }
@ -443,7 +443,16 @@ Describe 'Security descriptor parameter sets' {
# Before 5.0.0, PowerShell could not choose between the SDSimple and SDComplex parameter sets. # Before 5.0.0, PowerShell could not choose between the SDSimple and SDComplex parameter sets.
It '<_> should accept -SecurityDescriptor without -AppliesTo or the flag parameters' -ForEach @( It '<_> should accept -SecurityDescriptor without -AppliesTo or the flag parameters' -ForEach @(
'Add-NTFSAccess', 'Remove-NTFSAccess', 'Add-NTFSAudit', 'Remove-NTFSAudit' 'Add-NTFSAccess', 'Remove-NTFSAccess'
) {
$sd = Get-NTFSSecurityDescriptor -Path $folder
{ & $_ -SecurityDescriptor $sd -Account 'Everyone' -AccessRights ReadData -ErrorAction Stop } | Should -Not -Throw
}
# A descriptor from Get-NTFSSecurityDescriptor contains the audit entries only with the Security privilege.
It '<_> should accept -SecurityDescriptor without -AppliesTo or the flag parameters' -Skip:(-not $holdsSecurityPrivilege) -ForEach @(
'Add-NTFSAudit', 'Remove-NTFSAudit'
) { ) {
$sd = Get-NTFSSecurityDescriptor -Path $folder $sd = Get-NTFSSecurityDescriptor -Path $folder

11
Tests/Audit.Tests.ps1

@ -99,7 +99,8 @@ Describe 'Add-NTFSAudit' {
$positions['AccessRights'] | Should -Be 3 $positions['AccessRights'] | Should -Be 3
} }
It 'Should bind an account and access rights that are passed by position' { # A descriptor from Get-NTFSSecurityDescriptor contains the audit entries only with the Security privilege.
It 'Should bind an account and access rights that are passed by position' -Skip:(-not $canReadAudit) {
$file = New-TestSandboxItem -Sandbox $sandbox -Name 'Positional' $file = New-TestSandboxItem -Sandbox $sandbox -Name 'Positional'
$sd = Get-NTFSSecurityDescriptor -Path $file $sd = Get-NTFSSecurityDescriptor -Path $file
@ -110,7 +111,8 @@ Describe 'Add-NTFSAudit' {
} }
} }
Context 'With -PassThru' { # A descriptor from Get-NTFSSecurityDescriptor contains the audit entries only with the Security privilege.
Context 'With -PassThru' -Skip:(-not $canReadAudit) {
It 'Should return the audit entries of a security descriptor, not its access entries' { It 'Should return the audit entries of a security descriptor, not its access entries' {
$file = New-TestSandboxItem -Sandbox $sandbox -Name 'PassThru' $file = New-TestSandboxItem -Sandbox $sandbox -Name 'PassThru'
$sd = Get-NTFSSecurityDescriptor -Path $file $sd = Get-NTFSSecurityDescriptor -Path $file
@ -222,7 +224,8 @@ Describe 'Remove-NTFSAudit' {
} }
} }
Context 'With -RemoveSpecific' { # A descriptor from Get-NTFSSecurityDescriptor contains the audit entries only with the Security privilege.
Context 'With -RemoveSpecific' -Skip:(-not $canReadAudit) {
BeforeEach { BeforeEach {
$removeFolder = New-TestSandboxItem -Sandbox $sandbox -Name 'RemoveSpecific' -Directory $removeFolder = New-TestSandboxItem -Sandbox $sandbox -Name 'RemoveSpecific' -Directory
$sd = Get-NTFSSecurityDescriptor -Path $removeFolder $sd = Get-NTFSSecurityDescriptor -Path $removeFolder
@ -299,6 +302,8 @@ Describe 'Audit cmdlets with a security descriptor without the audit entries' {
$result | Should -BeNullOrEmpty $result | Should -BeNullOrEmpty
$auditErrors | Should -HaveCount 1 $auditErrors | Should -HaveCount 1
$auditErrors[0].FullyQualifiedErrorId | Should -BeLike 'ReadSecurityError,*' $auditErrors[0].FullyQualifiedErrorId | Should -BeLike 'ReadSecurityError,*'
# The descriptor was read with the access entries only, not without the Security privilege.
$auditErrors[0].Exception.Message | Should -Not -BeLike '*because it was read without the Security privilege*'
$sd.SecurityDescriptor.GetSecurityDescriptorSddlForm('Audit') | Should -BeNullOrEmpty $sd.SecurityDescriptor.GetSecurityDescriptorSddlForm('Audit') | Should -BeNullOrEmpty
} }
} }

11
Tests/DriveRoot.Tests.ps1

@ -39,4 +39,15 @@ Describe 'The root folder of a drive' {
It 'Get-NTFSOwner should return the owner of the root folder' { It 'Get-NTFSOwner should return the owner of the root folder' {
(Get-NTFSOwner -Path $root).Owner.Sid | Should -Be $acl.GetOwner($sidType).Value (Get-NTFSOwner -Path $root).Owner.Sid | Should -Be $acl.GetOwner($sidType).Value
} }
It 'Get-NTFSAccess should return the access entries of the root folder for the volume name, such as \\?\Volume{GUID}\' {
# Win32_Volume returns nothing to a user without elevation; mountvol works for every user.
$volume = (mountvol.exe $root /L | Out-String).Trim()
$volume | Should -BeLike '\\?\Volume{*}\'
$expected = @($acl.GetAccessRules($true, $true, $sidType) | ForEach-Object -Process { $_.IdentityReference.Value } | Sort-Object)
$entries = @(Get-NTFSAccess -Path $volume)
@($entries | ForEach-Object -Process { $_.Account.Sid } | Sort-Object) | Should -Be $expected
}
} }

Loading…
Cancel
Save