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
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
124 commits
Select commit Hold shift + click to select a range
42c3eea
pull application state members into a base class
zadjii-msft Aug 30, 2021
eb243f5
Split ApplicationState into a Base and add an Elevated version
zadjii-msft Aug 31, 2021
2857324
proof of concept add a dialog to pop when opening new tabs
zadjii-msft Aug 31, 2021
ccbcb42
Merge remote-tracking branch 'origin/main' into dev/migrie/f/elevatio…
zadjii-msft Aug 31, 2021
14d21f4
nits, cleanup
zadjii-msft Sep 1, 2021
c66a566
Allow a Pane to host a UserControl instead of a TermControl
zadjii-msft Sep 1, 2021
ed1cf2a
add the boilerplate for a custom content dialog like thing
zadjii-msft Sep 1, 2021
1ee3522
Allow a TerminalTab to have a UserControl
zadjii-msft Sep 1, 2021
7fb7d64
These are things I might need for #997
zadjii-msft Sep 1, 2021
631cdf7
Who ever said nested lambdas is a bad thing?
zadjii-msft Sep 1, 2021
736a351
This works amazingly well
zadjii-msft Sep 1, 2021
7f29e1e
More things we might need for Non-Terminal content, #997
zadjii-msft Sep 2, 2021
9b32681
This works to prompt before splitting a pane
zadjii-msft Sep 2, 2021
58c6646
Format that dialog with the actual commandline
zadjii-msft Sep 2, 2021
e5dc64e
Hot-reload the elevated state?
zadjii-msft Sep 2, 2021
64898b1
update other panes too
zadjii-msft Sep 2, 2021
b592155
Merge remote-tracking branch 'origin/main' into dev/migrie/f/non-term…
zadjii-msft Sep 8, 2021
447c2f9
I think this creates a test file that's only accessible by SYSTEM. As…
zadjii-msft Sep 8, 2021
880222d
This file isn't even readable by admins, so maybe that's bad
zadjii-msft Sep 9, 2021
5197dc4
this worked, nice
zadjii-msft Sep 9, 2021
721b636
unfortunately, we can't _write_ this file...
zadjii-msft Sep 9, 2021
8569211
Now, we can update the file after it's created
zadjii-msft Sep 9, 2021
aef422d
This is definitely not right.
zadjii-msft Sep 9, 2021
9a1cf5a
bunch of dead ends
zadjii-msft Sep 9, 2021
7854abe
I bet that's what I was doing wrong. The rename is just dandy, it don…
zadjii-msft Sep 9, 2021
6265f4f
this looks like it checks the permissions as we'd expect
zadjii-msft Sep 9, 2021
1111d41
This works to do the 'blow the file away when permissions have change…
zadjii-msft Sep 13, 2021
4f697ec
I thing this was unneeded
zadjii-msft Sep 13, 2021
4da965f
Merge remote-tracking branch 'origin/main' into dev/migrie/f/non-term…
zadjii-msft Sep 13, 2021
1e3a319
more unique_sid's, and m,ore comments
zadjii-msft Sep 13, 2021
d6c2fb5
THis fixes #7754
zadjii-msft Sep 14, 2021
6be6972
This is everything from dev/migrie/f/non-terminal-content-elevation-w…
zadjii-msft Sep 14, 2021
c106f64
:facepalm:
zadjii-msft Sep 14, 2021
51e0473
minor nits
zadjii-msft Sep 14, 2021
da0cc7b
todo! in the code
zadjii-msft Sep 14, 2021
4e69a32
bunch of new allowed words
zadjii-msft Sep 14, 2021
6757452
fix the tests
zadjii-msft Sep 14, 2021
edd7126
Merge branch 'dev/migrie/f/just-elevated-state-2' into dev/migrie/f/n…
zadjii-msft Sep 14, 2021
47d55a8
don't do the lookup for things in system32
zadjii-msft Sep 14, 2021
54a0027
comments
zadjii-msft Sep 14, 2021
723037e
fix the focus thing, frick the env vars thing
zadjii-msft Sep 14, 2021
d944a68
manually allow env var parsing. Remember that wsl needs manual allows…
zadjii-msft Sep 14, 2021
56d5f9e
every day I'm mangling
zadjii-msft Sep 14, 2021
a4acdeb
blindly remove ElevatedState
zadjii-msft Sep 20, 2021
5685063
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Sep 20, 2021
d0f05f6
Merge branch 'dev/migrie/f/just-elevated-state-2' into dev/migrie/f/n…
zadjii-msft Sep 20, 2021
7f9f75c
gah, I might have broke persisting window state
zadjii-msft Sep 20, 2021
9b3b9e0
remove baseapplicationstate and just merge it back in
zadjii-msft Sep 20, 2021
5a8e27e
Merge branch 'dev/migrie/f/just-elevated-state-2' into dev/migrie/f/n…
zadjii-msft Sep 20, 2021
507a48e
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Sep 22, 2021
b1b1bef
this is a crazy idea that I hate but gotta start somewhere
zadjii-msft Sep 22, 2021
b755eb0
Merge remote-tracking branch 'origin/main' into dev/migrie/f/non-term…
zadjii-msft Sep 22, 2021
b279601
Merge branch 'dev/migrie/f/just-elevated-state-2' into dev/migrie/f/n…
zadjii-msft Sep 22, 2021
94c4cca
I think this works ALMOST as I want
zadjii-msft Sep 22, 2021
64d02f2
Move window state, approvedCommandlines into `user-state.json`
zadjii-msft Sep 22, 2021
de9dc32
pr nits
zadjii-msft Sep 22, 2021
8635537
fix hot reloading for this file
zadjii-msft Sep 22, 2021
7e2e4ea
I think this works ALMOST as I want
zadjii-msft Sep 22, 2021
f3738f5
Move window state, approvedCommandlines into `user-state.json`
zadjii-msft Sep 22, 2021
a6e044d
pr nits
zadjii-msft Sep 22, 2021
eee657b
fix hot reloading for this file
zadjii-msft Sep 22, 2021
02e9871
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Sep 22, 2021
7e2b371
Merge branch 'dev/migrie/f/just-elevated-state-2' into dev/migrie/f/n…
zadjii-msft Sep 22, 2021
5ff9a24
okay so I guess that's not a word
zadjii-msft Sep 22, 2021
62fe823
Merge branch 'dev/migrie/f/just-elevated-state-2' into dev/migrie/f/n…
zadjii-msft Sep 22, 2021
5e9d0b8
use the background from the control when we can
zadjii-msft Sep 22, 2021
a3ac32a
derp
zadjii-msft Sep 22, 2021
abb847b
pr nits from miniksa
zadjii-msft Sep 27, 2021
620ee30
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Sep 28, 2021
a751156
Merge branch 'dev/migrie/f/just-elevated-state-2' into dev/migrie/f/n…
zadjii-msft Sep 28, 2021
d053f6c
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Sep 30, 2021
5699229
Apply suggestions from code review
zadjii-msft Sep 30, 2021
866832b
This seemingly works the way I'd expect, going to merge into the warn…
zadjii-msft Sep 30, 2021
ff33387
Merge branch 'dev/migrie/f/just-elevated-state-2' into dev/migrie/f/n…
zadjii-msft Sep 30, 2021
b4e0496
cleanup
zadjii-msft Sep 30, 2021
9ff2775
Merge branch 'dev/migrie/f/just-trying-something-on-just-elevated-sta…
zadjii-msft Sep 30, 2021
48b20de
Add some logging. Can't seem to get the crash to repro?
zadjii-msft Sep 30, 2021
3c1866a
Merge remote-tracking branch 'origin/dev/migrie/b/1.12-crash-on-exit'…
zadjii-msft Sep 30, 2021
aea3752
we want this
zadjii-msft Sep 30, 2021
0e7217d
Merge commit 'aea37520b3cdb1c1752a6c8e0ff598991518ce28' into dev/migr…
zadjii-msft Sep 30, 2021
ae99ce9
teh
zadjii-msft Sep 30, 2021
f49c3fc
Merge branch 'dev/migrie/f/just-elevated-state-2' into dev/migrie/f/n…
zadjii-msft Sep 30, 2021
bf3c6e7
spel is hard
zadjii-msft Sep 30, 2021
faa06f8
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Oct 7, 2021
945c81d
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Oct 27, 2021
21d6ffe
Merge branch 'dev/migrie/f/just-elevated-state-2' into dev/migrie/f/n…
zadjii-msft Oct 27, 2021
fe5a78c
Fix OpenConsoleProxy for Debug builds
lhecker Oct 27, 2021
90b7962
Merge remote-tracking branch 'origin/dev/lhecker/proxy-no-default-lib…
zadjii-msft Oct 27, 2021
69f1068
update appearance
zadjii-msft Oct 27, 2021
242de14
Incredible that this works at all
zadjii-msft Oct 27, 2021
d6d7087
Cleanup from the experimentation phase
zadjii-msft Oct 27, 2021
7fb4906
More cleanup
zadjii-msft Oct 27, 2021
b9979ff
Esc to dismiss too
zadjii-msft Oct 28, 2021
bdf0816
spel
zadjii-msft Oct 28, 2021
3a8a83a
last pr nits
zadjii-msft Nov 1, 2021
9b4ae9e
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Nov 1, 2021
25c34df
Merge branch 'dev/migrie/f/just-elevated-state-2' into dev/migrie/f/n…
zadjii-msft Nov 1, 2021
c90eb87
good catch
zadjii-msft Nov 1, 2021
4976a09
this is a collection of the trivial nits while I wait on a reply to t…
zadjii-msft Nov 4, 2021
fd849a5
trying to do the thing eryksun mentioned. This seems to actually work…
zadjii-msft Nov 8, 2021
25b2675
this works really quite well
zadjii-msft Nov 9, 2021
b212871
this is the right way to initialize the unique_hlocal_security_descri…
zadjii-msft Nov 9, 2021
4f16dfb
many comments
zadjii-msft Nov 9, 2021
a93d17e
I believe this is the rest of the comments
zadjii-msft Nov 9, 2021
ce6a9c5
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Nov 9, 2021
c09bdbd
Merge branch 'dev/migrie/f/just-elevated-state-2' into dev/migrie/f/n…
zadjii-msft Nov 9, 2021
fdc5749
make text selectable
zadjii-msft Nov 9, 2021
5253c11
make sure event handlers get replaced, too
zadjii-msft Nov 9, 2021
1c66877
pwsh core fixed too
zadjii-msft Nov 9, 2021
7024f44
whoops
zadjii-msft Nov 10, 2021
db9cbf3
spell
zadjii-msft Nov 10, 2021
08cbd16
the last of it?
zadjii-msft Nov 10, 2021
999f21f
Merge remote-tracking branch 'origin/main' into dev/migrie/f/just-ele…
zadjii-msft Nov 11, 2021
33e96e7
mitigate a TOCTOU
zadjii-msft Nov 11, 2021
7f03d4d
dustins nits
zadjii-msft Nov 11, 2021
97d11d1
Merge branch 'dev/migrie/f/just-elevated-state-2' into dev/migrie/f/n…
zadjii-msft Nov 11, 2021
bad27a9
THIS NEEDS TO GO TO THE PARENT
zadjii-msft Nov 11, 2021
fd72b79
Merge remote-tracking branch 'origin/main' into dev/migrie/f/non-term…
zadjii-msft Nov 15, 2021
8e43c9d
more tests
zadjii-msft Nov 15, 2021
2445ced
this passes all the tests
zadjii-msft Nov 16, 2021
25947c2
cleanup for the tests"
zadjii-msft Nov 16, 2021
056446c
guard some GetActiveTerminalControl calls
zadjii-msft Nov 16, 2021
841ced7
cant type when it's 26 degrees in here
zadjii-msft Nov 16, 2021
065e5f7
fix unit test build
zadjii-msft Nov 16, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,12 @@ namespace TerminalAppLocalTests

void TrustCommandlineTests::WslTests()
{
Log::Comment(L"We are explicitly deciding to not auto-approve "
L"`wsl.exe -d distro`-like commandlines. If we change this"
L" policy, remove this test.");

VERIFY_IS_FALSE(trust(L"C:\\Windows\\System32\\wsl"));
VERIFY_IS_FALSE(trust(L"C:\\Windows\\System32\\wsl.exe"));
VERIFY_IS_TRUE(trust(L"C:\\Windows\\System32\\wsl.exe"), L"This we will trust though, since it's an exe in system32");
VERIFY_IS_FALSE(trust(L"C:\\Windows\\System32\\wsl.exe -d Ubuntu"));
VERIFY_IS_FALSE(trust(L"wsl.exe"));
}
Expand All @@ -94,5 +98,8 @@ namespace TerminalAppLocalTests
{
VERIFY_IS_TRUE(trust(L"%ProgramFiles%\\PowerShell\\7\\pwsh.exe"));
VERIFY_IS_TRUE(trust(L"%LOCALAPPDATA%\\Microsoft\\WindowsApps\\pwsh.exe"));
VERIFY_IS_TRUE(trust(L"%ProgramFiles%\\PowerShell\\10\\pwsh.exe"));
VERIFY_IS_TRUE(trust(L"%ProgramFiles%\\PowerShell\\7.1.5\\pwsh.exe"));
VERIFY_IS_FALSE(trust(L"%ProgramFiles%\\PowerShell\\7\\pwsh.exe bad-stuff pwsh.exe"));
}
}
140 changes: 58 additions & 82 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1526,19 +1526,19 @@ namespace winrt::TerminalApp::implementation
return false;
}

