Skip to content

Optimize packaged temp ACL handling without weakening ACL repairs#6171

Merged
JohnMcPMS merged 3 commits intomicrosoft:masterfrom
mamoreau-devolutions:fix-apply-acl-performance-issue
Apr 24, 2026
Merged

Optimize packaged temp ACL handling without weakening ACL repairs#6171
JohnMcPMS merged 3 commits intomicrosoft:masterfrom
mamoreau-devolutions:fix-apply-acl-performance-issue

Conversation

@mamoreau-devolutions
Copy link
Copy Markdown
Contributor

@mamoreau-devolutions mamoreau-devolutions commented Apr 22, 2026


Fixes #6162

Summary

This change reduces the severe slowdown caused by recursive ACL propagation on packaged temp paths while preserving the security behavior that repairs improperly secured directories before use.

The fix keeps the existing fail-closed model, but avoids calling ApplyACL() when the target directory is already in the exact secure state winget expects.

Changes

  • add a fail-closed ACL inspection path before ApplyACL()
  • only skip ACL reapplication when the current owner, DACL protection state, and effective permissions match the expected secure state
  • continue to reapply ACLs on any mismatch, unreadable security descriptor, unknown principal, or unsupported ACL shape
  • scope packaged temp paths under the runtime state name so any required repair stays bounded to the active winget-managed subtree instead of the full shared %TEMP%\WinGet tree
  • add runtime coverage for the steady-state secure case and for mismatch cases that must still force ACL repair

Why this is safe

The security intent of the original code is preserved: winget still repairs attacker-created or incorrectly ACL'd directories before using them.

This PR only removes unnecessary recursive ACL work in the steady state where the directory is already secure. If the ACL state cannot be verified confidently, winget keeps the existing repair behavior.

Validation

  • added runtime tests covering ACL skip/reapply behavior
  • added coverage for packaged temp path behavior
  • locally validated that steady-state temp-path resolution no longer pays the recursive ACL propagation cost when the directory is already secure
Microsoft Reviewers: Open in CodeFlow

@mamoreau-devolutions
Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree company="Devolutions"

@mamoreau-devolutions mamoreau-devolutions force-pushed the fix-apply-acl-performance-issue branch from 8976b07 to 25fb658 Compare April 22, 2026 20:51
@mamoreau-devolutions mamoreau-devolutions marked this pull request as ready for review April 22, 2026 21:11
@mamoreau-devolutions mamoreau-devolutions requested a review from a team as a code owner April 22, 2026 21:11
@JohnMcPMS JohnMcPMS requested a review from Copilot April 22, 2026 22:59
Comment thread src/AppInstallerCommonCore/Runtime.cpp Outdated
Comment thread src/AppInstallerSharedLib/Filesystem.cpp Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses the packaged-context slowdown when resolving temp paths by avoiding unnecessary recursive ACL propagation on large %TEMP%\WinGet trees, while preserving the existing fail-closed “repair ACLs before use” security behavior.

Changes:

  • Add an ACL inspection/short-circuit path so ApplyACL() is only invoked when ownership/DACL protection/effective permissions don’t already match the expected secure state.
  • Scope packaged temp paths under the runtime state name (matching unpackaged behavior) to keep any required ACL repair bounded to a winget-managed subtree.
  • Add runtime tests covering the steady-state “skip” case and mismatch cases that must force ACL repair.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/AppInstallerSharedLib/Public/winget/Filesystem.h Updates ShouldApplyACL() description to reflect the new “needs to be applied” semantics.
src/AppInstallerSharedLib/Filesystem.cpp Implements ACL inspection logic and updates ShouldApplyACL() to be fail-closed while skipping redundant ACL reapplication.
src/AppInstallerCommonCore/Runtime.cpp Updates packaged temp path to include the runtime state name to avoid operating on the shared %TEMP%\\WinGet root.
src/AppInstallerCLITests/Runtime.cpp Adds runtime tests for ACL skip/reapply behavior and for the packaged temp path state-name scoping.

Comment thread src/AppInstallerSharedLib/Filesystem.cpp Outdated
@JohnMcPMS
Copy link
Copy Markdown
Member

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@JohnMcPMS
Copy link
Copy Markdown
Member

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

Comment thread src/AppInstallerCLITests/Runtime.cpp Outdated
@JohnMcPMS
Copy link
Copy Markdown
Member

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@JohnMcPMS JohnMcPMS merged commit d567b47 into microsoft:master Apr 24, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Packaged temp path ACL reapplication on %TEMP%\WinGet causes recursive security traversal and severe slowdown

3 participants