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; +}