Skip to content

Improve windows stemcell hardening by explicitly handeling inherited … #166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Alphasite wants to merge 1 commit into
base: windows-2019
Choose a base branch
from
+51 −14
Draft

Improve windows stemcell hardening by explicitly handeling inherited … #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion acceptance_test/assets/bwats-release/jobs/check-system/templates/run.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,8 @@ function Test-Acls {

$errCount = 0

Get-ChildItem -Path $path -Recurse | ForEach-Object {
$items = @(Get-Item -Path $path) + @(Get-ChildItem -Path $path -Recurse)
$items | ForEach-Object {
Comment on lines +134 to +135
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$items = @(Get-Item ...) + @(Get-ChildItem -Recurse ...) materializes the entire recursive file list in memory before processing. On large trees (e.g., C:\var) this can significantly increase memory/time compared to the previous streaming pipeline. Consider processing Get-Item $path first and then streaming Get-ChildItem -Recurse through the same per-item check (e.g., via a helper function) to avoid building a full array.

Copilot uses AI. Check for mistakes.
$name = $_.FullName
If (-Not ($_.Attributes -match "ReparsePoint")) {
Get-Acl $name | Select-Object -ExpandProperty Access | ForEach-Object {
Expand All @@ -140,6 +141,10 @@ function Test-Acls {
$errCount += 1
Write-Host "Error ($name): $ident"
}
If ($_.IdentityReference -eq "NT AUTHORITY\Authenticated Users" -and ($path -eq "C:\bosh" -or $path -eq "C:\var")) {
$errCount += 1
Write-Host "Error ($name): Authenticated Users should not have access"
}
}
}
}
Expand Down
58 changes: 45 additions & 13 deletions modules/BOSH.Utils/BOSH.Utils.psm1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,33 +100,49 @@ function Protect-Dir
[bool]$disableInheritance = $True
)

if ($disableInheritance)
{
Write-Log "Protect-Dir: Disable Inheritance"
icacls.exe $path /inheritance:d /T
if ($LASTEXITCODE -ne 0)
{
Throw "Error disabling inheritance for $path exited with $LASTEXITCODE"
}
}

Write-Log "Protect-Dir: Grant Administrator"
cmd.exe /c cacls.exe $path /T /E /P Administrators:F
icacls.exe $path /grant "Administrators:(OI)(CI)F" /T
if ($LASTEXITCODE -ne 0)
Comment on lines 113 to 115
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protect-Dir uses icacls /grant which appends a new ACE each time it runs. This can make the function non-idempotent (duplicate Administrators entries accumulate) compared to the prior cacls /P behavior. Consider using the replace form (/grant:r) or first removing/resetting the existing Administrators ACE to keep ACLs stable across repeated runs.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@ay901246 ay901246 Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems reasonable to do.

{
Throw "Error setting ACL for $path exited with $LASTEXITCODE"
}

Write-Log "Protect-Dir: Remove BUILTIN\Users"
cmd.exe /c cacls.exe $path /T /E /R "BUILTIN\Users"
icacls.exe $path /remove "BUILTIN\Users" /T
if ($LASTEXITCODE -ne 0)
{
Throw "Error setting ACL for $path exited with $LASTEXITCODE"
}

Write-Log "Protect-Dir: Remove BUILTIN\IIS_IUSRS"
cmd.exe /c cacls.exe $path /T /E /R "BUILTIN\IIS_IUSRS"
icacls.exe $path /remove "BUILTIN\IIS_IUSRS" /T
if ($LASTEXITCODE -ne 0)
{
Throw "Error setting ACL for $path exited with $LASTEXITCODE"
}

if ($disableInheritance)
Write-Log "Protect-Dir: Remove NT AUTHORITY\Authenticated Users"
icacls.exe $path /remove "NT AUTHORITY\Authenticated Users" /T
if ($LASTEXITCODE -ne 0)
{
Write-Log "Protect-Dir: Disable Inheritance"
$acl = Get-ACL -LiteralPath $path
$acl.SetAccessRuleProtection($True, $True)
Set-Acl -LiteralPath $path -AclObject $acl
Throw "Error setting ACL for $path exited with $LASTEXITCODE"
}

Write-Log "Protect-Dir: Remove Everyone"
icacls.exe $path /remove "Everyone" /T
Comment on lines +134 to +142
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new removals of NT AUTHORITY\Authenticated Users and Everyone are executed even when $disableInheritance is $False. With inheritance still enabled, these groups can remain present via inherited ACEs (so the hardening may not actually take effect), and the logs imply success even though inherited entries won’t be removed. Consider gating these removals behind $disableInheritance (or force-disabling inheritance when these removals are required), or at least make the behavior explicit.

Copilot uses AI. Check for mistakes.
if ($LASTEXITCODE -ne 0)
{
Throw "Error setting ACL for $path exited with $LASTEXITCODE"
}
Comment on lines +134 to 146
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protect-Dir now hardens additional principals (Authenticated Users, Everyone) but the existing Pester coverage for Protect-Dir (in modules/BOSH.Utils/BOSH.Utils.Tests.ps1) only asserts removal of BUILTIN\Users and BUILTIN\IIS_IUSRS. Add/adjust tests to validate the new hardening behavior (and the $disableInheritance interaction) so regressions are caught in CI.

Copilot uses AI. Check for mistakes.
}

Expand All @@ -137,6 +153,16 @@ function Protect-Path
[bool]$disableInheritance = $True
)

if ($disableInheritance)
{
Write-Log "Protect-Path: Disable Inheritance"
icacls.exe $path /inheritance:d
if ($LASTEXITCODE -ne 0)
{
Throw "Error disabling inheritance for $path exited with $LASTEXITCODE"
}
}

Write-Log "Protect-Path: Grant Administrator"
icacls.exe $path /grant "Administrators:(OI)(CI)F"
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protect-Path also uses icacls /grant (append semantics). If this function is called multiple times on the same target, ACLs can grow with duplicate Administrators entries. Prefer an idempotent form such as replacing the ACE (/grant:r) to keep permissions stable across repeated provisioning runs.

Suggested change
icacls.exe $path /grant "Administrators:(OI)(CI)F"
icacls.exe $path /grant:r "Administrators:(OI)(CI)F"

Copilot uses AI. Check for mistakes.
if ($LASTEXITCODE -ne 0)
Expand All @@ -158,12 +184,18 @@ function Protect-Path
Throw "Error setting ACL for $path exited with $LASTEXITCODE"
}

if ($disableInheritance)
Write-Log "Protect-Path: Remove NT AUTHORITY\Authenticated Users"
icacls.exe $path /remove "NT AUTHORITY\Authenticated Users"
if ($LASTEXITCODE -ne 0)
{
Write-Log "Protect-Path: Disable Inheritance"
$acl = Get-ACL -LiteralPath $path
$acl.SetAccessRuleProtection($True, $True)
Set-Acl -LiteralPath $path -AclObject $acl
Throw "Error setting ACL for $path exited with $LASTEXITCODE"
}

Write-Log "Protect-Path: Remove Everyone"
icacls.exe $path /remove "Everyone"
if ($LASTEXITCODE -ne 0)
{
Throw "Error setting ACL for $path exited with $LASTEXITCODE"
}
}

Expand Down
Loading