Skip to content

Commit

Permalink
Ensure a terminal requesting FG rights actually has them (#12899)
Browse files Browse the repository at this point in the history
#### ⚠️ _Targets #12799_ ⚠️

This is an atomic bit of code that partners with #12799. It's separated as an individual PR to keep diffs more simple. 

This ensures that when a terminal tells ConPTY that it's focused, that ConPTY doesn't do the `ConsoleControl(CONSOLE_FOREGROUND` thing unless the terminal application is actually in the foreground. This prevents a trivial exploit whereby a `malicious.exe` could create a PTY, tell ConPTY it has focus (when it doesn't), then use this mechanism to launch an instance of itself into the foreground. 

When the terminal tells us it's in the foreground, we're gonna look at the owner of the ConPTY window handle. If that owner has focus, then cool, this is allowed. Otherwise, we won't grant them the FG right. For this to work, the terminal just have already called `ReparentPseudoConsole`.

* built on top of #12799 and #12526
* [x] Part of #2988
* [x] Tested manually.
  • Loading branch information
zadjii-msft authored Apr 19, 2022
1 parent a496af3 commit 0da5bd7
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 10 deletions.
4 changes: 2 additions & 2 deletions src/host/outputStream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -378,8 +378,8 @@ void ConhostInternalGetSet::ReparentWindow(const uint64_t handle)
// If the window hasn't been created yet, by some other call to
// LocatePseudoWindow, then this will also initialize the owner of the
// window.
if (const auto psuedoHwnd{ ServiceLocator::LocatePseudoWindow(reinterpret_cast<HWND>(handle)) })
if (const auto pseudoHwnd{ ServiceLocator::LocatePseudoWindow(reinterpret_cast<HWND>(handle)) })
{
LOG_LAST_ERROR_IF_NULL(::SetParent(psuedoHwnd, reinterpret_cast<HWND>(handle)));
LOG_LAST_ERROR_IF_NULL(::SetParent(pseudoHwnd, reinterpret_cast<HWND>(handle)));
}
}
2 changes: 1 addition & 1 deletion src/interactivity/inc/ServiceLocator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ namespace Microsoft::Console::Interactivity

static Globals& LocateGlobals();

static HWND LocatePseudoWindow(const HWND owner = 0 /*HWND_DESKTOP*/);
static HWND LocatePseudoWindow(const HWND owner = nullptr /*HWND_DESKTOP = 0*/);

protected:
ServiceLocator(ServiceLocator const&) = delete;
Expand Down
59 changes: 52 additions & 7 deletions src/terminal/adapter/InteractDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -195,15 +195,60 @@ bool InteractDispatch::IsVtInputEnabled() const
// - true always.
bool InteractDispatch::FocusChanged(const bool focused) const
{
// When we do GH#11682, we should make sure that ConPTY requests this mode
// from the terminal when it starts up, and ConPTY never unsets that flag.
// It should only ever internally disable the events from flowing to the
// client application.

auto& g = ServiceLocator::LocateGlobals();
auto& gci = g.getConsoleInformation();
WI_UpdateFlag(gci.Flags, CONSOLE_HAS_FOCUS, focused);
gci.ProcessHandleList.ModifyConsoleProcessFocus(focused);

// This should likely always be true - we shouldn't ever have an
// InteractDispatch outside ConPTY mode, but just in case...
if (gci.IsInVtIoMode())
{
bool shouldActuallyFocus = false;

// From https://github.com/microsoft/terminal/pull/12799#issuecomment-1086289552
// Make sure that the process that's telling us it's focused, actually
// _is_ in the FG. We don't want to allow malicious.exe to say "yep I'm
// in the foreground, also, here's a popup" if it isn't actually in the
// FG.
if (focused)
{
if (const auto pseudoHwnd{ ServiceLocator::LocatePseudoWindow() })
{
// They want focus, we found a pseudo hwnd.

// Note: ::GetParent(pseudoHwnd) will return 0. GetAncestor works though.
// GA_PARENT and GA_ROOT seemingly return the same thing for
// Terminal. We're going with GA_ROOT since it seems
// semantically more correct here.
if (const auto ownerHwnd{ ::GetAncestor(pseudoHwnd, GA_ROOT) })
{
// We have an owner from a previous call to ReparentWindow

if (const auto currentFgWindow{ ::GetForegroundWindow() })
{
// There is a window in the foreground (it's possible there
// isn't one)

// Get the PID of the current FG window, and compare with our owner's PID.
DWORD currentFgPid{ 0 };
DWORD ownerPid{ 0 };
GetWindowThreadProcessId(currentFgWindow, &currentFgPid);
GetWindowThreadProcessId(ownerHwnd, &ownerPid);

if (ownerPid == currentFgPid)
{
// Huzzah, the app that owns us is actually the FG
// process. They're allowed to grand FG rights.
shouldActuallyFocus = true;
}
}
}
}
}

WI_UpdateFlag(gci.Flags, CONSOLE_HAS_FOCUS, shouldActuallyFocus);
gci.ProcessHandleList.ModifyConsoleProcessFocus(shouldActuallyFocus);
}
// Does nothing outside of ConPTY. If there's a real HWND, then the HWND is solely in charge.

// Theoretically, this could be propagated as a focus event as well, to the
// input buffer. That should be considered when implementing GH#11682.
Expand Down

0 comments on commit 0da5bd7

Please sign in to comment.