Skip to content
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

Run as Admin shortcut + elevate:true -> crash loop #13928

Closed
ShawnXie01 opened this issue Sep 6, 2022 · 7 comments · Fixed by #14946
Closed

Run as Admin shortcut + elevate:true -> crash loop #13928

ShawnXie01 opened this issue Sep 6, 2022 · 7 comments · Fixed by #14946
Labels
Area-Windowing Window frame, quake mode, tearout In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal.

Comments

@ShawnXie01
Copy link

Windows Terminal version

1.14.2281.0 or preview 1.15.2282.0

Windows build number

10.0.19044.0

Other Software

No response

Steps to reproduce

  1. Install terminal regardless preview version or not.
  2. Turn on the "Run this profile as Administrator" in Profile->default
  3. Restart the terminal.

Expected Behavior

No response

Actual Behavior

bandicam.2022-09-06.11-26-11-658.mp4

I also tried run terminal as administrator,but it seemed didn't work.

@ShawnXie01 ShawnXie01 added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Sep 6, 2022
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Sep 6, 2022
@ShawnXie01
Copy link
Author

Just xlinking a few issues:

Something weird is going on, for sure

I read them before I raised the issue.I logged in as my admin user,the only user on my windows,and tried to install Terminal from the MicrosoftStore,yet it still didn't work.

@zadjii-msft
Copy link
Member

Oh man, you know what, I think another thread might have a better repro. Copying deets here.

If a shortcut properties are set to run wt.exe as Admin
AND
Run as Admin is also set in wt app settings, new tab or the app will not start, and DesktopWindowXamlSource continuously flashes in taskbar.

Terminal ver = 1.15.2874.0
Windows ver = 10.0.19044

@zadjii-msft zadjii-msft changed the title Cannot open terminal when "Run this profile as Administrator" option is on Run as Admin shortcut + elevate:true -> crash loop Oct 24, 2022
@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. Priority-1 A description (P1) Area-Windowing Window frame, quake mode, tearout labels Oct 24, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 24, 2022
@zadjii-msft zadjii-msft added Needs-Tag-Fix Doesn't match tag requirements and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Oct 24, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 24, 2022
@zadjii-msft zadjii-msft added this to the Backlog milestone Oct 24, 2022
@jboelter
Copy link
Contributor

jboelter commented Jan 8, 2023

Repro'd a version of the crash loop with the default Administrator account. The issue stems from the way IsElevated() is checking for elevation.

edit: these instructions reproduce the bug; I'm not aware of a fix.

  1. Enable the default Administrator account on your machine (this account is special)
  2. Log into the account (by default no password was set); I used the "Switch User" feature (note: you probably want to set a password and/or disable the account later)
  3. Launch Terminal and change the settings to include a default elevate=true for all profiles
  4. Exit Terminal
  5. Launch Terminal (now w/ all profiles default to elevate); observe an infinite loop while it repeatedly tries to launch an elevated instance from the current instance.

image

Change the default profile settings

    "profiles": 
    {
        "defaults": { "elevate": true },
    ...

Root Cause

I built a custom dev build with some extra logging in the utils.cpp IsElevated check. The built-in Administrator account meets the criteria that it has a TokenElevationTypeDefault token and TokenIsElevated is 1. This in turn causes IsElevated() to return false.

This interacts with TerminalPage::ShouldImmediatelyHandoffToElevated which knows it wants to launch an elevated profile, but (mistakenly) thinks it is not currently elevated. This causes an infinite loop of WindowsTerminal.exe -> elevate-shim.exe while it tries to get an elevated process.

In short, the check for TokenElevationTypeDefault short circuits the elevation check to false and creates an infinite loop.

https://github.com/microsoft/terminal/blob/main/src/types/utils.cpp#L664-L673

            if (elevationType == TokenElevationTypeDefault && elevationState.TokenIsElevated)
            {
                // In this case, the user has UAC entirely disabled. This is sort of
                // weird, we treat this like the user isn't an admin at all. There's no
                // separation of powers, so the things we normally want to gate on
                // "having special powers" doesn't apply.
                //
                // See GH#7754, GH#11096
                return false;
            }

Related Docs:

TokenElevationTypeDefault: Type 1 is a full token with no privileges removed or groups disabled. A full token is only used if User Account Control is disabled or if the user is the built-in Administrator account (for which UAC disabled by default), service account or local system account.

@jboelter
Copy link
Contributor

procmon trace (screenshot) of the loop on latest 1.16.10261.0. This is running under the default Administrator account on 10.0.22621.1105 with elevate=true in the defaults.

The sequence doesn't look right given some lost events, but the PID parent/child relationships are as expected.

WindowsTerminal.exe doesn't think it's elevated (per comment above); it's launching elevate-shim which is using shell:Appsfolder with the runas verb to launch WindowsTerminal elevated and the cycle repeats.

image

@DHowett
Copy link
Member

DHowett commented Jan 31, 2023

                return false;

Ah, heck. This came up in an earlier code review that we didn't end up merging, too.

We need separate checks for IsUserAnAdminInAWayThatBreaksSomeOfOurFeatures and IsUserAnAdminAtAll. Names TBD. @zadjii-msft, do you remember when that came up? Was it for elevated-state?

@zadjii-msft
Copy link
Member

image

zadjii-msft added a commit that referenced this issue Mar 3, 2023
…/drop

Credit where credit is due - @jboelter did literally all the hard work.

I just separated this out to two elements:
* Are we running elevated?
* Can we drag drop?

As we learned in #7754, the builtin administrator _can_ drag drop. But
critically, they are also running as admin! The way we had this logic before,
we're treat them as unelevated, because we had been overloading the meaning
here.

This splits these into two separate functions. Comes with the added benefit of
re-adding the elevation shield to the Terminal window for users with UAC
disabled (which was missing before) (and can _still_ be disabled).

Closes #13928

Tested on a Win10 VM with `EnableLua=0`
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Mar 3, 2023
DHowett pushed a commit that referenced this issue Mar 17, 2023
Credit where credit is due - @jboelter did literally all the hard work.

I just separated this out to two elements:
* Are we running elevated?
* Can we drag drop?

As we learned in #7754, the builtin administrator _can_ drag drop. But
critically, they are also running as admin! The way we had this logic
before, we're treat them as unelevated, because we had been overloading
the meaning here.

This splits these into two separate functions. Comes with the added
benefit of re-adding the elevation shield to the Terminal window for
users with UAC disabled (which was missing before) (and can _still_ be
disabled).

Closes #13928

Tested on a Win10 VM with `EnableLua=0`
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Mar 17, 2023
DHowett pushed a commit that referenced this issue May 15, 2023
Credit where credit is due - @jboelter did literally all the hard work.

I just separated this out to two elements:
* Are we running elevated?
* Can we drag drop?

As we learned in #7754, the builtin administrator _can_ drag drop. But
critically, they are also running as admin! The way we had this logic
before, we're treat them as unelevated, because we had been overloading
the meaning here.

This splits these into two separate functions. Comes with the added
benefit of re-adding the elevation shield to the Terminal window for
users with UAC disabled (which was missing before) (and can _still_ be
disabled).

Closes #13928

Tested on a Win10 VM with `EnableLua=0`

(cherry picked from commit c5c15e8)
Service-Card-Id: 88484047
Service-Version: 1.17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Windowing Window frame, quake mode, tearout In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants