Start-Process: Fix -Credential error as non-admin by jborean93 · Pull Request #21393 · PowerShell/PowerShell

Jordan Borean (jborean93) · 2024-03-30T03:14:36Z

PR Summary

When using Start-Process -Credential in a non-administrator context with a different user's credentials the code currently throws an exception when attempting to get the new process' handle. This change treats this error as a warning only if -PassThru was specified as it only affects certain members on the returned object like ExitCode but others like WaitForExit() still work fine.

Fixes: #21073

PR Context

The code to retrieve the Handle property was added by #20749 which tries to set the internal state of the Process object. This works in most cases but can fail if the user is not running as admin and the credentials specified was for another user. This PR turns the exception it raises about being unable to retrieve the handle to now become a warning as some things can still be used with the returned Process object.

This isn't a change in the status quo as going back to .NET Framework the returned Process object in the above scenario still could not do things like get the ExitCode due to permissions problems. This just ensures that an error/exception is not raised and code that worked in the back will still continue to do so.

The ultimately fix here is to start the process through the .NET System.Diagnostics.Process class instead of using PInvoke with CreateProcess* but until they add an API to create a suspended process or build the Process object from a SafeHandle PowerShell cannot do so for all scenarios it deals with

The PR should be considered for a backport to 7.4.x as that was when the change was also introduced.

PR Checklist

Jordan Borean (jborean93) · 2024-03-30T03:22:30Z

Just as an FYI I did not create a test in this as it would require being able to create another user (requires admin) then run as non-admin. The test runner doesn't seem to have an easy way to really do this so I skipped it.

When using Start-Process -Credential in a non-administrator context with a different user's credentials the code currently throws an exception when attempting to get the new process' handle. This change treats this error as a warning only if -PassThru was specified as it only affects certain members on the returned object like ExitCode but others like WaitForExit() still work fine.

Ilya (iSazonov) · 2024-03-30T13:13:36Z

Just as an FYI I did not create a test in this as it would require being able to create another user (requires admin) then run as non-admin. The test runner doesn't seem to have an easy way to really do this so I skipped it.

We could try to create a test hook. Keyword is InternalTestHooks. I don't know how much complexity is warranted, but a regression defense would be desirable.

Jordan Borean (jborean93) · 2024-03-30T22:20:53Z

We could try to create a test hook. Keyword is InternalTestHooks

Isn't that to just tell PowerShell to do something differently internally? I'm not sure how that would help here as the exception occurs in .NET code and not PowerShell. Unless I'm mistaken?

The problem that we need to test is doing Start-Process -Credential $cred where $cred is another user's credentials. The trouble with setting this up is the test needs to run as non-admin but we need admin rights to create the user.

Ilya (iSazonov) · 2024-03-31T05:49:10Z

Test hooks are definitely something we do internally. It's kind of like having our own test Web server. And that's not easy to implement for processes. This effort makes sense if we want to actively develop this cmdlet.

On the other hand, I suppose we could use the GiHub action config to create a regular user with a single-session password. Or try net user /add inside the test. That would be easier than the test hook.

Jordan Borean (jborean93) · 2024-03-31T06:12:03Z

Yea the proper method would be to configure CI to create a test user for this (and other tests) as an admin before it starts pester as the non-admin user. I’ve got no experience with the CI setup in this repository and I probably don’t want to go fiddling around with it without someone from that side advising what they want to do there.

As for the test hook I can see if being useful if we want to test out specific code branches but in this case it’s an exception being raised in code that we are calling rather than in code that we control. It wouldn’t make sense to raise an exception ourselves here to just catch it IMO.

Ilya (iSazonov) · 2024-03-31T07:28:17Z

Interesting, .Net uses WindowsTestAccount to create a test user.
We could try to add this to our test common module. Then it would be easier for us to create tests for the Start-Process

Jordan Borean (jborean93) · 2024-03-31T08:17:50Z

Creating the test user is the simple part with builtin cmdlets New-LocalUser. The hard part is getting the rights to do so in a non-admin context or running the test as admin an de-elevating for this particular scenario.

Ilya (iSazonov) · 2024-04-01T04:48:39Z

Creating the test user is the simple part with builtin cmdlets New-LocalUser.

I forget that we have Windows PowerShell. 😄

We run unelevated tests with
Start-UnelevatedProcess

PowerShell/build.psm1

Lines 1664 to 1682 in b23607e

    
           function script:Start-UnelevatedProcess 
        
           { 
        
               param( 
        
                   [string]$process, 
        
                   [string[]]$arguments 
        
               ) 
        
               if (-not $environment.IsWindows) 
        
               { 
        
                   throw "Start-UnelevatedProcess is currently not supported on non-Windows platforms" 
        
               } 
        
               if (-not $environment.OSArchitecture -eq 'arm64') 
        
               { 
        
                   throw "Start-UnelevatedProcess is currently not supported on arm64 platforms" 
        
               } 
        
               runas.exe /trustlevel:0x20000 "$process $arguments" 
        
           }

So it seems it is easy to create a test user account.

Jordan Borean (jborean93) · 2024-04-01T04:54:48Z

So it seems it is easy to create a test user account.

The question is more when to integrate it. We would need to create the test user before running the Pester process and pass through the credentials somehow. It also would need to cleanup the user once the test is done. I don’t feel comfortable enough without the people who maintain the CI part of this repo to provide their input. I’m also somewhat concerned that the talk to add tests will bog down the fix especially since it should be backported to the next 7.4 release.

Ilya (iSazonov) · 2024-04-01T08:45:49Z

I think this PR will be reviewed quickly as it is a regression.

Also, I think we can easily add (and remove) a user to build.psm1 in the try-catch-finally block that is listed above. The only requirement is to generate a password dynamically. It will be in a pwsh variable for the duration of the test.
Travis Plunk (@TravisEz13) Is this acceptable?

Dongbo Wang (daxian-dbw)

Code changes look good but need Cmdlet WG to review the behavior.

Ilya (iSazonov) · 2024-04-02T04:14:08Z

For WG. It seems we never write "warnings" to avoid "noise". So we could remove the "warning" at all or replace it with "verbose" or "debug".

Steve Lee (SteveL-MSFT) · 2024-04-03T16:09:42Z

+                {
+                    _ = process.Handle;
+                }
+                catch (Win32Exception e)


Should we check the HRESULT for the specific error rather than treating them all the same?

As getting the Handle was an attempt to try and set the internal state and not something that is being used in the code I didn't think it was worth setting a specific error code. I can change it but I didn't think it warranted catching a specific error because we aren't doing anything to work around the error.

Steve Lee (@SteveL-MSFT) Here we should rather move in the other direction. Since the only operation we do is getting a handle, we rather need to catch and ignore all exceptions, especially since if there is a question about behavior, we can always display the debug message.
Although I think Win32Exception covers everything that is needed.

Steve Lee (SteveL-MSFT) · 2024-04-03T16:40:06Z

The @PowerShell/wg-powershell-cmdlets discussed this and compared the behavior to WinPS5.1. We recommend changing from Warning to a Debug message, then handle is null w/o any message so the behavior is consistent with 5.1.

Jordan Borean (jborean93) · 2024-04-04T20:44:57Z

Updated to become WriteDebug.

Jordan Borean (jborean93) · 2024-04-15T20:19:41Z

Thanks for the reviews and merge!

Aditya Patwardhan (adityapatwardhan) · 2024-05-07T20:10:24Z

We will wait on this change to be released in a preview before considering a backport to 7.4.

Jordan Borean (jborean93) · 2024-05-07T22:08:26Z

Will that even make a difference. There are people already affected by this and the chances of someone actually using the preview for such use cases I see as rare.

…n privilege (#21393)

…n privilege (#21393) (#23933)

…n privilege (PowerShell#21393)

Jordan Borean (jborean93) requested review from Aditya Patwardhan (adityapatwardhan) and Dongbo Wang (daxian-dbw) as code owners March 30, 2024 03:14

Jordan Borean (jborean93) mentioned this pull request Mar 30, 2024

PS v7.4.1 breaks passing credentials to start-process #21073

Closed

5 tasks

Ilya (iSazonov) approved these changes Mar 30, 2024

View reviewed changes

Ilya (iSazonov) assigned Dongbo Wang (daxian-dbw) Mar 30, 2024

Ilya (iSazonov) requested a review from Steve Lee (SteveL-MSFT) March 30, 2024 13:14

Ilya (iSazonov) added CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log 7.4-regression Regression in 7.4 Backport-7.4.x-Consider labels Mar 30, 2024

Dongbo Wang (daxian-dbw) added the WG-Cmdlets general cmdlet issues label Apr 1, 2024

Steve Lee (SteveL-MSFT) added the WG-NeedsReview Needs a review by the labeled Working Group label Apr 1, 2024

Dongbo Wang (daxian-dbw) reviewed Apr 1, 2024

View reviewed changes

Steve Lee (SteveL-MSFT) reviewed Apr 3, 2024

View reviewed changes

Steve Lee (SteveL-MSFT) added WG-Reviewed A Working Group has reviewed this and made a recommendation and removed WG-NeedsReview Needs a review by the labeled Working Group labels Apr 3, 2024

Change warning to debug

c12a42f

microsoft-github-policy-service Bot added the Review - Needed The PR is being reviewed label Apr 12, 2024

Dongbo Wang (daxian-dbw) approved these changes Apr 15, 2024

View reviewed changes

Dongbo Wang (daxian-dbw) merged commit 588dff6 into PowerShell:master Apr 15, 2024

microsoft-github-policy-service Bot removed the Review - Needed The PR is being reviewed label Apr 15, 2024

Dongbo Wang (daxian-dbw) removed the 7.4-regression Regression in 7.4 label Apr 15, 2024

Jordan Borean (jborean93) deleted the process-cred branch April 15, 2024 18:58

Aditya Patwardhan (adityapatwardhan) pushed a commit that referenced this pull request Jun 11, 2024

Fix the error when using Start-Process -Credential without the admi…

3b15495

…n privilege (#21393)

Aditya Patwardhan (adityapatwardhan) mentioned this pull request Jun 11, 2024

[release/v7.4.3] Start-Process: Fix -Credential error as non-admin #23933

Merged

Aditya Patwardhan (adityapatwardhan) added a commit that referenced this pull request Jun 11, 2024

Fix the error when using Start-Process -Credential without the admi…

1e13b52

…n privilege (#21393) (#23933)

Aditya Patwardhan (adityapatwardhan) added Backport-7.4.x-Done and removed Backport-7.4.x-Consider labels Jun 11, 2024

unfurl-links Bot mentioned this pull request Jun 20, 2024

macOS Packages for v7.4.3 not signed #23974

Closed

5 tasks

Chris Dent (chrisdent-de) pushed a commit to chrisdent-de/PowerShell that referenced this pull request Sep 12, 2024

Fix the error when using Start-Process -Credential without the admi…

0259742

…n privilege (PowerShell#21393)

Justin Chung (jshigetomi) mentioned this pull request Jan 21, 2025

Fix backport issues with release pipeline #24835

Merged

21 tasks

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026

Fix the error when using Start-Process -Credential without the admi…

b2c27fc

…n privilege (PowerShell#21393)

This was referenced Aug 10, 2026

AI prompt cache integration test, system_prompt_at_depth defaults, and browser install refactor steve02081504/fount#296

Merged

Enhance app removal verification and error reporting Raphire/Win11Debloat#735

Merged

Uh oh!

Conversation

Jordan Borean (jborean93) commented Mar 30, 2024

PR Summary

PR Context

PR Checklist

Uh oh!

Jordan Borean (jborean93) commented Mar 30, 2024

Uh oh!

Ilya (iSazonov) commented Mar 30, 2024

Uh oh!

Jordan Borean (jborean93) commented Mar 30, 2024

Uh oh!

Ilya (iSazonov) commented Mar 31, 2024

Uh oh!

Jordan Borean (jborean93) commented Mar 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ilya (iSazonov) commented Mar 31, 2024

Uh oh!

Jordan Borean (jborean93) commented Mar 31, 2024

Uh oh!

Ilya (iSazonov) commented Apr 1, 2024

Uh oh!

Jordan Borean (jborean93) commented Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ilya (iSazonov) commented Apr 1, 2024

Uh oh!

Dongbo Wang (daxian-dbw) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Apr 2, 2024

Uh oh!

Steve Lee (SteveL-MSFT) Apr 3, 2024

Choose a reason for hiding this comment

Uh oh!

Jordan Borean (jborean93) Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jordan Borean (jborean93) commented Apr 4, 2024

Uh oh!

Jordan Borean (jborean93) commented Apr 15, 2024

Uh oh!

Aditya Patwardhan (adityapatwardhan) commented May 7, 2024

Uh oh!

Jordan Borean (jborean93) commented May 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Jordan Borean (jborean93) commented Mar 31, 2024 •

edited

Loading

Jordan Borean (jborean93) commented Apr 1, 2024 •

edited

Loading

Dongbo Wang (daxian-dbw) left a comment •

edited

Loading

Steve Lee (SteveL-MSFT) commented Apr 3, 2024 •

edited

Loading