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

When running elevated, use non-elevated processes unless required #942

Closed
henrygab opened this issue Dec 14, 2019 · 6 comments
Closed

When running elevated, use non-elevated processes unless required #942

henrygab opened this issue Dec 14, 2019 · 6 comments

Comments

@henrygab
Copy link

Summary of the new feature/enhancement

If started with elevated privileges, load individual powertoys in either an elevated process, or one with restricted tokens (non-elevated), based on the individual powertoys feature requirements (and user configuration override?).

Problem

Some powertoy capabilities (FancyZones, etc.) cannot work with elevated applications, unless powertoys is itself elevated. However, this means every powertoy gets elevated privileges, which unnecessarily increases attack surface / raises risks for any bugs.

A user could selectively enable which powertorys run in the process with elevated privileges. This means that, even if running elevated, only a smaller subset has full system access.

Proposed technical implementation details (optional)

See Stackoverflow post from 2016, for one way to remove administrative privileges.
There's also a Microsoft blog post from 2013 describing a second way.

Longer-term, and related to #938, each PowerToy could describe the privileges needed, and the runner could drop the non-necessary privileges when creating the process for that PowerToy...

@enricogior
Copy link
Contributor

enricogior commented Dec 16, 2019

Hi @henrygab
as a general approach I agree with the concept.
But the current architecture doesn't spawn a new process for each PowerToys. Unless they have a particular requirement (like PowerRename that is a shell extension and therefore is executed in the shell process) each PowerToy is loaded and executed in the main process.
So the technique described in the articles you linked don't apply to how PowerToys works.
The only external process that we are running elevated and that we could run non-elevated is the FZ editor. The fix is simple, I suggest you to open an issue for it.

The reason why PowerToys runs elevated, is to be able to interact with elevated windows and to process keyboard and mouse events when the active window is elevated.
We are changing the default behavior to allow the users to choose if they want to run PowerToys by default elevated or not.
In the future we may decide to run individual PowerToy in their own process if needed, but that is not currently a requirement for the existing modules.
We are going to document these motivations, so it will be easier for users to decide if they want to run PowerToys elevated or not.

@crutkas
Copy link
Member

crutkas commented Dec 17, 2019

@DHowett-MSFT here is the thread i was referring to.

@henrygab
Copy link
Author

@enricogior -- Two questions below...

I now understand that you are going to enable the entire powertoys app to run non-elevated.

Q1. Can you point me to the issue that is tracking this, so I can get notification when it closes?

I admit that I'm also surprised that each powertoy is not isolated from the others via a separate process. It just seems like it'd make it much harder to track down issues if there's ever a bug that causes memory or stack corruption, not to mention the inability to remove unneeded privileges of those processes.

Q2. Should I open a new issue to track the feature request of running each powertoy in its own, privilege-reduced process?

@enricogior
Copy link
Contributor

enricogior commented Dec 23, 2019

@henrygab

Q1. This has already be done 619ed23
It will be available in the next release.
It was done as part of #413 so feel free to follow it to get notified.

Q2. Spawning one process for each PowerToy would require to redesign from scratch the current PowerToys main process, it's not something we would consider doing in the near future given is arguably an improvement. We may reconsider it in the future. But feel free to open the issue to have a public discussion.

To give you some motivations why it wouldn't be necessary an improvement:

  • debugging with the current design is not challenging, also consider each module can be easily turned off if you want to reduce the footprint.
  • with the current design, the runner can register for low level events in one place and easily dispatch those events to each module that registered for those events. If each module runs in a separate process, that design may not fit at all and all modules may need to do their own registration and handling, with potential conflicts between modules.
  • can you provide a real world scenario where is critical to be able to run some modules elevated and other non-elevated? If a module raises security concern, it may be a not well designed/implemented module, or if there is such a scenario, the module itself can spawn a process or run a restricted privileges thread. So we don't need to run each module in their own process to cover this scenario.

@henrygab
Copy link
Author

@enricogior -- Thanks for the answers.

Necessity depends on the view of the user. Given the default is to run as administrator, and it's an all-or-nothing choice, it forces the user to fully trust all the modules.

Your viewpoint of "if there is no known security concern, then it is OK to run everything with full administrator privileges" is, bluntly, in direct contradiction of secure development lifecycle (SDL) best practices. Code should run in the minimum security context necessary to get its own job done. As it appears that you are arguing against this best practice, then I am simply left perplexed.

I will step away from this discussion for a while, as I do not feel I can contribute to a productive result.

@enricogior
Copy link
Contributor

Given the default is to run as administrator

In the next release the default will be to run non-elevated.

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

No branches or pull requests

3 participants