const std::filesystem::path fullCommandlinePath{
const std::wstring fullCommandline{
wil::ExpandEnvironmentStringsW<std::wstring>(commandLine.data())
};

if (fullCommandlinePath.wstring().size() > systemDirectory.size())
if (fullCommandline.size() > systemDirectory.size())
{
// Get the first part of the executable path
const auto start = fullCommandlinePath.wstring().substr(0, systemDirectory.size());
const auto start = fullCommandline.substr(0, systemDirectory.size());
// Doing this as an ASCII only check might be wrong, but I'm
// guessing if system32 isn't at X:\windows\system32... this isn't
// the only thing that is going to be sad in Windows.
const auto pathEquals = til::equals_insensitive_ascii(start, systemDirectory);
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
if (pathEquals && std::filesystem::exists(fullCommandlinePath))
if (pathEquals && std::filesystem::exists(std::filesystem::path{ fullCommandline }))
{
return true;
}
Expand All @@ -1547,98 +1547,74 @@ namespace winrt::TerminalApp::implementation
// TODO! Remove the WSL allowing. it's trivial to insert some malicious
// stuff into WSL, via .bash_profile, so we're not giving them the (y)

// TODO! CommandlineToArgv to get the executable from the commandline.
// If there's one argc, and it's parent path is %ProgramFiles%, and it
// ends in pwsh.exe, then it's fine.

// Also, if the path is literally
// %SystemRoot%\System32\wsl.exe -d <distro name>
// then allow it.

// Largely stolen from _tryMangleStartingDirectoryForWSL in ConptyConnection.
// Find the first space, quote or the end of the string -- we'll look
// for wsl before that.
const auto terminator{ commandLine.find_first_of(LR"(" )", 1) }; // look past the first character in case it starts with "
const auto start{ til::at(commandLine, 0) == L'"' ? 1 : 0 };
const std::filesystem::path executablePath{ commandLine.substr(start, terminator - start) };
const auto executableFilename{ executablePath.filename().wstring() };

if (executableFilename == L"wsl" || executableFilename == L"wsl.exe")
// is does executablePath start with %ProgramFiles%\\PowerShell, and end
// with `pwsh.exe`?
const std::vector<std::filesystem::path> powershellCoreRoots
{
// We've got a WSL -- let's just make sure it's the right one.
if (executablePath.has_parent_path())
{
if (executablePath.parent_path().wstring() != systemDirectory)
{
return false; // it wasn't in system32!
}
}
else
{
// Unqualified WSL, this is dangerous, so return false.
return false;
}
// 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.


// Get everything after the wsl.exe
const auto arguments{ terminator == std::wstring_view::npos ?
std::wstring_view{} :
commandLine.substr(terminator + 1) };
const auto dashD{ arguments.find(L"-d ") };

// If we found a "-d " IMMEDIATELY AFTER wsl.exe. If it wasn't
// immediately after, it could have been `wsl.exe --doSomethingEvil`
if (dashD == 0)
{
// Using the string following "-d "...
const auto afterDashD{ arguments.substr(dashD + 3) };
// Find the next space
const auto afterFirstWord = afterDashD.find(L" ");
// if that space _wasn't_ at the end of the commandline, then
// there were some other args. That means it was `wsl -d distro
// anything`, and we should ask the user.
//
// So if it was at the end of the commandline, then there were
// no other args besides the distro name.
if (afterFirstWord == std::wstring::npos)
{
return true;
}
}
}
else if (executableFilename == L"pwsh" || executableFilename == L"pwsh.exe")
{
// is does executablePath start with %ProgramFiles%\\PowerShell?
const std::vector<std::filesystem::path> powershellCoreRoots
{
// Always look in "%ProgramFiles%
// Always look in "%ProgramFiles%\PowerShell"
{ wil::ExpandEnvironmentStringsW<std::wstring>(L"%ProgramFiles%\\PowerShell") },

#if defined(_M_AMD64) || defined(_M_ARM64) // No point in looking for WOW if we're not somewhere it exists
{ wil::ExpandEnvironmentStringsW<std::wstring>(L"%ProgramFiles(x86)%\\PowerShell") },
{ wil::ExpandEnvironmentStringsW<std::wstring>(L"%ProgramFiles(x86)%\\PowerShell") },
#endif

#if defined(_M_ARM64) // same with ARM
{
wil::ExpandEnvironmentStringsW<std::wstring>(L"%ProgramFiles(Arm)%\\PowerShell")
}
{
wil::ExpandEnvironmentStringsW<std::wstring>(L"%ProgramFiles(Arm)%\\PowerShell")
}
#endif
};
};

// TODO! CommandlineToArgv to get the executable from the commandline.
// If there's one argc, and it's parent path is %ProgramFiles%, and it
// ends in pwsh.exe, then it's fine.

// const auto terminator{ commandLine.find_first_of(LR"(" )", 1) }; // look past the first character in case it starts with "
// const auto start{ til::at(commandLine, 0) == L'"' ? 1 : 0 };
// const std::filesystem::path executablePath{ commandLine.substr(start, terminator - start) };
// const auto executableFilename{ executablePath.filename().wstring() };

const std::filesystem::path exePath{ fullCommandline };
exePath;

for (const auto& pwshRoot : powershellCoreRoots)
for (const auto& pwshRoot : powershellCoreRoots)
{
// Is the commandline's length (root.length + 8 + 3) characters long?
// * root.length: Length of the parent directory
// * 8: pwsh.exe
// * 3: `/7/` (or some other version number)
//
// NO

// Does the commandline start with this root, and end with pwsh.exe?
const auto startsWithRoot{ til::starts_with(fullCommandline, pwshRoot.c_str()) };
// const auto endsWithPwsh{ til::ends_with(fullCommandline, L"pwsh.exe") };
const auto endsWithPwsh{ exePath.filename() == L"pwsh.exe" };
// Is the filename of the exe `pwsh.exe`
if (startsWithRoot && endsWithPwsh)
{
// Is the path to the commandline actually exactly one of the
// versions that exists in this directory?
for (const auto& versionedDir : std::filesystem::directory_iterator(pwshRoot))
{
const auto versionedPath = versionedDir.path();
if (executablePath.parent_path() == versionedPath)
{
return true;
}
}
return true;
}
}

// if (executableFilename == L"pwsh" || executableFilename == L"pwsh.exe")
// {
// // Is the path to the commandline actually exactly one of the
// // versions that exists in this directory?
// for (const auto& versionedDir : std::filesystem::directory_iterator(pwshRoot))
// {
// const auto versionedPath = versionedDir.path();
// if (executablePath.parent_path() == versionedPath)
// {
// return true;
// }
// }
// }

return false;
}

Expand Down