From c79334ffbbcbb0d0483579035328aebd27f517f4 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 12 Nov 2021 18:58:43 -0600 Subject: [PATCH] Add a file for storing elevated-only state (#11222) ## Summary of the Pull Request This creates an `elevated-state.json` that lives in `%LOCALAPPDATA%` next to `state.json`, that's only writable when elevated. It doesn't _use_ this file for anything, it just puts the framework down for use later. It's _just like `ApplicationState`_. We'll use it the same way. It's readable when unelevated, which is nice, but not writable. If you're dumb and try to write to the file when unelevated, it'll just silently do nothing. If we try opening the file and find out the permissions are different, we'll _blow the file away entirely_. This is to prevent someone from renaming the original file (which they can do unelevated), then slapping a new file that's writable by them down in it's place. ## References * We're going to use this in #11096, but these PRs need to be broken up. ## PR Checklist * [x] Closes nothing * [x] I work here * [x] Tests added/passed * [ ] Requires documentation to be updated - maybe? not sure we have docs on `state.json` at all yet ## Validation Steps Performed I've played with this much more in `dev/migrie/f/non-terminal-content-elevation-warning` ###### followed by #11308, #11310 --- .github/actions/spelling/allow/allow.txt | 1 + .github/actions/spelling/allow/apis.txt | 7 + .github/actions/spelling/allow/names.txt | 1 + src/cascadia/TerminalApp/AppLogic.cpp | 59 ++- src/cascadia/TerminalApp/TerminalPage.cpp | 3 - .../ApplicationState.cpp | 335 +++++++++++++----- .../TerminalSettingsModel/ApplicationState.h | 65 ++-- .../ApplicationState.idl | 5 +- .../TerminalSettingsModel/FileUtils.cpp | 219 +++++++----- .../TerminalSettingsModel/FileUtils.h | 36 +- .../TerminalSettingsModel/JsonUtils.h | 1 - src/cascadia/TerminalSettingsModel/pch.h | 3 + src/cascadia/WindowsTerminal/AppHost.cpp | 32 +- src/types/inc/utils.hpp | 1 + src/types/utils.cpp | 32 ++ 15 files changed, 523 insertions(+), 277 deletions(-) diff --git a/.github/actions/spelling/allow/allow.txt b/.github/actions/spelling/allow/allow.txt index f23f8a581..6025abf76 100644 --- a/.github/actions/spelling/allow/allow.txt +++ b/.github/actions/spelling/allow/allow.txt @@ -1,3 +1,4 @@ +admins apc Apc bsd diff --git a/.github/actions/spelling/allow/apis.txt b/.github/actions/spelling/allow/apis.txt index 45f4e02f1..83ca55178 100644 --- a/.github/actions/spelling/allow/apis.txt +++ b/.github/actions/spelling/allow/apis.txt @@ -1,5 +1,7 @@ ACCEPTFILES ACCESSDENIED +acl +aclapi alignas alignof APPLYTOSUBMENUS @@ -21,6 +23,7 @@ commandlinetoargv cstdint CXICON CYICON +Dacl dataobject dcomp DERR @@ -117,15 +120,19 @@ OSVERSIONINFOEXW otms OUTLINETEXTMETRICW overridable +PACL PAGESCROLL +PEXPLICIT PICKFOLDERS pmr +ptstr rcx REGCLS RETURNCMD rfind roundf RSHIFT +SACL schandle semver serializer diff --git a/.github/actions/spelling/allow/names.txt b/.github/actions/spelling/allow/names.txt index 3635d3723..2a13d67ba 100644 --- a/.github/actions/spelling/allow/names.txt +++ b/.github/actions/spelling/allow/names.txt @@ -9,6 +9,7 @@ Diviness dsafa duhowett ekg +eryksun ethanschoonover Firefox Gatta diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index 7e5949577..ebc92d505 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -12,6 +12,8 @@ #include #include +#include "../../types/inc/utils.hpp" + using namespace winrt::Windows::ApplicationModel; using namespace winrt::Windows::ApplicationModel::DataTransfer; using namespace winrt::Windows::UI::Xaml; @@ -131,38 +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 -try -{ - wil::unique_handle processToken{ GetCurrentProcessToken() }; - const auto elevationType = wil::get_token_information(processToken.get()); - const auto elevationState = wil::get_token_information(processToken.get()); - if (elevationType == TokenElevationTypeDefault && elevationState.TokenIsElevated) - { - // In this case, the user has UAC entirely disabled. This is sort of - // weird, we treat this like the user isn't an admin at all. There's no - // separation of powers, so the things we normally want to gate on - // "having special powers" doesn't apply. - // - // See GH#7754, GH#11096 - return false; - } - - return wil::test_token_membership(nullptr, SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS); -} -catch (...) -{ - LOG_CAUGHT_EXCEPTION(); - return false; -} - namespace winrt::TerminalApp::implementation { // Function Description: @@ -214,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()]() { @@ -910,8 +880,6 @@ namespace winrt::TerminalApp::implementation void AppLogic::_RegisterSettingsChange() { const std::filesystem::path settingsPath{ std::wstring_view{ CascadiaSettings::SettingsPath() } }; - const std::filesystem::path statePath{ std::wstring_view{ ApplicationState::SharedInstance().FilePath() } }; - _reader.create( settingsPath.parent_path().c_str(), false, @@ -920,14 +888,29 @@ namespace winrt::TerminalApp::implementation // editors, who will write a temp file, then rename it to be the // actual file you wrote. So listen for that too. wil::FolderChangeEvents::FileName | wil::FolderChangeEvents::LastWriteTime, - [this, settingsBasename = settingsPath.filename(), stateBasename = statePath.filename()](wil::FolderChangeEvent, PCWSTR fileModified) { - const auto modifiedBasename = std::filesystem::path{ fileModified }.filename(); + [this, settingsBasename = settingsPath.filename()](wil::FolderChangeEvent, PCWSTR fileModified) { + // 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 invoke + // 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() }; if (modifiedBasename == settingsBasename) { _reloadSettings->Run(); } - else if (modifiedBasename == stateBasename) + else if (ApplicationState::SharedInstance().IsStatePath(modifiedBasename)) { _reloadState(); } diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index bd9787f04..81e2bc517 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -300,10 +300,7 @@ namespace winrt::TerminalApp::implementation // - true if the ApplicationState should be used. bool TerminalPage::ShouldUsePersistedLayout(CascadiaSettings& settings) const { - // GH#5000 Until there is a separate state file for elevated sessions we should just not - // save at all while in an elevated window. return Feature_PersistedWindowLayout::IsEnabled() && - !IsElevated() && settings.GlobalSettings().FirstWindowPreference() == FirstWindowPreference::PersistedWindowLayout; } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index 7e5bc1da3..d623ced29 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -9,8 +9,11 @@ #include "ActionAndArgs.h" #include "JsonUtils.h" #include "FileUtils.h" +#include "../../types/inc/utils.hpp" static constexpr std::wstring_view stateFileName{ L"state.json" }; +static constexpr std::wstring_view elevatedStateFileName{ L"elevated-state.json" }; + static constexpr std::string_view TabLayoutKey{ "tabLayout" }; static constexpr std::string_view InitialPositionKey{ "initialPosition" }; static constexpr std::string_view InitialSizeKey{ "initialSize" }; @@ -85,15 +88,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return trait.FromJson(root); } - // Returns the application-global ApplicationState object. - Microsoft::Terminal::Settings::Model::ApplicationState ApplicationState::SharedInstance() - { - static auto state = winrt::make_self(GetBaseSettingsPath() / stateFileName); - return *state; - } - - ApplicationState::ApplicationState(std::filesystem::path path) noexcept : - _path{ std::move(path) }, + ApplicationState::ApplicationState(const std::filesystem::path& stateRoot) noexcept : + _sharedPath{ stateRoot / stateFileName }, + _elevatedPath{ stateRoot / elevatedStateFileName }, _throttler{ std::chrono::seconds(1), [this]() { _write(); } } { _read(); @@ -102,9 +99,21 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // The destructor ensures that the last write is flushed to disk before returning. ApplicationState::~ApplicationState() { + TraceLoggingWrite(g_hSettingsModelProvider, + "ApplicationState_Dtor_Start", + TraceLoggingDescription("Event at the start of the ApplicationState destructor"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + // This will ensure that we not just cancel the last outstanding timer, // but instead force it to run as soon as possible and wait for it to complete. _throttler.flush(); + + TraceLoggingWrite(g_hSettingsModelProvider, + "ApplicationState_Dtor_End", + TraceLoggingDescription("Event at the end of the ApplicationState destructor"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); } // Re-read the state.json from disk. @@ -113,34 +122,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation _read(); } - // Returns the state.json path on the disk. - winrt::hstring ApplicationState::FilePath() const noexcept + bool ApplicationState::IsStatePath(const winrt::hstring& filename) { - return winrt::hstring{ _path.wstring() }; + static const auto sharedPath{ _sharedPath.filename() }; + static const auto elevatedPath{ _elevatedPath.filename() }; + return filename == sharedPath || filename == elevatedPath; } - // Generate all getter/setters -#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) \ - type ApplicationState::name() const noexcept \ - { \ - const auto state = _state.lock_shared(); \ - const auto& value = state->name; \ - return value ? *value : type{ __VA_ARGS__ }; \ - } \ - \ - void ApplicationState::name(const type& value) noexcept \ - { \ - { \ - auto state = _state.lock(); \ - state->name.emplace(value); \ - state->name##Changed = true; \ - } \ - \ - _throttler(); \ - } - MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) -#undef MTSM_APPLICATION_STATE_GEN - // Method Description: // - See GH#11119. Removes all of the data in this ApplicationState object // and resets it to the defaults. This will delete the state file! That's @@ -156,57 +144,58 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void ApplicationState::Reset() noexcept try { - LOG_LAST_ERROR_IF(!DeleteFile(_path.c_str())); + LOG_LAST_ERROR_IF(!DeleteFile(_sharedPath.c_str())); + LOG_LAST_ERROR_IF(!DeleteFile(_elevatedPath.c_str())); *_state.lock() = {}; } CATCH_LOG() - Json::Value ApplicationState::_getRoot(const locked_hfile& file) const noexcept - { - Json::Value root; - try - { - const auto data = ReadUTF8FileLocked(file); - if (data.empty()) - { - return root; - } - - std::string errs; - std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; - - if (!reader->parse(data.data(), data.data() + data.size(), &root, &errs)) - { - throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); - } - } - CATCH_LOG() - - return root; - } - - // Deserializes the state.json at _path into this ApplicationState. + // Deserializes the state.json and user-state (or elevated-state if + // elevated) into this ApplicationState. // * ANY errors during app state will result in the creation of a new empty state. // * ANY errors during runtime will result in changes being partially ignored. void ApplicationState::_read() const noexcept try { - auto state = _state.lock(); - const auto file = OpenFileReadSharedLocked(_path); + std::string errs; + std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; - auto root = _getRoot(file); - // GetValueForKey() comes in two variants: - // * take a std::optional reference - // * return std::optional by value - // At the time of writing the former version skips missing fields in the json, - // but we want to explicitly clear state fields that were removed from state.json. -#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) \ - if (!state->name##Changed) \ - { \ - state->name = JsonUtils::GetValueForKey>(root, key); \ - } - MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) -#undef MTSM_APPLICATION_STATE_GEN + // First get shared state out of `state.json`. + const auto sharedData = _readSharedContents().value_or(std::string{}); + if (!sharedData.empty()) + { + Json::Value root; + if (!reader->parse(sharedData.data(), sharedData.data() + sharedData.size(), &root, &errs)) + { + throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); + } + + // - If we're elevated, we want to only load the Shared properties + // from state.json. We'll then load the Local props from + // `elevated-state.json` + // - If we're unelevated, then load _everything_ from state.json. + if (::Microsoft::Console::Utils::IsElevated()) + { + // Only load shared properties if we're elevated + FromJson(root, FileSource::Shared); + + // Then, try and get anything in elevated-state + if (const auto localData{ _readLocalContents().value_or(std::string{}) }; !localData.empty()) + { + Json::Value root; + if (!reader->parse(localData.data(), localData.data() + localData.size(), &root, &errs)) + { + throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); + } + FromJson(root, FileSource::Local); + } + } + else + { + // If we're unelevated, then load everything. + FromJson(root, FileSource::Shared | FileSource::Local); + } + } } CATCH_LOG() @@ -214,29 +203,191 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // * Errors are only logged. // * _state->_writeScheduled is set to false, signaling our // setters that _synchronize() needs to be called again. - void ApplicationState::_write() noexcept + void ApplicationState::_write() const noexcept try { - // re-read the state so that we can only update the properties that were changed. - Json::Value root{}; + Json::StreamWriterBuilder wbuilder; + + // When we're elevated, we've got to be tricky. We don't want to write + // our window state, allowed commandlines, and other Local properties + // into the shared `state.json`. But, if we only serialize the Shared + // properties to a json blob, then we'll omit windowState entirely, + // _removing_ the window state of the unelevated instance. Oh no! + // + // So, to be tricky, we'll first _load_ the shared state to a json blob. + // We'll then serialize our view of the shared properties on top of that + // blob. Then we'll write that blob back to the file. This will + // round-trip the Local properties for the unelevated instances + // untouched in state.json + // + // After that's done, we'll write our Local properties into + // elevated-state.json. + if (::Microsoft::Console::Utils::IsElevated()) { - auto state = _state.lock(); - const auto file = OpenFileRWExclusiveLocked(_path); - root = _getRoot(file); + std::string errs; + std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; + Json::Value root; -#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) \ - if (state->name##Changed) \ - { \ - JsonUtils::SetValueForKey(root, key, state->name); \ - state->name##Changed = false; \ - } - MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) -#undef MTSM_APPLICATION_STATE_GEN + // First load the contents of state.json into a json blob. This will + // contain the Shared properties and the unelevated instance's Local + // properties. + const auto sharedData = _readSharedContents().value_or(std::string{}); + if (!sharedData.empty()) + { + if (!reader->parse(sharedData.data(), sharedData.data() + sharedData.size(), &root, &errs)) + { + throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); + } + } + // Layer our shared properties on top of the blob from state.json, + // and write it back out. + _writeSharedContents(Json::writeString(wbuilder, _toJsonWithBlob(root, FileSource::Shared))); - Json::StreamWriterBuilder wbuilder; - const auto content = Json::writeString(wbuilder, root); - WriteUTF8FileLocked(file, content); + // Finally, write our Local properties back to elevated-state.json + _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); + } + else + { + // We're unelevated, this is easy. Just write everything back out. + _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local | FileSource::Shared))); } } CATCH_LOG() + + // Returns the application-global ApplicationState object. + Microsoft::Terminal::Settings::Model::ApplicationState ApplicationState::SharedInstance() + { + std::filesystem::path root{ GetBaseSettingsPath() }; + static auto state = winrt::make_self(root); + return *state; + } + + // Method Description: + // - Loads data from the given json blob. Will only read the data that's in + // the specified parseSource - so if we're reading the Local state file, + // we won't destroy previously parsed Shared data. + // - READ: there's no layering for app state. + void ApplicationState::FromJson(const Json::Value& root, FileSource parseSource) const noexcept + { + auto state = _state.lock(); + // GetValueForKey() comes in two variants: + // * take a std::optional reference + // * return std::optional by value + // At the time of writing the former version skips missing fields in the json, + // but we want to explicitly clear state fields that were removed from state.json. + // + // GH#11222: We only load properties that are of the same type (Local or + // Shared) which we requested. If we didn't want to load this type of + // property, just skip it. +#define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) \ + if (WI_IsFlagSet(parseSource, source)) \ + state->name = JsonUtils::GetValueForKey>(root, key); + + MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) +#undef MTSM_APPLICATION_STATE_GEN + } + + Json::Value ApplicationState::ToJson(FileSource parseSource) const noexcept + { + Json::Value root{ Json::objectValue }; + return _toJsonWithBlob(root, parseSource); + } + + Json::Value ApplicationState::_toJsonWithBlob(Json::Value& root, FileSource parseSource) const noexcept + { + { + auto state = _state.lock_shared(); + + // GH#11222: We only write properties that are of the same type (Local + // or Shared) which we requested. If we didn't want to serialize this + // type of property, just skip it. +#define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) \ + if (WI_IsFlagSet(parseSource, source)) \ + JsonUtils::SetValueForKey(root, key, state->name); + + MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) +#undef MTSM_APPLICATION_STATE_GEN + } + return root; + } + + // Generate all getter/setters +#define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) \ + type ApplicationState::name() const noexcept \ + { \ + const auto state = _state.lock_shared(); \ + const auto& value = state->name; \ + return value ? *value : type{ __VA_ARGS__ }; \ + } \ + \ + void ApplicationState::name(const type& value) noexcept \ + { \ + { \ + auto state = _state.lock(); \ + state->name.emplace(value); \ + } \ + \ + _throttler(); \ + } + MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) +#undef MTSM_APPLICATION_STATE_GEN + + // Method Description: + // - Read the contents of our "shared" state - state that should be shared + // for elevated and unelevated instances. This is things like the list of + // generated profiles, the command palette commandlines. + std::optional ApplicationState::_readSharedContents() const + { + return ReadUTF8FileIfExists(_sharedPath); + } + + // Method Description: + // - Read the contents of our "local" state - state that should be kept in + // separate files for elevated and unelevated instances. This is things + // like the persisted window state, and the approved commandlines (though, + // those don't matter when unelevated). + // - When elevated, this will DELETE `elevated-state.json` if it has bad + // permissions, so we don't potentially read malicious data. + std::optional ApplicationState::_readLocalContents() const + { + return ::Microsoft::Console::Utils::IsElevated() ? + ReadUTF8FileIfExists(_elevatedPath, true) : + ReadUTF8FileIfExists(_sharedPath, false); + } + + // Method Description: + // - Write the contents of our "shared" state - state that should be shared + // for elevated and unelevated instances. This will atomically write to + // `state.json` + void ApplicationState::_writeSharedContents(const std::string_view content) const + { + WriteUTF8FileAtomic(_sharedPath, content); + } + + // Method Description: + // - Write the contents of our "local" state - state that should be kept in + // separate files for elevated and unelevated instances. When elevated, + // this will write to `elevated-state.json`, and when unelevated, this + // will atomically write to `user-state.json` + void ApplicationState::_writeLocalContents(const std::string_view content) const + { + if (::Microsoft::Console::Utils::IsElevated()) + { + // DON'T use WriteUTF8FileAtomic, which will write to a temporary file + // then rename that file to the final filename. That actually lets us + // overwrite the elevate file's contents even when unelevated, because + // we're effectively deleting the original file, then renaming a + // different file in it's place. + // + // We're not worried about someone else doing that though, if they do + // that with the wrong permissions, then we'll just ignore the file and + // start over. + WriteUTF8File(_elevatedPath, content, true); + } + else + { + WriteUTF8FileAtomic(_sharedPath, content); + } + } + } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index d6d2d1749..1c0943cfe 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -16,21 +16,30 @@ Abstract: #include "WindowLayout.g.h" #include -#include -#include -#include "FileUtils.h" #include +namespace winrt::Microsoft::Terminal::Settings::Model::implementation +{ + // If a property is Shared, then it'll be stored in `state.json`, and used + // in both elevated and unelevated instances of the Terminal. If a property + // is marked Local, then it will have separate values for elevated and + // unelevated instances. + enum FileSource : int + { + Shared = 0x1, + Local = 0x2 + }; + DEFINE_ENUM_FLAG_OPERATORS(FileSource); + // This macro generates all getters and setters for ApplicationState. // It provides X with the following arguments: -// (type, function name, JSON key, ...variadic construction arguments) -namespace winrt::Microsoft::Terminal::Settings::Model::implementation -{ -#define MTSM_APPLICATION_STATE_FIELDS(X) \ - X(std::unordered_set, GeneratedProfiles, "generatedProfiles") \ - X(Windows::Foundation::Collections::IVector, PersistedWindowLayouts, "persistedWindowLayouts") \ - X(Windows::Foundation::Collections::IVector, RecentCommands, "recentCommands") \ - X(Windows::Foundation::Collections::IVector, DismissedMessages, "dismissedMessages") +// (source, type, function name, JSON key, ...variadic construction arguments) +#define MTSM_APPLICATION_STATE_FIELDS(X) \ + X(FileSource::Shared, std::unordered_set, GeneratedProfiles, "generatedProfiles") \ + X(FileSource::Local, Windows::Foundation::Collections::IVector, PersistedWindowLayouts, "persistedWindowLayouts") \ + X(FileSource::Shared, Windows::Foundation::Collections::IVector, RecentCommands, "recentCommands") \ + X(FileSource::Shared, Windows::Foundation::Collections::IVector, DismissedMessages, "dismissedMessages") \ + X(FileSource::Local, Windows::Foundation::Collections::IVector, AllowedCommandlines, "allowedCommandlines") struct WindowLayout : WindowLayoutT { @@ -44,23 +53,26 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation friend ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait; }; - struct ApplicationState : ApplicationStateT + struct ApplicationState : public ApplicationStateT { static Microsoft::Terminal::Settings::Model::ApplicationState SharedInstance(); - ApplicationState(std::filesystem::path path) noexcept; + ApplicationState(const std::filesystem::path& stateRoot) noexcept; ~ApplicationState(); // Methods void Reload() const noexcept; void Reset() noexcept; + void FromJson(const Json::Value& root, FileSource parseSource) const noexcept; + Json::Value ToJson(FileSource parseSource) const noexcept; + // General getters/setters - winrt::hstring FilePath() const noexcept; + bool IsStatePath(const winrt::hstring& filename); // State getters/setters -#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) \ - type name() const noexcept; \ +#define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) \ + type name() const noexcept; \ void name(const type& value) noexcept; MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) #undef MTSM_APPLICATION_STATE_GEN @@ -68,21 +80,24 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: struct state_t { -#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) \ - std::optional name{ __VA_ARGS__ }; \ - bool name##Changed = false; - +#define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) std::optional name{ __VA_ARGS__ }; MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) #undef MTSM_APPLICATION_STATE_GEN }; + til::shared_mutex _state; + std::filesystem::path _sharedPath; + std::filesystem::path _elevatedPath; + til::throttled_func_trailing<> _throttler; - Json::Value _getRoot(const winrt::Microsoft::Terminal::Settings::Model::locked_hfile& file) const noexcept; - void _write() noexcept; + void _write() const noexcept; void _read() const noexcept; - std::filesystem::path _path; - til::shared_mutex _state; - til::throttled_func_trailing<> _throttler; + Json::Value _toJsonWithBlob(Json::Value& root, FileSource parseSource) const noexcept; + + std::optional _readSharedContents() const; + void _writeSharedContents(const std::string_view content) const; + std::optional _readLocalContents() const; + void _writeLocalContents(const std::string_view content) const; }; } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.idl b/src/cascadia/TerminalSettingsModel/ApplicationState.idl index 7890b58c6..b88ec144a 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.idl +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.idl @@ -30,12 +30,15 @@ namespace Microsoft.Terminal.Settings.Model void Reload(); void Reset(); - String FilePath { get; }; + Boolean IsStatePath(String filename); Windows.Foundation.Collections.IVector PersistedWindowLayouts { get; set; }; Windows.Foundation.Collections.IVector RecentCommands { get; set; }; Windows.Foundation.Collections.IVector DismissedMessages { get; set; }; + + Windows.Foundation.Collections.IVector AllowedCommandlines { get; set; }; + } } diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index 599d29a55..4535c23be 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -8,6 +8,10 @@ #include #include +#include +#include +#include + static constexpr std::string_view Utf8Bom{ u8"\uFEFF" }; static constexpr std::wstring_view UnpackagedSettingsFolderName{ L"Microsoft\\Windows Terminal\\" }; @@ -39,86 +43,44 @@ namespace winrt::Microsoft::Terminal::Settings::Model return baseSettingsPath; } - locked_hfile OpenFileReadSharedLocked(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 will be checking to see if the file is owned + // by the Builtin\Administrators group. If it's not, then it was likely + // tampered with. + // Arguments: + // - handle: a HANDLE to the file to check + // Return Value: + // - true if it had the expected permissions. False otherwise. + static bool _isOwnedByAdministrators(const HANDLE& handle) { - wil::unique_hfile file{ CreateFileW(path.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr) }; - THROW_LAST_ERROR_IF(!file); - // just lock the entire file - OVERLAPPED sOverlapped; - sOverlapped.Offset = 0; - sOverlapped.OffsetHigh = 0; - // Shared lock - THROW_LAST_ERROR_IF(!LockFileEx(file.get(), - 0, // lock shared, wait to return until lock is obtained - 0, // reserved, does nothing - INT_MAX, // lock INT_MAX bytes - 0, // higher-order bytes, if our state file is greater than 2GB I guess this will be a problem - &sOverlapped)); - return { std::move(file), sOverlapped }; + // If the file is owned by the administrators group, trust the + // administrators instead of checking the DACL permissions. It's simpler + // and more flexible. + + wil::unique_hlocal_security_descriptor sd; + PSID psidOwner{ nullptr }; + // The psidOwner pointer references the security descriptor, so it + // doesn't have to be freed separate from sd. + const auto status = GetSecurityInfo(handle, + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION, + &psidOwner, + nullptr, + nullptr, + nullptr, + wil::out_param_ptr(sd)); + THROW_IF_WIN32_ERROR(status); + + wil::unique_any_psid psidAdmins{ nullptr }; + THROW_IF_WIN32_BOOL_FALSE( + ConvertStringSidToSidW(L"BA", wil::out_param_ptr(psidAdmins))); + + return EqualSid(psidOwner, psidAdmins.get()); } - - locked_hfile OpenFileRWExclusiveLocked(const std::filesystem::path& path) - { - wil::unique_hfile file{ CreateFileW(path.c_str(), GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr) }; - THROW_LAST_ERROR_IF(!file); - // just lock the entire file - OVERLAPPED sOverlapped; - sOverlapped.Offset = 0; - sOverlapped.OffsetHigh = 0; - // Shared lock - THROW_LAST_ERROR_IF(!LockFileEx(file.get(), - LOCKFILE_EXCLUSIVE_LOCK, // lock exclusive, wait to return until lock is obtained - 0, // reserved, does nothing - INT_MAX, // lock INT_MAX bytes - 0, // higher-order bytes, if our state file is greater than 2GB I guess this will be a problem - &sOverlapped)); - return { std::move(file), sOverlapped }; - } - - std::string ReadUTF8FileLocked(const locked_hfile& file) - { - const auto fileSize = GetFileSize(file.get(), nullptr); - THROW_LAST_ERROR_IF(fileSize == INVALID_FILE_SIZE); - - // By making our buffer just slightly larger we can detect if - // the file size changed and we've failed to read the full file. - std::string buffer(static_cast(fileSize) + 1, '\0'); - DWORD bytesRead = 0; - THROW_IF_WIN32_BOOL_FALSE(ReadFile(file.get(), buffer.data(), gsl::narrow(buffer.size()), &bytesRead, nullptr)); - - // As mentioned before our buffer was allocated oversized. - buffer.resize(bytesRead); - - if (til::starts_with(buffer, Utf8Bom)) - { - // Yeah this memmove()s the entire content. - // But I don't really want to deal with UTF8 BOMs any more than necessary, - // as basically not a single editor writes a BOM for UTF8. - buffer.erase(0, Utf8Bom.size()); - } - - return buffer; - } - - void WriteUTF8FileLocked(const locked_hfile& file, const std::string_view& content) - { - // truncate the file because we want to overwrite it - SetFilePointer(file.get(), 0, nullptr, FILE_BEGIN); - THROW_IF_WIN32_BOOL_FALSE(SetEndOfFile(file.get())); - - const auto fileSize = gsl::narrow(content.size()); - DWORD bytesWritten = 0; - THROW_IF_WIN32_BOOL_FALSE(WriteFile(file.get(), content.data(), fileSize, &bytesWritten, nullptr)); - - if (bytesWritten != fileSize) - { - THROW_WIN32_MSG(ERROR_WRITE_FAULT, "failed to write whole file"); - } - } - // Tries to read a file somewhat atomically without locking it. // Strips the UTF8 BOM if it exists. - std::string ReadUTF8File(const std::filesystem::path& path) + std::string ReadUTF8File(const std::filesystem::path& path, const bool elevatedOnly) { // From some casual observations we can determine that: // * ReadFile() always returns the requested amount of data (unless the file is smaller) @@ -126,9 +88,40 @@ namespace winrt::Microsoft::Terminal::Settings::Model // -> Lets add a retry-loop just in case, to not fail if the file size changed while reading. for (int i = 0; i < 3; ++i) { - wil::unique_hfile file{ CreateFileW(path.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr) }; + wil::unique_hfile file{ CreateFileW(path.c_str(), + GENERIC_READ, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + nullptr, + OPEN_EXISTING, + FILE_ATTRIBUTE_NORMAL, + nullptr) }; THROW_LAST_ERROR_IF(!file); + // Open the file _first_, then check if it has the right + // permissions. This prevents a "Time-of-check to time-of-use" + // vulnerability where a malicious exe could delete the file and + // replace it between us checking the permissions, and reading the + // contents. We've got a handle to the file now, which means we're + // going to read the contents of that instance of the file + // regardless. If someone replaces the file on us before we get to + // the GetSecurityInfo call below, then only the subsequent call to + // ReadUTF8File will notice it. + if (elevatedOnly) + { + const bool hadExpectedPermissions{ _isOwnedByAdministrators(file.get()) }; + if (!hadExpectedPermissions) + { + // Close the handle + file.reset(); + + // delete the file. It's been compromised. + LOG_LAST_ERROR_IF(!DeleteFile(path.c_str())); + + // Exit early, because obviously there's nothing to read from the deleted file. + return ""; + } + } + const auto fileSize = GetFileSize(file.get(), nullptr); THROW_LAST_ERROR_IF(fileSize == INVALID_FILE_SIZE); @@ -166,11 +159,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model } // Same as ReadUTF8File, but returns an empty optional, if the file couldn't be opened. - std::optional ReadUTF8FileIfExists(const std::filesystem::path& path) + std::optional ReadUTF8FileIfExists(const std::filesystem::path& path, const bool elevatedOnly) { try { - return { ReadUTF8File(path) }; + return { ReadUTF8File(path, elevatedOnly) }; } catch (const wil::ResultException& exception) { @@ -183,9 +176,70 @@ namespace winrt::Microsoft::Terminal::Settings::Model } } - void WriteUTF8File(const std::filesystem::path& path, const std::string_view& content) + void WriteUTF8File(const std::filesystem::path& path, + const std::string_view& content, + const bool elevatedOnly) { - wil::unique_hfile file{ CreateFileW(path.c_str(), GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr) }; + SECURITY_ATTRIBUTES sa; + // stash the security descriptor here, so it will stay in context until + // after the call to CreateFile. If it gets cleaned up before that, then + // CreateFile will fail + wil::unique_hlocal_security_descriptor sd; + if (elevatedOnly) + { + // Initialize the security descriptor so only admins can write the + // file. We'll initialize the SECURITY_DESCRIPTOR with a + // single entry (ACE) -- a mandatory label (i.e. a + // LABEL_SECURITY_INFORMATION) that sets the file integrity level to + // "high", with a no-write-up policy. + // + // When accessed from a security context at a lower integrity level, + // the no-write-up policy filters out rights that aren't in the + // object type's generic read and execute set (for the file type, + // that's FILE_GENERIC_READ | FILE_GENERIC_EXECUTE). + // + // Another option we considered here was manually setting the ACLs + // on this file such that Builtin\Admins could read&write the file, + // and all users could only read. + // + // Big thanks to @eryksun in GH#11222 for helping with this. This + // alternative method was chosen because it's considerably simpler. + + // The required security descriptor can be created easily from the + // SDDL string: "S:(ML;;NW;;;HI)" + // (i.e. SACL:mandatory label;;no write up;;;high integrity level) + unsigned long cb; + THROW_IF_WIN32_BOOL_FALSE( + ConvertStringSecurityDescriptorToSecurityDescriptor(L"S:(ML;;NW;;;HI)", + SDDL_REVISION_1, + wil::out_param_ptr(sd), + &cb)); + + // Initialize a security attributes structure. + sa.nLength = sizeof(SECURITY_ATTRIBUTES); + sa.lpSecurityDescriptor = sd.get(); + sa.bInheritHandle = false; + + // If we're running in an elevated context, when this file is + // created, it will automatically be owned by + // Builtin\Administrators, which will pass the above + // _isOwnedByAdministrators check. + // + // Programs running in an elevated context will be free to write the + // file, and unelevated processes will be able to read the file. An + // unelevated process could always delete the file and rename a new + // file in it's place (a la the way `vim.exe` saves files), but if + // they do that, the new file _won't_ be owned by Administrators, + // failing the above check. + } + + wil::unique_hfile file{ CreateFileW(path.c_str(), + GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_DELETE, + elevatedOnly ? &sa : nullptr, + CREATE_ALWAYS, + FILE_ATTRIBUTE_NORMAL, + nullptr) }; THROW_LAST_ERROR_IF(!file); const auto fileSize = gsl::narrow(content.size()); @@ -198,7 +252,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model } } - void WriteUTF8FileAtomic(const std::filesystem::path& path, const std::string_view& content) + void WriteUTF8FileAtomic(const std::filesystem::path& path, + const std::string_view& content) { // GH#10787: rename() will replace symbolic links themselves and not the path they point at. // It's thus important that we first resolve them before generating temporary path. diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.h b/src/cascadia/TerminalSettingsModel/FileUtils.h index 187051e7c..082da6611 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.h +++ b/src/cascadia/TerminalSettingsModel/FileUtils.h @@ -1,41 +1,11 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. -#pragma once - namespace winrt::Microsoft::Terminal::Settings::Model { - // I couldn't find a wil helper for this so I made it myself - class locked_hfile - { - public: - wil::unique_hfile file; - OVERLAPPED lockedRegion; - - ~locked_hfile() - { - if (file) - { - // Need to unlock the file before it is closed - UnlockFileEx(file.get(), 0, INT_MAX, 0, &lockedRegion); - } - } - - HANDLE get() const noexcept - { - return file.get(); - } - }; - std::filesystem::path GetBaseSettingsPath(); - - locked_hfile OpenFileReadSharedLocked(const std::filesystem::path& path); - locked_hfile OpenFileRWExclusiveLocked(const std::filesystem::path& path); - std::string ReadUTF8FileLocked(const locked_hfile& file); - void WriteUTF8FileLocked(const locked_hfile& file, const std::string_view& content); - - std::string ReadUTF8File(const std::filesystem::path& path); - std::optional ReadUTF8FileIfExists(const std::filesystem::path& path); - void WriteUTF8File(const std::filesystem::path& path, const std::string_view& content); + std::string ReadUTF8File(const std::filesystem::path& path, const bool elevatedOnly = false); + std::optional ReadUTF8FileIfExists(const std::filesystem::path& path, const bool elevatedOnly = false); + void WriteUTF8File(const std::filesystem::path& path, const std::string_view& content, const bool elevatedOnly = false); void WriteUTF8FileAtomic(const std::filesystem::path& path, const std::string_view& content); } diff --git a/src/cascadia/TerminalSettingsModel/JsonUtils.h b/src/cascadia/TerminalSettingsModel/JsonUtils.h index cea006b2b..be29f15de 100644 --- a/src/cascadia/TerminalSettingsModel/JsonUtils.h +++ b/src/cascadia/TerminalSettingsModel/JsonUtils.h @@ -382,7 +382,6 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils }; template - struct ConversionTrait> { std::unordered_map FromJson(const Json::Value& json) const diff --git a/src/cascadia/TerminalSettingsModel/pch.h b/src/cascadia/TerminalSettingsModel/pch.h index d5473939c..25773a45d 100644 --- a/src/cascadia/TerminalSettingsModel/pch.h +++ b/src/cascadia/TerminalSettingsModel/pch.h @@ -53,3 +53,6 @@ TRACELOGGING_DECLARE_PROVIDER(g_hSettingsModelProvider); // Manually include til after we include Windows.Foundation to give it winrt superpowers #include "til.h" + +#include +#include diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 544c33e55..36d489212 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -845,8 +845,30 @@ winrt::Windows::Foundation::IAsyncAction AppHost::_SaveWindowLayouts() if (_logic.ShouldUsePersistedLayout()) { - const auto layoutJsons = _windowManager.GetAllWindowLayouts(); - _logic.SaveWindowLayoutJsons(layoutJsons); + try + { + TraceLoggingWrite(g_hWindowsTerminalProvider, + "AppHost_SaveWindowLayouts_Collect", + TraceLoggingDescription("Logged when collecting window state"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + const auto layoutJsons = _windowManager.GetAllWindowLayouts(); + TraceLoggingWrite(g_hWindowsTerminalProvider, + "AppHost_SaveWindowLayouts_Save", + TraceLoggingDescription("Logged when writing window state"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + _logic.SaveWindowLayoutJsons(layoutJsons); + } + catch (...) + { + LOG_CAUGHT_EXCEPTION(); + TraceLoggingWrite(g_hWindowsTerminalProvider, + "AppHost_SaveWindowLayouts_Failed", + TraceLoggingDescription("An error occurred when collecting or writing window state"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + } } co_return; @@ -867,6 +889,12 @@ winrt::fire_and_forget AppHost::_SaveWindowLayoutsRepeat() // per 10 seconds, if a save is requested by another source simultaneously. if (_getWindowLayoutThrottler.has_value()) { + TraceLoggingWrite(g_hWindowsTerminalProvider, + "AppHost_requestGetLayout", + TraceLoggingDescription("Logged when triggering a throttled write of the window state"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + _getWindowLayoutThrottler.value()(); } } diff --git a/src/types/inc/utils.hpp b/src/types/inc/utils.hpp index a2a2c6cac..9bd1c3141 100644 --- a/src/types/inc/utils.hpp +++ b/src/types/inc/utils.hpp @@ -94,4 +94,5 @@ namespace Microsoft::Console::Utils GUID CreateV5Uuid(const GUID& namespaceGuid, const gsl::span name); + bool IsElevated(); } diff --git a/src/types/utils.cpp b/src/types/utils.cpp index d8af05c2e..1dc18a68e 100644 --- a/src/types/utils.cpp +++ b/src/types/utils.cpp @@ -5,6 +5,8 @@ #include "inc/utils.hpp" #include "inc/colorTable.hpp" +#include + using namespace Microsoft::Console; // Routine Description: @@ -559,3 +561,33 @@ GUID Utils::CreateV5Uuid(const GUID& namespaceGuid, const gsl::span(processToken.get()); + const auto elevationState = wil::get_token_information(processToken.get()); + if (elevationType == TokenElevationTypeDefault && elevationState.TokenIsElevated) + { + // In this case, the user has UAC entirely disabled. This is sort of + // weird, we treat this like the user isn't an admin at all. There's no + // separation of powers, so the things we normally want to gate on + // "having special powers" doesn't apply. + // + // See GH#7754, GH#11096 + return false; + } + + return wil::test_token_membership(nullptr, SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS); + } + catch (...) + { + LOG_CAUGHT_EXCEPTION(); + return false; + } + }(); + return isElevated; +}