-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Guard against null or blank path components when adding to module path #19922
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.
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
Guard against null or blank path components when adding to module path #19922
Conversation
JamesWTruher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add some validation for this new behavior
@JamesWTruher - the bug was caused by unexpected return data from this API call in ModuleIntrinsics.cs string myDocumentsPath = Environment.GetFolderPath(Environment.SpecialFolder.MyDocuments);To add tests for this in the build we'd either have to have the ability to run under an account where it returns "" or provide some way to simulate the API returning the empty string (e.g. through a layer of indirection controlled by an environment variable). Is this something you'd like to see, or if you have an alternative approach please let me know. The easiest way I've found to test the impact of this change manually is to use psexec from SysInternals to launch a System account (elevated) command prompt (which returns "" for the API call above). See issue #19875 comments for some screenshots from that testing. psexec -i -s cmd.exeThen in the system account elevated command prompt start the pwsh executable and observe:
I have also tested it manually in the scenario where a domain joined PC with network located MyDocuments is disconnected from the domain network and confirmed it no longer crashes in that scenario either. I'm not familiar enough with the build environment to know how such a test could be orchestrated. The only alternative would be to use some interfaces and create some C# unit tests but I haven't seen any unit test projects in the source base so don't know if this would be an acceptable approach here. |
|
@stevenebutler Easiest way is to use a test hook. See
|
The issue is the test hook would have to be configured before powershell starts up to trigger the bug because this crash happens long before you get an interactive shell or can run a script. It happens while PowerShell itself is initializing. Maybe we can start up PowerShell then tweak the test hook and call into the method that crashes in the module intrinsics code and possibly trigger the same exception afterPowerShell has started. I'll look into that possibility. |
We could use specific/hidden env variable like |
|
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
|
It's not necessary to use env vars as the code that was failing can be called directly after startup with the test hook set to fake an empty MyDocuments location. I've added this test, which failed without the patch. try {
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('SetMyDocumentsSpecialFolderToBlank', $true)
[System.Management.Automation.ModuleIntrinsics]::GetModulePath($null, $null, $null)
}
finally {
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('SetMyDocumentsSpecialFolderToBlank', $false)
} |
TravisEz13
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocking for security review
TravisEz13
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good.
TravisEz13
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed with WG-Security and we don't have any concerns
|
@JamesWTruher Can you please take another look and update your review? Thanks! |
JamesWTruher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
Closes #19875
When MyDocuments folder is blank (e.g. when network drive containing folder is disconnected, or running under local system account) PowerShell crashes on startup. This PR adds some null/blank path checking to strings before they are added to
the module path.
PR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).