-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[Investigate] - Detecting elevated processes. #1015
Comments
As a suggestion, its possible by using PowerShell script. Source: Detecting if a certain process is elevated I have tested this script on local machine and it seems like it reports everything correctly, but only from non-elevated PowerShell (I tried PowerShell Core as well as Native version). Script itself is pretty short: |
Now the question becomes how do we do this programmatically. :) |
Actually, its not complicated and if using c# we can go with something like in this one: |
Running a PowerShell script every time the current active window changes is something I would consider unacceptable. |
just brainstorming, hence this is an investigation step. How do we monitor and how do we not spam the OS to see if something is in an elevated state (aka polling every 10 seconds or something) to me, i think the big thing we need to detect is the user's environment in a state we can't work properly in. If it is, we should prompt them and go "hey, we see some stuff is running as admin, some stuff may not work correctly until that process is ended or we are also elevated" And if then detect no more processes aren't elevated, we should say "hey friend, we detected there is no longer a reason for us to run as admin, we're going to drop down to a normal process". This could be a setting, auto detect and raise/lower your permission as needed. This could also be a setting of something like
|
I suspect this will open the door to scenarios where the execution time of PowerToys actions may be unacceptable, for example:
|
I think we should not relaunch anything, #1016, but instead create some kind of grabber that runs elevated and manages apps that runs on high level, it will be used just in case if PT is not running as admin. I bet its possible to run processes as elevated from non-elevated app. As result we don't need to relaunch PT, we have grabber that deals with elevated apps, we ask users if they want to run that grabber. And we don't ask anything if PT itself is running elevated. |
Can you explain what the grabber functionalities are? |
I would imagine that dragging an elevated window should trigger a one-time (or with a couple of days cooldown) notification to the user inviting her to read a detailed explanation why she might want to run PT elevated. An average user imho won't care when exactly in time we've decided to drop the privileges, so as long as she understands that we do the right thing the dropping privileges notification might be unnecessary. Regarding the isolation discussion in #942, it'd be great to run each PT in a separate process, which we might refactor for in the future starting from #1016. As @enricogior stated, that would be a pretty fundamental change. For now, as @alexr3 proposed, we could extract the part which does the actual window moving/resizing into a separate elevated process and send the commands to it. I've also traced which of the API functions fail during the elevated window moving/resizing, and it looks like the only one that fails with Implementation-wise, we can determine if a process is elevated using the usual #define PSAPI_VERSION 1
#include <Windows.h>
#include <shlwapi.h>
#pragma comment( lib, "Psapi.lib" )
#include <stdio.h>
#include <tchar.h>
#include <psapi.h>
// To obtain a HWND's PID GetWindowThreadProcessId should suffice.
void PrintPidAndElevation(DWORD processID)
{
HANDLE hProcess = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION,
FALSE, processID);
HANDLE token = nullptr;
bool elevated = false;
if(OpenProcessToken(hProcess, TOKEN_QUERY, &token)) {
TOKEN_ELEVATION elevation;
DWORD size;
if(GetTokenInformation(token, TokenElevation, &elevation, sizeof(elevation), &size))
elevated = (elevation.TokenIsElevated != 0);
}
_tprintf(TEXT("PID: %u %s\n"), processID, elevated? L"elevated" : L"normal");
CloseHandle(token);
CloseHandle(hProcess);
}
int main(void)
{
DWORD aProcesses[1024], cbNeeded, cProcesses;
unsigned int i;
if(!EnumProcesses(aProcesses, sizeof(aProcesses), &cbNeeded))
return 1;
cProcesses = cbNeeded / sizeof(DWORD);
for(i = 0; i < cProcesses; i++)
{
if(aProcesses[i] != 0)
PrintPidAndElevation(aProcesses[i]);
}
return 0;
} Finally, we're not showing the zones when shift-dragging an elevated window when the |
@yuyoyuppe these are all very useful points when evaluating changes to the current design vs complete new architecture. |
@enricogior agree, we might investigate whether it's possible to also integrate lowlevel keyboard handler with this imagined fine-grained elevated local COM server process which will communicate via COM RPC with all the different non-elevated toys. |
Adding a separate elevated process to pull the strings sounds unnecessary complicated. Having each powertoy run in a separate process has its own benefits and would be simpler to implement. |
@bzoz I agree, though we'll have to send all events to subscribers using IPC anyway in case of separate processes if we still want to keep the "hooks architecture". Abandoning it will result in a simplest architecture and probably won't affect performance that much. |
As @yuyoyuppe explained its some kind of module or part of a PT. While I am not good at COM and slightly understand possible implications using such technology I imagined this module as the one that calls functions or is executing commands which involve elevated processes. In case the target process is not elevated existing code would be used. Primary function of this module would be execution of code which involves interaction with elevated processes. That's was my initial vision, but maybe there is better approach to this. |
@alexr3 |
I agree with @bzoz and @yuyoyuppe, I'd rather have fully separate processes with complete autonomy from each other. |
Remember, this is an investigation, I think there are a lot of things we may want to adjust. I was thinking about a lot of different ways we COULD do things. I actually think #938 (documenting what needs elevated priv, when, and why) needs to happen before this as we can start talking about this and the upcoming utilities for what makes them need to run as admin and in what scenarios. @enricogior, we have lots of stuff that will share components and need think how those interact with one another along with if one runs at admin and the other doesn't, what happens. That is why i think #938 is a early item to knock out We are still early days for this project. This is something to think about for next release and see what the impacts will be. Breaking changes could happen. Architectural changes may happen once we start seeing new scenarios. |
@crutkas JK ;) |
I agree with @crutkas … the first step is documenting what needs elevation, when, and why. Once that is agreed upon, it will provide the common foundation for discussions on solutions, such as determining if a solution would work for each item or not. [[ Note: I am participating in my free time, in a personal capacity only ]] |
@crutkas How do we want to show a warning that a window can't be dragged because its process is elevated? To avoid notification spam, we could show a toast notification e.g. once a day. We could also have a "Never show again" button on it. |
The fix is now available in 0.16 https://github.com/microsoft/PowerToys/releases |
Can we detect if applications are running elevated and suggest PT needs to be relaunched elevated if they want to interact? This would keep us in more secure state.
Background:
Since PowerToys itself is a process, it can only do things at or below the same 'privilege'. If PowerShell or Terminal is launched as an admin and PT isn't, PT can't interact with those windows.
Why:
The text was updated successfully, but these errors were encountered: