From 4976a091a057a6ffd49a06ac764534931265f45e Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 4 Nov 2021 12:48:45 -0500 Subject: [PATCH] this is a collection of the trivial nits while I wait on a reply to the hard questions --- src/cascadia/TerminalApp/AppLogic.cpp | 33 +++++++++---------- .../TerminalSettingsModel/FileUtils.cpp | 33 +++++++++---------- src/types/utils.cpp | 2 +- 3 files changed, 32 insertions(+), 36 deletions(-) diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index 047750383..6df506210 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -133,18 +133,6 @@ static Documents::Run _BuildErrorRun(const winrt::hstring& text, const ResourceD return textRun; } -// Method Description: -// - Returns whether the user is either a member of the Administrators group or -// is currently elevated. -// - This will return **FALSE** if the user has UAC disabled entirely, because -// there's no separation of power between the user and an admin in that case. -// Return Value: -// - true if the user is an administrator -static bool _isUserAdmin() noexcept -{ - return Microsoft::Console::Utils::IsElevated(); -} - namespace winrt::TerminalApp::implementation { // Function Description: @@ -196,7 +184,7 @@ namespace winrt::TerminalApp::implementation // The TerminalPage has to be constructed during our construction, to // make sure that there's a terminal page for callers of // SetTitleBarContent - _isElevated = _isUserAdmin(); + _isElevated = Microsoft::Console::Utils::IsElevated(); _root = winrt::make_self(); _reloadSettings = std::make_shared>(winrt::Windows::System::DispatcherQueue::GetForCurrentThread(), std::chrono::milliseconds(100), [weakSelf = get_weak()]() { @@ -898,11 +886,20 @@ namespace winrt::TerminalApp::implementation // actual file you wrote. So listen for that too. wil::FolderChangeEvents::FileName | wil::FolderChangeEvents::LastWriteTime, [this, settingsBasename = settingsPath.filename()](wil::FolderChangeEvent, PCWSTR fileModified) { - // DO NOT create a static reference to - // ApplicationState::SharedInstance here. See - // https://github.com/microsoft/terminal/pull/11222/files/9ff2775122a496fb8b1bcc7a0b83a64ce5b26c5f#r719627541 - // for why. ApplicationState::SharedInstance already caches it's - // own static ref. + // DO NOT create a static reference to ApplicationState::SharedInstance here. + // + // ApplicationState::SharedInstance already caches its own + // static ref. If _we_ keep a static ref to the member in + // AppState, then our reference will keep ApplicationState alive + // after the `ActionToStringMap` gets cleaned up. Then, when we + // try to persist the actions in the window state, we won't be + // able to. We'll try to look up the action and the map just + // won't exist. We'll explode, even though the Terminal is + // tearing down anyways. So we'll just die, but still inboke + // WinDBG's post-mortem debugger, who won't be able to attach to + // the process that's already exiting. + // + // So DON'T ~give a mouse a cookie~ take a static ref here. const winrt::hstring modifiedBasename{ std::filesystem::path{ fileModified }.filename().c_str() }; diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index 845b37567..b509fdc3b 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -41,7 +41,17 @@ namespace winrt::Microsoft::Terminal::Settings::Model return baseSettingsPath; } - static bool _hasExpectedPermissions(const std::filesystem::path& path) + // Function Description: + // - Checks the permissions on this file, to make sure it can only be opened + // for writing by admins. We want to make sure that: + // * Everyone has permission to read + // * Admins can do anything + // * No one else can do anything. + // Arguments: + // - path: the path to the file to check + // Return Value: + // - true if it had the expected permissions. False otherwise. + static bool _hasElevatedOnlyPermissions(const std::filesystem::path& path) { // If we want to only open the file if it's elevated, check the // permissions on this file. We want to make sure that: @@ -74,17 +84,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model // Now, get the Everyone and Admins SIDS so we can make sure they're // the ones in this file. - - wil::unique_sid everyoneSid; - wil::unique_sid adminGroupSid; - SID_IDENTIFIER_AUTHORITY SIDAuthNT = SECURITY_NT_AUTHORITY; - SID_IDENTIFIER_AUTHORITY SIDAuthWorld = SECURITY_WORLD_SID_AUTHORITY; - // Create a SID for the BUILTIN\Administrators group. - THROW_IF_WIN32_BOOL_FALSE(AllocateAndInitializeSid(&SIDAuthNT, 2, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS, 0, 0, 0, 0, 0, 0, &adminGroupSid)); + const auto adminGroupSid = wil::make_static_sid(SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS); // Create a well-known SID for the Everyone group. - THROW_IF_WIN32_BOOL_FALSE(AllocateAndInitializeSid(&SIDAuthWorld, 1, SECURITY_WORLD_RID, 0, 0, 0, 0, 0, 0, 0, &everyoneSid)); + const auto everyoneSid = wil::make_static_sid(SECURITY_WORLD_SID_AUTHORITY, SECURITY_WORLD_RID); bool hadExpectedPermissions = true; @@ -114,7 +118,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model { if (elevatedOnly) { - const bool hadExpectedPermissions{ _hasExpectedPermissions(path) }; + const bool hadExpectedPermissions{ _hasElevatedOnlyPermissions(path) }; if (!hadExpectedPermissions) { // delete the file. It's been compromised. @@ -206,16 +210,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model // SYSTEM, but if I did that, then even we can't write the file // while elevated, which isn't what we want. - wil::unique_sid everyoneSid; - wil::unique_sid adminGroupSid; - SID_IDENTIFIER_AUTHORITY SIDAuthNT = SECURITY_NT_AUTHORITY; - SID_IDENTIFIER_AUTHORITY SIDAuthWorld = SECURITY_WORLD_SID_AUTHORITY; - // Create a SID for the BUILTIN\Administrators group. - THROW_IF_WIN32_BOOL_FALSE(AllocateAndInitializeSid(&SIDAuthNT, 2, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS, 0, 0, 0, 0, 0, 0, &adminGroupSid)); + const auto adminGroupSid = wil::make_static_sid(SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS); // Create a well-known SID for the Everyone group. - THROW_IF_WIN32_BOOL_FALSE(AllocateAndInitializeSid(&SIDAuthWorld, 1, SECURITY_WORLD_RID, 0, 0, 0, 0, 0, 0, 0, &everyoneSid)); + const auto everyoneSid = wil::make_static_sid(SECURITY_WORLD_SID_AUTHORITY, SECURITY_WORLD_RID); EXPLICIT_ACCESS ea[2]{}; diff --git a/src/types/utils.cpp b/src/types/utils.cpp index 59919bba7..1dc18a68e 100644 --- a/src/types/utils.cpp +++ b/src/types/utils.cpp @@ -5,7 +5,7 @@ #include "inc/utils.hpp" #include "inc/colorTable.hpp" -#include +#include using namespace Microsoft::Console;