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

Warn before the user runs a new commandline elevated #11308

Closed

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Sep 22, 2021

targets #11222, followed by #11310

Summary of the Pull Request

As a part of #8455, we identified that we should probably warn before running commandlines, to make sure the user knows what they're about to do. This dialog looks like the following:

image

image

When the user approves a commandline, we'll remember that commandline, so they won't get prompted every time.
If they reject a commandline, then we're just going to close that pane.

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

This PR changes Pane to be able to host any UserControl, not necessarily a TermControl. This of course has bigger repurcussions than just revealed here. It's gonna wildly conflict with some of the other open PRs. I wanted to do this more generically for #997, but alas, nobody got time for that.

This adds a AdminWarningPlaceholder which is a type of UserControl that just holds another UserControl. It looks just like a ContentDialog, but will actually resize as the window resizes.

We won't prompt for certain commandlines:

  • Anything that exists in system32 AND IS FULLY QUALIFIED eg C:\windows\system32\cmd.exe.
    • cmd.exe will prompt, because it's unqualified.
    • C:\windows\system32\cmd.exe /k echo sneaky sneak will also prompt, because it's got other args.
  • %SystemRoot%\System32\cmd.exe won't prompt, because it's smart enoguh to handle env vars
  • %SystemRoot%\System32\wsl.exe -d <distroname> won't prompt, because we trust wsl distros.
  • %SystemRoot%\System32\wsl.exe -d <distroname> bash -c do-malicious-shit.sh WILL prompt

Validation Steps Performed

Opened a bunch of terminals, in random orders, and made sure that the above commandlines would work as expected. My list of running profiles:

    {
        "commandline": "%SystemRoot%\\System32\\WindowsPowerShell\\v1.0\\powershell.exe",
        "guid": "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}",
        "hidden": false,
        "name": "Windows PowerShell"
    },
    {
        "commandline": "%SystemRoot%\\System32\\cmd.exe",
        "guid": "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}",
        "hidden": false,
        "name": "Command Prompt"
    },
    {
        "guid": "{574e775e-4f2a-5b96-ac1e-a2962a402336}",
        "hidden": false,
        "name": "PowerShell",
        "source": "Windows.Terminal.PowershellCore"
    },
    {
        "guid": "{c6eaf9f4-32a7-5fdc-b5cf-066e8a4b1e40}",
        "hidden": false,
        "name": "Ubuntu-18.04",
        "source": "Windows.Terminal.Wsl"
    },
    {
        "commandline": "c:\\windows\\system32\\cmd.exe",
        "guid": "{5aea3919-92fa-5990-bb39-2321f316d9b9}",
        "name": "the COOLER cmd",
        "startingDirectory": "%USERPROFILE%"
    },
    {
        "commandline": "c:\\windows\\system32\\cmd.exe /k echo sneaky sneaks",
        "guid": "{4fd85e9a-919a-5033-9118-1b7518b676a7}",
        "name": "the sneaky cmd",
        "startingDirectory": "%USERPROFILE%"
    },
    {
        "background": "#9C1C0C",
        "commandline": "cmd.exe /k echo This profile is always elevated",
        "guid": "{7a7854d2-65bc-57c2-a2c6-70a32a2f600e}",
        "name": "elevated cmd",
        "startingDirectory": "well this is garbage",
        "tabColor": "#9C1C0C"
    },
    {
        "background": "#1C0C9C",
        "commandline": "cmd.exe /k echo This profile is just as elevated as you started with",
        "guid": "{b5c1cbf5-217f-5e0f-b3ee-3c70150ef037}",
        "name": "unelevated cmd",
        "tabColor": "#1C0C9C",
        "useAcrylic": true
    },

  Sublime can delete the file just fine (and write any old file in it's place)
  Even we can modify the file when running unelevated, which is **B**ad.

  So there's gotta be some way to allow _us_ to write the file only when elevated
…'t care about elevation or none of that, so the unelevated terminal could atomically rewrite the elevated file, which isn't what we wanted.
(cherry picked from commit b499d44d4baf21c279dbb9f3a766bc9c37528b62)
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

There's still a lot of TODOs here... do you intend to finish those before merge?

Base automatically changed from dev/migrie/f/just-elevated-state-2 to main November 13, 2021 00:58
@lhecker lhecker dismissed stale reviews from miniksa and carlos-zamora via bad27a9 November 13, 2021 00:58
lhecker pushed a commit that referenced this pull request Nov 13, 2021
## Summary of the Pull Request

This creates an `elevated-state.json` that lives in `%LOCALAPPDATA%` next to `state.json`, that's only writable when elevated. It doesn't _use_ this file for anything, it just puts the framework down for use later.

It's _just like `ApplicationState`_. We'll use it the same way. 

It's readable when unelevated, which is nice, but not writable. If you're dumb and try to write to the file when unelevated, it'll just silently do nothing.

If we try opening the file and find out the permissions are different, we'll _blow the file away entirely_. This is to prevent someone from renaming the original file (which they can do unelevated), then slapping a new file that's writable by them down in it's place. 

## References
* We're going to use this in #11096, but these PRs need to be broken up.

## PR Checklist
* [x] Closes nothing
* [x] I work here
* [x] Tests added/passed
* [ ] Requires documentation to be updated - maybe? not sure we have docs on `state.json` at all yet

## Validation Steps Performed
I've played with this much more in `dev/migrie/f/non-terminal-content-elevation-warning`

###### followed by #11308, #11310
@zadjii-msft
Copy link
Member Author

There's still a lot of TODOs here... do you intend to finish those before merge?

Yea, just wanted to push to get it on the cloud before the weekend. Between the three PRs in this chain they'll all get cleaned up.

@DHowett
Copy link
Member

DHowett commented Nov 15, 2021

So, in the recent 1.13 selfhost builds of Terminal I've been seeing all sorts of weird sizing issues -- sometimes when I launch unpackaged I will get a normal-sized window with a one-column-wide TermControl:

M
i
c
r
o
s
o
f
t

W

Other times -- you know that inverse block of text when powershell starts up telling you about a new version? That looks like it's been resized down and is no longer square (I'll screenshoot next time it happens.)

I wonder if that's related to the changes here that make panes no longer host TermControls?

Further: if you right-click a tab that's displaying an elevation warning and choose "split," terminal crashes.

@DHowett
Copy link
Member

DHowett commented Nov 15, 2021

There it is-

image

It's like the opposite of #32!

@zadjii-msft
Copy link
Member Author

So, in the recent 1.13 selfhost builds of Terminal I've been seeing all sorts of weird sizing issues -- sometimes when I launch unpackaged I will get a normal-sized window with a one-column-wide TermControl:

Fix it in post? Couldn't get it repro'd this morning 😕 Were you launching unpackaged, elevated, hitting the dialog, then accepting it? Or just elevated (no prompt)? Or unelevated?


Further: if you right-click a tab that's displaying an elevation warning and choose "split," terminal crashes.

That's fixed in 056446c

@github-actions

This comment has been minimized.

}
// Always look in "%LOCALAPPDATA%\Microsoft\WindowsApps", which is
// where the pwsh.exe execution alias lives.
{ wil::ExpandEnvironmentStringsW<std::wstring>(L"%LOCALAPPDATA%\\Microsoft\\WindowsApps") },
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to be a stick in the mud, but unfortunately this path is user-controlled.

(dhowett-sl) ~\src\wt-1.13 % echo hi > $Env:LOCALAPPDATA\Microsoft\WindowsApps\FUN.EXE
(dhowett-sl) ~\src\wt-1.13 % FUN
ResourceUnavailable: Program 'FUN.EXE' failed to run: The specified executable is not a valid application for this OS platform.

@DHowett
Copy link
Member

DHowett commented Nov 17, 2021

Fix it in post? Couldn't get it repro'd this morning 😕 Were you launching unpackaged, elevated, hitting the dialog, then accepting it? Or just elevated (no prompt)? Or unelevated?

Unelevated, but weirdly I was hitting an issue where OpenConsole.exe wouldn't run. Try running unpackaged, deleting OpenConsole, and then opening wt unelevated. It was a solid 100% repro for my terminal window being the wrong size.

@zadjii-msft zadjii-msft self-assigned this Jan 4, 2022
ghost pushed a commit that referenced this pull request Jan 12, 2022
## Summary of the Pull Request

This is the resurrection of #8514 and #11310. WE determined that we didn't want to do #11308 after all, so this should be profile auto-elevation, without the warning.

