Skip to content

Validate installer functionality and error handling - #92

Open
AmirMS (AmelBawa-msft) wants to merge 1 commit into
mainfrom
user/amelbawa/rust-download
Open

AmirMS (AmelBawa-msft) wants to merge 1 commit into
mainfrom
user/amelbawa/rust-download

Conversation

@AmelBawa-msft

Copy link
Copy Markdown
Collaborator

No description provided.

@ranm-msft ranm-msft left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The Authenticode and publisher check is the right addition, and failing closed with the file removed is the right shape.

One gap it does not close: this resource declares securityContext: elevated, but $env:TEMP\vs_BuildTools.exe is a fixed, predictable path. In the normal split-token local-admin case the elevated process runs as the same account and gets that account's per-user TEMP, so a medium-integrity process under the same user can pre-create that exact path. Invoke-WebRequest overwriting an existing file does not establish that the pathname is exclusively owned by the elevated process, so Get-AuthenticodeSignature and Start-Process are not guaranteed to be looking at the same bytes.

Would you consider staging into a directory the elevated process creates itself with an admin-only ACL, rejecting a pre-existing or reparse-point target, so the verified file cannot be swapped before launch? A random subdirectory name on its own only reduces predictability - the ACL is what makes the check meaningful through to execution.

To be fair, that staging shape predates this PR and these three lines do not introduce it. Raising it because this change is what turns the verification into a stated guarantee.

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.

2 participants