This PR adds two features:
* the `elevate: bool` property to profiles
  - If the user is running unelevated, **and** `elevate` is set to `true`, then instead of opening a new tab, we'll open an elevated Terminal window with the profile.
  - Otherwise, we'll just open a new tab in the existing window. This includes cases where the window is elevated, and the profile is set to `elevate:false`. `elevate:false` basically just means "do nothing special with me".
* the `elevate: bool?` property to `NewTerminalArgs` (`newTab`, `splitPane`)
  - This allows a user to create an action that will elevate the profile, even if the profile is not otherwise set to auto-elevate.
  - `elevate:null` (_the default_) does not change the profile's elevation status. The action will use whatever is set by the profile.
  - `elevate:true` will attempt to auto-elevate the profile
  - `elevate:false` will do nothing special. 


## References
* #5000 for obvious reasons
* Spec'd in #8455

## PR Checklist
* [x] Closes #632
* [x] I work here
* [x] Tests added/passed
* [ ] Requires documentation to be updated - sure does, but that'll come all at the end.

## Detailed Description of the Pull Request / Additional comments

After playing with de-elevation a bit, it turns out it behaves weirdly with packaged applications. I can try and ask `explorer.exe` to launch the process on our behalf. However, if the thing we're launching is an execution alias (`wt.exe`), and we're elevated, then the child process will _still launch elevated_. 

There's also something super BODGEY at work here. `ShellExecute` is the function we use to ask the OS to elevate something for us. But `ShellExecute` needs to be able to send a window message to the process that called it (if the caller was a WINDOWS subsystem application). So if we die immediately after calling `ShellExecute`, then the elevated process never actually spawns - sad. So we're adding a helper process, `elevate-shim.exe`, that lives in our process. That'll be the one that actually calls `ShellExecute`, so that it can live for the duration of the UAC prompt.

## Validation Steps Performed

* Ran tests
* Opened a bunch of terminal tabs at various different elevation levels
* opened new splits too
* In the defaults (base layer) as well, for madness 

Some settings to use for testing

<details>

```jsonc
    "keybindings" :
    [
        ////////// ELEVATION ///////////////
        { "keys": "f1", "name": "ELEVATED TAB", "icon": "\uEA18", "command": { "action": "newTab", "elevate": true } },
        { "keys": "f2", "name": "ELEVATED, Color", "icon": "\uEA18", "command": {
            "action": "newTab", "elevate": true, "commandline": "PowerShell.exe", "startingDirectory": "C:\\Windows", "tabColor": "#bbaa00"
        } },
        { "keys": "f3", "name": "unelevated ELEVATED", "icon": "🙃", "command": {
            "action": "newTab", "elevate": false, "profile": "elevated cmd"
        } },
        //////////////////////////////
    ],

    "profiles":
    {
        "defaults":
        {
            "elevate": true,
        },
        "list":
        [
            {
                "hidden":false,
                "name" : "cmd",
                "commandline" : "cmd.exe",
                "guid": "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}",
                "startingDirectory" : "%USERPROFILE%",
                "opacity" : 20
            },
            {
                "name" : "the COOLER cmd",
                "commandline" : "c:\\windows\\system32\\cmd.exe",
                "startingDirectory" : "%USERPROFILE%",
            },
            {
                "name" : "the sneaky cmd",
                "commandline" : "c:\\windows\\system32\\cmd.exe /k echo sneaky sneaks",
                "startingDirectory" : "%USERPROFILE%",
            },
            {
                "name": "elevated cmd",
                "commandline": "cmd.exe /k echo This profile is always elevated",
                "startingDirectory" : "well this is garbage",

                "elevate": true,
                "background": "#9C1C0C",
                "tabColor": "#9C1C0C",
                "colorScheme": "Desert"
            },
            {
                "name": "unelevated cmd",
                "commandline": "cmd.exe /k echo This profile is just as elevated as you started with",
                "elevate": false,
                "background": "#1C0C9C",
                "tabColor": "#1C0C9C",
                "colorScheme": "DotGov",
                "useAcrylic": true
            },
        ]
```

</details>

Also try:
* `wtd nt -p "elevated cmd" ; sp -p "elevated cmd"`
* `wtd nt -p "elevated cmd" ; nt -p "elevated cmd"`




This was merged manually via 

```
git diff dev/migrie/f/non-terminal-content-elevation-warning dev/migrie/f/632-on-warning-dialog > ..\632.patch
git apply ..\632.patch --ignore-whitespace --reject
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prompt before launching commandlines when elevated ; cache answers in elevated state.json
6 participants