From 6be697221d6c2b0099535cc9ac9fce5ab5e97f8d Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 14 Sep 2021 05:44:25 -0500 Subject: [PATCH 01/30] This is everything from dev/migrie/f/non-terminal-content-elevation-warning for specifically ElevatedState (cherry picked from commit 97b0f065047b1591c3b125bd0a7b131505c44aca) --- patch.diff | 896 ++++++++++++++++++ src/cascadia/LocalTests_SettingsModel/pch.h | 3 + src/cascadia/TerminalApp/AppLogic.cpp | 13 +- .../ApplicationState.cpp | 98 +- .../TerminalSettingsModel/ApplicationState.h | 19 +- .../BaseApplicationState.cpp | 92 ++ .../BaseApplicationState.h | 36 + .../TerminalSettingsModel/ElevatedState.cpp | 134 +++ .../TerminalSettingsModel/ElevatedState.h | 60 ++ .../TerminalSettingsModel/ElevatedState.idl | 15 + .../TerminalSettingsModel/FileUtils.cpp | 160 +++- .../TerminalSettingsModel/FileUtils.h | 6 +- .../TerminalSettingsModel/JsonUtils.h | 1 - ...crosoft.Terminal.Settings.ModelLib.vcxproj | 9 + src/cascadia/TerminalSettingsModel/pch.h | 3 + 15 files changed, 1439 insertions(+), 106 deletions(-) create mode 100644 patch.diff create mode 100644 src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp create mode 100644 src/cascadia/TerminalSettingsModel/BaseApplicationState.h create mode 100644 src/cascadia/TerminalSettingsModel/ElevatedState.cpp create mode 100644 src/cascadia/TerminalSettingsModel/ElevatedState.h create mode 100644 src/cascadia/TerminalSettingsModel/ElevatedState.idl diff --git a/patch.diff b/patch.diff new file mode 100644 index 00000000000..6f59399e38c --- /dev/null +++ b/patch.diff @@ -0,0 +1,896 @@ +diff --git a/src/cascadia/LocalTests_SettingsModel/pch.h b/src/cascadia/LocalTests_SettingsModel/pch.h +index 53f6145d2..01b4cdfe8 100644 +--- a/src/cascadia/LocalTests_SettingsModel/pch.h ++++ b/src/cascadia/LocalTests_SettingsModel/pch.h +@@ -61,6 +61,9 @@ Author(s): + // Manually include til after we include Windows.Foundation to give it winrt superpowers + #include "til.h" + ++#include ++#include ++ + // Common includes for most tests: + #include "../../inc/argb.h" + #include "../../inc/conattrs.hpp" +diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp +index 6d53a6ba7..38edbc2c9 100644 +--- a/src/cascadia/TerminalApp/AppLogic.cpp ++++ b/src/cascadia/TerminalApp/AppLogic.cpp +@@ -189,7 +210,7 @@ namespace winrt::TerminalApp::implementation + } + + AppLogic::AppLogic() : +- _reloadState{ std::chrono::milliseconds(100), []() { ApplicationState::SharedInstance().Reload(); } } ++ _reloadState{ std::chrono::milliseconds(100), []() { ApplicationState::SharedInstance().Reload(); ElevatedState::SharedInstance().Reload(); } } + { + // For your own sanity, it's better to do setup outside the ctor. + // If you do any setup in the ctor that ends up throwing an exception, +@@ -906,8 +927,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, +@@ -916,14 +935,17 @@ 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) { ++ [this, settingsPath](wil::FolderChangeEvent, PCWSTR fileModified) { ++ static const std::filesystem::path statePath{ std::wstring_view{ ApplicationState::SharedInstance().FilePath() } }; ++ static const std::filesystem::path elevatedStatePath{ std::wstring_view{ ElevatedState::SharedInstance().FilePath() } }; ++ + const auto modifiedBasename = std::filesystem::path{ fileModified }.filename(); + +- if (modifiedBasename == settingsBasename) ++ if (modifiedBasename == settingsPath.filename()) + { + _reloadSettings->Run(); + } +- else if (modifiedBasename == stateBasename) ++ else if (modifiedBasename == statePath.filename() || modifiedBasename == elevatedStatePath.filename()) + { + _reloadState(); + } +diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +index 5a00ba2b4..ec7f5cd7a 100644 +--- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp ++++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +@@ -60,38 +60,40 @@ using namespace ::Microsoft::Terminal::Settings::Model; + + namespace winrt::Microsoft::Terminal::Settings::Model::implementation + { ++ ApplicationState::ApplicationState(std::filesystem::path path) noexcept : ++ BaseApplicationState{ path } {} ++ + // Returns the application-global ApplicationState object. + Microsoft::Terminal::Settings::Model::ApplicationState ApplicationState::SharedInstance() + { + static auto state = winrt::make_self(GetBaseSettingsPath() / stateFileName); ++ state->Reload(); + return *state; + } + +- ApplicationState::ApplicationState(std::filesystem::path path) noexcept : +- _path{ std::move(path) }, +- _throttler{ std::chrono::seconds(1), [this]() { _write(); } } ++ void ApplicationState::FromJson(const Json::Value& root) const noexcept + { +- _read(); +- } +- +- // The destructor ensures that the last write is flushed to disk before returning. +- ApplicationState::~ApplicationState() +- { +- // 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(); ++ 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. ++#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) state->name = JsonUtils::GetValueForKey>(root, key); ++ MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) ++#undef MTSM_APPLICATION_STATE_GEN + } +- +- // Re-read the state.json from disk. +- void ApplicationState::Reload() const noexcept ++ Json::Value ApplicationState::ToJson() const noexcept + { +- _read(); +- } ++ Json::Value root{ Json::objectValue }; + +- // Returns the state.json path on the disk. +- winrt::hstring ApplicationState::FilePath() const noexcept +- { +- return winrt::hstring{ _path.wstring() }; ++ { ++ auto state = _state.lock_shared(); ++#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) 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 +@@ -115,58 +117,4 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation + MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) + #undef MTSM_APPLICATION_STATE_GEN + +- // Deserializes the state.json at _path 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 +- { +- const auto data = ReadUTF8FileIfExists(_path).value_or(std::string{}); +- if (data.empty()) +- { +- return; +- } +- +- std::string errs; +- std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; +- +- Json::Value root; +- if (!reader->parse(data.data(), data.data() + data.size(), &root, &errs)) +- { +- throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); +- } +- +- 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. +-#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) state->name = JsonUtils::GetValueForKey>(root, key); +- MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) +-#undef MTSM_APPLICATION_STATE_GEN +- } +- CATCH_LOG() +- +- // Serialized this ApplicationState (in `context`) into the state.json at _path. +- // * Errors are only logged. +- // * _state->_writeScheduled is set to false, signaling our +- // setters that _synchronize() needs to be called again. +- void ApplicationState::_write() const noexcept +- try +- { +- Json::Value root{ Json::objectValue }; +- +- { +- auto state = _state.lock_shared(); +-#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) JsonUtils::SetValueForKey(root, key, state->name); +- MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) +-#undef MTSM_APPLICATION_STATE_GEN +- } +- +- Json::StreamWriterBuilder wbuilder; +- const auto content = Json::writeString(wbuilder, root); +- WriteUTF8FileAtomic(_path, content); +- } +- CATCH_LOG() + } +diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h +index 71c6a576e..f7011289e 100644 +--- a/src/cascadia/TerminalSettingsModel/ApplicationState.h ++++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h +@@ -12,12 +12,11 @@ Abstract: + --*/ + #pragma once + ++#include "BaseApplicationState.h" + #include "ApplicationState.g.h" + #include "WindowLayout.g.h" + + #include +-#include +-#include + #include + + // This macro generates all getters and setters for ApplicationState. +@@ -40,18 +39,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation + friend ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait; + }; + +- struct ApplicationState : ApplicationStateT ++ struct ApplicationState : public BaseApplicationState, ApplicationStateT + { + static Microsoft::Terminal::Settings::Model::ApplicationState SharedInstance(); + + ApplicationState(std::filesystem::path path) noexcept; +- ~ApplicationState(); + +- // Methods +- void Reload() const noexcept; +- +- // General getters/setters +- winrt::hstring FilePath() const noexcept; ++ virtual void FromJson(const Json::Value& root) const noexcept override; ++ virtual Json::Value ToJson() const noexcept override; + + // State getters/setters + #define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) \ +@@ -67,13 +62,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation + MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) + #undef MTSM_APPLICATION_STATE_GEN + }; +- +- void _write() const noexcept; +- void _read() const noexcept; +- +- std::filesystem::path _path; + til::shared_mutex _state; +- til::throttled_func_trailing<> _throttler; + }; + } + +diff --git a/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp b/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp +new file mode 100644 +index 000000000..3b5c02446 +--- /dev/null ++++ b/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp +@@ -0,0 +1,92 @@ ++// Copyright (c) Microsoft Corporation. ++// Licensed under the MIT license. ++ ++#include "pch.h" ++#include "BaseApplicationState.h" ++#include "CascadiaSettings.h" ++ ++#include "JsonUtils.h" ++#include "FileUtils.h" ++ ++constexpr std::wstring_view stateFileName{ L"state.json" }; ++using namespace ::Microsoft::Terminal::Settings::Model; ++ ++BaseApplicationState::BaseApplicationState(std::filesystem::path path) noexcept : ++ _path{ std::move(path) }, ++ _throttler{ std::chrono::seconds(1), [this]() { _write(); } } ++{ ++ // DON'T _read() here! _read() will call FromJson, which is virtual, and ++ // needs to be implemented in a derived class. Classes that derive from ++ // BaseApplicationState should make sure to call Reload() after construction ++ // to ensure the data is loaded. ++} ++ ++// The destructor ensures that the last write is flushed to disk before returning. ++BaseApplicationState::~BaseApplicationState() ++{ ++ // 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(); ++} ++ ++// Re-read the state.json from disk. ++void BaseApplicationState::Reload() const noexcept ++{ ++ _read(); ++} ++ ++// Returns the state.json path on the disk. ++winrt::hstring BaseApplicationState::FilePath() const noexcept ++{ ++ return winrt::hstring{ _path.wstring() }; ++} ++ ++// Deserializes the state.json at _path 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 BaseApplicationState::_read() const noexcept ++try ++{ ++ const auto data = _readFileContents().value_or(std::string{}); ++ if (data.empty()) ++ { ++ return; ++ } ++ ++ std::string errs; ++ std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; ++ ++ Json::Value root; ++ if (!reader->parse(data.data(), data.data() + data.size(), &root, &errs)) ++ { ++ throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); ++ } ++ ++ FromJson(root); ++} ++CATCH_LOG() ++ ++// Serialized this ApplicationState (in `context`) into the state.json at _path. ++// * Errors are only logged. ++// * _state->_writeScheduled is set to false, signaling our ++// setters that _synchronize() needs to be called again. ++void BaseApplicationState::_write() const noexcept ++try ++{ ++ Json::Value root{ this->ToJson() }; ++ ++ Json::StreamWriterBuilder wbuilder; ++ const auto content = Json::writeString(wbuilder, root); ++ _writeFileContents(content); ++} ++CATCH_LOG() ++ ++std::optional BaseApplicationState::_readFileContents() const ++{ ++ return ReadUTF8FileIfExists(_path); ++} ++ ++void BaseApplicationState::_writeFileContents(const std::string_view content) const ++{ ++ WriteUTF8FileAtomic(_path, content); ++} +diff --git a/src/cascadia/TerminalSettingsModel/BaseApplicationState.h b/src/cascadia/TerminalSettingsModel/BaseApplicationState.h +new file mode 100644 +index 000000000..e684d3192 +--- /dev/null ++++ b/src/cascadia/TerminalSettingsModel/BaseApplicationState.h +@@ -0,0 +1,36 @@ ++/*++ ++Copyright (c) Microsoft Corporation ++Licensed under the MIT license. ++ ++Module Name: ++- ApplicationState.h ++ ++Abstract: ++- TODO! ++--*/ ++#pragma once ++ ++struct BaseApplicationState ++{ ++ BaseApplicationState(std::filesystem::path path) noexcept; ++ ~BaseApplicationState(); ++ ++ // Methods ++ void Reload() const noexcept; ++ ++ // General getters/setters ++ winrt::hstring FilePath() const noexcept; ++ ++ virtual void FromJson(const Json::Value& root) const noexcept = 0; ++ virtual Json::Value ToJson() const noexcept = 0; ++ ++protected: ++ virtual std::optional _readFileContents() const; ++ virtual void _writeFileContents(const std::string_view content) const; ++ ++ void _write() const noexcept; ++ void _read() const noexcept; ++ ++ std::filesystem::path _path; ++ til::throttled_func_trailing<> _throttler; ++}; +diff --git a/src/cascadia/TerminalSettingsModel/ElevatedState.cpp b/src/cascadia/TerminalSettingsModel/ElevatedState.cpp +new file mode 100644 +index 000000000..f7d72207e +--- /dev/null ++++ b/src/cascadia/TerminalSettingsModel/ElevatedState.cpp +@@ -0,0 +1,134 @@ ++// Copyright (c) Microsoft Corporation. ++// Licensed under the MIT license. ++ ++#include "pch.h" ++#include "ElevatedState.h" ++#include "CascadiaSettings.h" ++#include "ElevatedState.g.cpp" ++ ++#include "JsonUtils.h" ++#include "FileUtils.h" ++ ++#include ++ ++constexpr std::wstring_view stateFileName{ L"elevated-state.json" }; ++ ++using namespace ::Microsoft::Terminal::Settings::Model; ++ ++namespace winrt::Microsoft::Terminal::Settings::Model::implementation ++{ ++ ElevatedState::ElevatedState(std::filesystem::path path) noexcept : ++ BaseApplicationState{ path } {} ++ ++ // Returns the application-global ElevatedState object. ++ Microsoft::Terminal::Settings::Model::ElevatedState ElevatedState::SharedInstance() ++ { ++ // TODO! place in a totally different file! and path! ++ static auto state = winrt::make_self(GetBaseSettingsPath() / stateFileName); ++ state->Reload(); ++ ++ // const auto testPath{ GetBaseSettingsPath() / L"test.json" }; ++ ++ // PSID pEveryoneSID = NULL; ++ // SID_IDENTIFIER_AUTHORITY SIDAuthWorld = SECURITY_NT_AUTHORITY; ++ // BOOL success = AllocateAndInitializeSid(&SIDAuthWorld, 1, SECURITY_LOCAL_SYSTEM_RID, 0, 0, 0, 0, 0, 0, 0, &pEveryoneSID); ++ ++ // EXPLICIT_ACCESS ea[1]; ++ // ZeroMemory(&ea, 1 * sizeof(EXPLICIT_ACCESS)); ++ // ea[0].grfAccessPermissions = KEY_READ; ++ // ea[0].grfAccessMode = SET_ACCESS; ++ // ea[0].grfInheritance = NO_INHERITANCE; ++ // ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID; ++ // ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; ++ // ea[0].Trustee.ptstrName = (LPTSTR)pEveryoneSID; ++ ++ // ACL acl; ++ // PACL pAcl = &acl; ++ // DWORD dwRes = SetEntriesInAcl(1, ea, NULL, &pAcl); ++ // dwRes; ++ ++ // SECURITY_DESCRIPTOR sd; ++ // success = InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION); ++ // success = SetSecurityDescriptorDacl(&sd, ++ // TRUE, // bDaclPresent flag ++ // pAcl, ++ // FALSE); ++ ++ // SECURITY_ATTRIBUTES sa; ++ // // Initialize a security attributes structure. ++ // sa.nLength = sizeof(SECURITY_ATTRIBUTES); ++ // sa.lpSecurityDescriptor = &sd; ++ // sa.bInheritHandle = FALSE; ++ // success; ++ ++ // wil::unique_hfile file{ CreateFileW(testPath.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr) }; ++ // THROW_LAST_ERROR_IF(!file); ++ ++ return *state; ++ } ++ ++ void ElevatedState::FromJson(const Json::Value& root) 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. ++#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) state->name = JsonUtils::GetValueForKey>(root, key); ++ MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) ++#undef MTSM_ELEVATED_STATE_GEN ++ } ++ Json::Value ElevatedState::ToJson() const noexcept ++ { ++ Json::Value root{ Json::objectValue }; ++ ++ { ++ auto state = _state.lock_shared(); ++#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) JsonUtils::SetValueForKey(root, key, state->name); ++ MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) ++#undef MTSM_ELEVATED_STATE_GEN ++ } ++ return root; ++ } ++ ++ // Generate all getter/setters ++#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) \ ++ type ElevatedState::name() const noexcept \ ++ { \ ++ const auto state = _state.lock_shared(); \ ++ const auto& value = state->name; \ ++ return value ? *value : type{ __VA_ARGS__ }; \ ++ } \ ++ \ ++ void ElevatedState::name(const type& value) noexcept \ ++ { \ ++ { \ ++ auto state = _state.lock(); \ ++ state->name.emplace(value); \ ++ } \ ++ \ ++ _throttler(); \ ++ } ++ MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) ++#undef MTSM_ELEVATED_STATE_GEN ++ ++ void ElevatedState::_writeFileContents(const std::string_view content) const ++ { ++ // 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(_path, content, true); ++ } ++ ++ std::optional ElevatedState::_readFileContents() const ++ { ++ return ReadUTF8FileIfExists(_path, true); ++ } ++} +diff --git a/src/cascadia/TerminalSettingsModel/ElevatedState.h b/src/cascadia/TerminalSettingsModel/ElevatedState.h +new file mode 100644 +index 000000000..d0eb8381f +--- /dev/null ++++ b/src/cascadia/TerminalSettingsModel/ElevatedState.h +@@ -0,0 +1,60 @@ ++/*++ ++Copyright (c) Microsoft Corporation ++Licensed under the MIT license. ++ ++Module Name: ++- ElevatedState.h ++ ++Abstract: ++- If the CascadiaSettings class were AppData, then this class would be LocalAppData. ++ Put anything in here that you wouldn't want to be stored next to user-editable settings. ++- Modify ElevatedState.idl and MTSM_ELEVATED_STATE_FIELDS to add new fields. ++--*/ ++#pragma once ++ ++#include "BaseApplicationState.h" ++#include "ElevatedState.g.h" ++#include ++ ++// This macro generates all getters and setters for ElevatedState. ++// It provides X with the following arguments: ++// (type, function name, JSON key, ...variadic construction arguments) ++#define MTSM_ELEVATED_STATE_FIELDS(X) \ ++ X(Windows::Foundation::Collections::IVector, AllowedCommandlines, "allowedCommandlines") ++ ++namespace winrt::Microsoft::Terminal::Settings::Model::implementation ++{ ++ struct ElevatedState : ElevatedStateT, public BaseApplicationState ++ { ++ static Microsoft::Terminal::Settings::Model::ElevatedState SharedInstance(); ++ ++ ElevatedState(std::filesystem::path path) noexcept; ++ ++ void FromJson(const Json::Value& root) const noexcept override; ++ Json::Value ToJson() const noexcept override; ++ ++ // State getters/setters ++#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) \ ++ type name() const noexcept; \ ++ void name(const type& value) noexcept; ++ MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) ++#undef MTSM_ELEVATED_STATE_GEN ++ ++ private: ++ struct state_t ++ { ++#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) std::optional name{ __VA_ARGS__ }; ++ MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) ++#undef MTSM_ELEVATED_STATE_GEN ++ }; ++ til::shared_mutex _state; ++ ++ virtual std::optional _readFileContents() const override; ++ virtual void _writeFileContents(const std::string_view content) const override; ++ }; ++} ++ ++namespace winrt::Microsoft::Terminal::Settings::Model::factory_implementation ++{ ++ BASIC_FACTORY(ElevatedState); ++} +diff --git a/src/cascadia/TerminalSettingsModel/ElevatedState.idl b/src/cascadia/TerminalSettingsModel/ElevatedState.idl +new file mode 100644 +index 000000000..09e2d05b0 +--- /dev/null ++++ b/src/cascadia/TerminalSettingsModel/ElevatedState.idl +@@ -0,0 +1,15 @@ ++// Copyright (c) Microsoft Corporation. ++// Licensed under the MIT license. ++ ++namespace Microsoft.Terminal.Settings.Model ++{ ++ [default_interface] runtimeclass ElevatedState { ++ static ElevatedState SharedInstance(); ++ ++ void Reload(); ++ ++ String FilePath { get; }; ++ ++ Windows.Foundation.Collections.IVector AllowedCommandlines { get; set; }; ++ } ++} +diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp +index 81fbb6cee..dff35897a 100644 +--- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp ++++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp +@@ -8,6 +8,8 @@ + #include + #include + ++#include ++ + static constexpr std::string_view Utf8Bom{ u8"\uFEFF" }; + static constexpr std::wstring_view UnpackagedSettingsFolderName{ L"Microsoft\\Windows Terminal\\" }; + +@@ -39,10 +41,89 @@ namespace Microsoft::Terminal::Settings::Model + return baseSettingsPath; + } + ++ static bool _hasExpectedPermissions(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: ++ // * Everyone has permission to read ++ // * admins can do anything ++ // * no one else can do anything. ++ PACL pAcl{ nullptr }; // This doesn't need to be cleanup up apparently ++ ++ auto status = GetNamedSecurityInfo(path.c_str(), ++ SE_FILE_OBJECT, ++ DACL_SECURITY_INFORMATION, ++ nullptr, ++ nullptr, ++ &pAcl, ++ nullptr, ++ nullptr); ++ THROW_IF_WIN32_ERROR(status); ++ ++ PEXPLICIT_ACCESS pEA{ nullptr }; ++ DWORD count = 0; ++ status = GetExplicitEntriesFromAcl(pAcl, &count, &pEA); ++ THROW_IF_WIN32_ERROR(status); ++ ++ auto explicitAccessCleanup = wil::scope_exit([&]() { ::LocalFree(pEA); }); ++ ++ if (count != 2) ++ { ++ return false; ++ } ++ ++ // 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)); ++ ++ // 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)); ++ ++ bool hadExpectedPermissions = true; ++ ++ // Check that the permissions are what we'd expect them to be if only ++ // admins can write to the file. This is basically a mirror of what we ++ // set up in `WriteUTF8File`. ++ ++ // For grfAccessPermissions, GENERIC_ALL turns into STANDARD_RIGHTS_ALL, ++ // and GENERIC_READ -> READ_CONTROL ++ hadExpectedPermissions = hadExpectedPermissions && WI_AreAllFlagsSet(pEA[0].grfAccessPermissions, STANDARD_RIGHTS_ALL); ++ hadExpectedPermissions = hadExpectedPermissions && pEA[0].grfInheritance == NO_INHERITANCE; ++ hadExpectedPermissions = hadExpectedPermissions && pEA[0].Trustee.TrusteeForm == TRUSTEE_IS_SID; ++ // SIDs are void*'s that happen to convert to a wchar_t ++ hadExpectedPermissions = hadExpectedPermissions && *(pEA[0].Trustee.ptstrName) == *(LPWSTR)(adminGroupSid.get()); ++ ++ // Now check the other EXPLICIT_ACCESS ++ hadExpectedPermissions = hadExpectedPermissions && WI_IsFlagSet(pEA[1].grfAccessPermissions, READ_CONTROL); ++ hadExpectedPermissions = hadExpectedPermissions && pEA[1].grfInheritance == NO_INHERITANCE; ++ hadExpectedPermissions = hadExpectedPermissions && pEA[1].Trustee.TrusteeForm == TRUSTEE_IS_SID; ++ hadExpectedPermissions = hadExpectedPermissions && *(pEA[1].Trustee.ptstrName) == *(LPWSTR)(everyoneSid.get()); ++ ++ return hadExpectedPermissions; ++ } + // 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) + { ++ if (elevatedOnly) ++ { ++ const bool hadExpectedPermissions{ _hasExpectedPermissions(path) }; ++ if (!hadExpectedPermissions) ++ { ++ // 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 ""; ++ } ++ } ++ + // From some casual observations we can determine that: + // * ReadFile() always returns the requested amount of data (unless the file is smaller) + // * It's unlikely that the file was changed between GetFileSize() and ReadFile() +@@ -89,11 +170,11 @@ namespace 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) + { +@@ -106,9 +187,75 @@ namespace 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; ++ if (elevatedOnly) ++ { ++ // This is very vaguely taken from ++ // https://docs.microsoft.com/en-us/windows/win32/secauthz/creating-a-security-descriptor-for-a-new-object-in-c-- ++ // With using https://docs.microsoft.com/en-us/windows/win32/secauthz/well-known-sids ++ // to find out that ++ // * SECURITY_NT_AUTHORITY+SECURITY_LOCAL_SYSTEM_RID == NT AUTHORITY\SYSTEM ++ // * SECURITY_NT_AUTHORITY+SECURITY_BUILTIN_DOMAIN_RID+DOMAIN_ALIAS_RID_ADMINS == BUILTIN\Administrators ++ // * SECURITY_WORLD_SID_AUTHORITY+SECURITY_WORLD_RID == Everyone ++ // ++ // Raymond Chen recommended that I make this file only writable by ++ // 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)); ++ ++ // 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)); ++ ++ EXPLICIT_ACCESS ea[2]; ++ ZeroMemory(&ea, 2 * sizeof(EXPLICIT_ACCESS)); ++ // Grant Admins all permissions on this file ++ ea[0].grfAccessPermissions = GENERIC_ALL; ++ ea[0].grfAccessMode = SET_ACCESS; ++ ea[0].grfInheritance = NO_INHERITANCE; ++ ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID; ++ ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; ++ ea[0].Trustee.ptstrName = (LPWSTR)(adminGroupSid.get()); ++ ++ // Grant Everyone the permission or read this file ++ ea[1].grfAccessPermissions = GENERIC_READ; ++ ea[1].grfAccessMode = SET_ACCESS; ++ ea[1].grfInheritance = NO_INHERITANCE; ++ ea[1].Trustee.TrusteeForm = TRUSTEE_IS_SID; ++ ea[1].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; ++ ea[1].Trustee.ptstrName = (LPWSTR)(everyoneSid.get()); ++ ++ ACL acl; ++ PACL pAcl = &acl; ++ THROW_IF_WIN32_ERROR(SetEntriesInAcl(2, ea, nullptr, &pAcl)); ++ ++ SECURITY_DESCRIPTOR sd; ++ THROW_IF_WIN32_BOOL_FALSE(InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)); ++ THROW_IF_WIN32_BOOL_FALSE(SetSecurityDescriptorDacl(&sd, true, pAcl, false)); ++ ++ // Initialize a security attributes structure. ++ sa.nLength = sizeof(SECURITY_ATTRIBUTES); ++ sa.lpSecurityDescriptor = &sd; ++ sa.bInheritHandle = false; ++ } ++ ++ wil::unique_hfile file{ CreateFileW(path.c_str(), ++ GENERIC_WRITE, ++ FILE_SHARE_READ | FILE_SHARE_WRITE, ++ elevatedOnly ? &sa : nullptr, ++ CREATE_ALWAYS, ++ FILE_ATTRIBUTE_NORMAL, ++ nullptr) }; + THROW_LAST_ERROR_IF(!file); + + const auto fileSize = gsl::narrow(content.size()); +@@ -121,7 +268,8 @@ namespace 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 d2e2eb53c..0426b4dd5 100644 +--- a/src/cascadia/TerminalSettingsModel/FileUtils.h ++++ b/src/cascadia/TerminalSettingsModel/FileUtils.h +@@ -4,8 +4,8 @@ + namespace Microsoft::Terminal::Settings::Model + { + std::filesystem::path GetBaseSettingsPath(); +- 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 58124d84b..1b26f7bae 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/Microsoft.Terminal.Settings.ModelLib.vcxproj b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj +index 202f07808..ce12741d1 100644 +--- a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj ++++ b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj +@@ -21,6 +21,7 @@ + + IconPathConverter.idl + ++ + + + ActionArgs.idl +@@ -35,6 +36,9 @@ + + ApplicationState.idl + ++ ++ ElevatedState.idl ++ + + CascadiaSettings.idl + +@@ -88,6 +92,7 @@ + IconPathConverter.idl + + ++ + + Create + +@@ -107,6 +112,9 @@ + + ApplicationState.idl + ++ ++ ElevatedState.idl ++ + + CascadiaSettings.idl + +@@ -156,6 +164,7 @@ + + + ++ + + + +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/LocalTests_SettingsModel/pch.h b/src/cascadia/LocalTests_SettingsModel/pch.h index 53f6145d27e..01b4cdfe87f 100644 --- a/src/cascadia/LocalTests_SettingsModel/pch.h +++ b/src/cascadia/LocalTests_SettingsModel/pch.h @@ -61,6 +61,9 @@ Author(s): // Manually include til after we include Windows.Foundation to give it winrt superpowers #include "til.h" +#include +#include + // Common includes for most tests: #include "../../inc/argb.h" #include "../../inc/conattrs.hpp" diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index 6d53a6ba7ee..79e041704d7 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -189,7 +189,7 @@ namespace winrt::TerminalApp::implementation } AppLogic::AppLogic() : - _reloadState{ std::chrono::milliseconds(100), []() { ApplicationState::SharedInstance().Reload(); } } + _reloadState{ std::chrono::milliseconds(100), []() { ApplicationState::SharedInstance().Reload(); ElevatedState::SharedInstance().Reload(); } } { // For your own sanity, it's better to do setup outside the ctor. // If you do any setup in the ctor that ends up throwing an exception, @@ -906,8 +906,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, @@ -916,14 +914,17 @@ 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) { + [this, settingsPath](wil::FolderChangeEvent, PCWSTR fileModified) { + static const std::filesystem::path statePath{ std::wstring_view{ ApplicationState::SharedInstance().FilePath() } }; + static const std::filesystem::path elevatedStatePath{ std::wstring_view{ ElevatedState::SharedInstance().FilePath() } }; + const auto modifiedBasename = std::filesystem::path{ fileModified }.filename(); - if (modifiedBasename == settingsBasename) + if (modifiedBasename == settingsPath.filename()) { _reloadSettings->Run(); } - else if (modifiedBasename == stateBasename) + else if (modifiedBasename == statePath.filename() || modifiedBasename == elevatedStatePath.filename()) { _reloadState(); } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index 5a00ba2b410..ec7f5cd7acf 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -60,38 +60,40 @@ using namespace ::Microsoft::Terminal::Settings::Model; namespace winrt::Microsoft::Terminal::Settings::Model::implementation { + ApplicationState::ApplicationState(std::filesystem::path path) noexcept : + BaseApplicationState{ path } {} + // Returns the application-global ApplicationState object. Microsoft::Terminal::Settings::Model::ApplicationState ApplicationState::SharedInstance() { static auto state = winrt::make_self(GetBaseSettingsPath() / stateFileName); + state->Reload(); return *state; } - ApplicationState::ApplicationState(std::filesystem::path path) noexcept : - _path{ std::move(path) }, - _throttler{ std::chrono::seconds(1), [this]() { _write(); } } + void ApplicationState::FromJson(const Json::Value& root) const noexcept { - _read(); - } - - // The destructor ensures that the last write is flushed to disk before returning. - ApplicationState::~ApplicationState() - { - // 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(); + 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. +#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) state->name = JsonUtils::GetValueForKey>(root, key); + MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) +#undef MTSM_APPLICATION_STATE_GEN } - - // Re-read the state.json from disk. - void ApplicationState::Reload() const noexcept + Json::Value ApplicationState::ToJson() const noexcept { - _read(); - } + Json::Value root{ Json::objectValue }; - // Returns the state.json path on the disk. - winrt::hstring ApplicationState::FilePath() const noexcept - { - return winrt::hstring{ _path.wstring() }; + { + auto state = _state.lock_shared(); +#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) 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 @@ -115,58 +117,4 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) #undef MTSM_APPLICATION_STATE_GEN - // Deserializes the state.json at _path 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 - { - const auto data = ReadUTF8FileIfExists(_path).value_or(std::string{}); - if (data.empty()) - { - return; - } - - std::string errs; - std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; - - Json::Value root; - if (!reader->parse(data.data(), data.data() + data.size(), &root, &errs)) - { - throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); - } - - 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. -#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) state->name = JsonUtils::GetValueForKey>(root, key); - MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) -#undef MTSM_APPLICATION_STATE_GEN - } - CATCH_LOG() - - // Serialized this ApplicationState (in `context`) into the state.json at _path. - // * Errors are only logged. - // * _state->_writeScheduled is set to false, signaling our - // setters that _synchronize() needs to be called again. - void ApplicationState::_write() const noexcept - try - { - Json::Value root{ Json::objectValue }; - - { - auto state = _state.lock_shared(); -#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) JsonUtils::SetValueForKey(root, key, state->name); - MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) -#undef MTSM_APPLICATION_STATE_GEN - } - - Json::StreamWriterBuilder wbuilder; - const auto content = Json::writeString(wbuilder, root); - WriteUTF8FileAtomic(_path, content); - } - CATCH_LOG() } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index 71c6a576eeb..f7011289e0f 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -12,12 +12,11 @@ Module Name: --*/ #pragma once +#include "BaseApplicationState.h" #include "ApplicationState.g.h" #include "WindowLayout.g.h" #include -#include -#include #include // This macro generates all getters and setters for ApplicationState. @@ -40,18 +39,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation friend ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait; }; - struct ApplicationState : ApplicationStateT + struct ApplicationState : public BaseApplicationState, ApplicationStateT { static Microsoft::Terminal::Settings::Model::ApplicationState SharedInstance(); ApplicationState(std::filesystem::path path) noexcept; - ~ApplicationState(); - // Methods - void Reload() const noexcept; - - // General getters/setters - winrt::hstring FilePath() const noexcept; + virtual void FromJson(const Json::Value& root) const noexcept override; + virtual Json::Value ToJson() const noexcept override; // State getters/setters #define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) \ @@ -67,13 +62,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) #undef MTSM_APPLICATION_STATE_GEN }; - - void _write() const noexcept; - void _read() const noexcept; - - std::filesystem::path _path; til::shared_mutex _state; - til::throttled_func_trailing<> _throttler; }; } diff --git a/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp b/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp new file mode 100644 index 00000000000..3b5c0244628 --- /dev/null +++ b/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp @@ -0,0 +1,92 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "pch.h" +#include "BaseApplicationState.h" +#include "CascadiaSettings.h" + +#include "JsonUtils.h" +#include "FileUtils.h" + +constexpr std::wstring_view stateFileName{ L"state.json" }; +using namespace ::Microsoft::Terminal::Settings::Model; + +BaseApplicationState::BaseApplicationState(std::filesystem::path path) noexcept : + _path{ std::move(path) }, + _throttler{ std::chrono::seconds(1), [this]() { _write(); } } +{ + // DON'T _read() here! _read() will call FromJson, which is virtual, and + // needs to be implemented in a derived class. Classes that derive from + // BaseApplicationState should make sure to call Reload() after construction + // to ensure the data is loaded. +} + +// The destructor ensures that the last write is flushed to disk before returning. +BaseApplicationState::~BaseApplicationState() +{ + // 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(); +} + +// Re-read the state.json from disk. +void BaseApplicationState::Reload() const noexcept +{ + _read(); +} + +// Returns the state.json path on the disk. +winrt::hstring BaseApplicationState::FilePath() const noexcept +{ + return winrt::hstring{ _path.wstring() }; +} + +// Deserializes the state.json at _path 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 BaseApplicationState::_read() const noexcept +try +{ + const auto data = _readFileContents().value_or(std::string{}); + if (data.empty()) + { + return; + } + + std::string errs; + std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; + + Json::Value root; + if (!reader->parse(data.data(), data.data() + data.size(), &root, &errs)) + { + throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); + } + + FromJson(root); +} +CATCH_LOG() + +// Serialized this ApplicationState (in `context`) into the state.json at _path. +// * Errors are only logged. +// * _state->_writeScheduled is set to false, signaling our +// setters that _synchronize() needs to be called again. +void BaseApplicationState::_write() const noexcept +try +{ + Json::Value root{ this->ToJson() }; + + Json::StreamWriterBuilder wbuilder; + const auto content = Json::writeString(wbuilder, root); + _writeFileContents(content); +} +CATCH_LOG() + +std::optional BaseApplicationState::_readFileContents() const +{ + return ReadUTF8FileIfExists(_path); +} + +void BaseApplicationState::_writeFileContents(const std::string_view content) const +{ + WriteUTF8FileAtomic(_path, content); +} diff --git a/src/cascadia/TerminalSettingsModel/BaseApplicationState.h b/src/cascadia/TerminalSettingsModel/BaseApplicationState.h new file mode 100644 index 00000000000..e684d319268 --- /dev/null +++ b/src/cascadia/TerminalSettingsModel/BaseApplicationState.h @@ -0,0 +1,36 @@ +/*++ +Copyright (c) Microsoft Corporation +Licensed under the MIT license. + +Module Name: +- ApplicationState.h + +Abstract: +- TODO! +--*/ +#pragma once + +struct BaseApplicationState +{ + BaseApplicationState(std::filesystem::path path) noexcept; + ~BaseApplicationState(); + + // Methods + void Reload() const noexcept; + + // General getters/setters + winrt::hstring FilePath() const noexcept; + + virtual void FromJson(const Json::Value& root) const noexcept = 0; + virtual Json::Value ToJson() const noexcept = 0; + +protected: + virtual std::optional _readFileContents() const; + virtual void _writeFileContents(const std::string_view content) const; + + void _write() const noexcept; + void _read() const noexcept; + + std::filesystem::path _path; + til::throttled_func_trailing<> _throttler; +}; diff --git a/src/cascadia/TerminalSettingsModel/ElevatedState.cpp b/src/cascadia/TerminalSettingsModel/ElevatedState.cpp new file mode 100644 index 00000000000..f7d72207ed2 --- /dev/null +++ b/src/cascadia/TerminalSettingsModel/ElevatedState.cpp @@ -0,0 +1,134 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "pch.h" +#include "ElevatedState.h" +#include "CascadiaSettings.h" +#include "ElevatedState.g.cpp" + +#include "JsonUtils.h" +#include "FileUtils.h" + +#include + +constexpr std::wstring_view stateFileName{ L"elevated-state.json" }; + +using namespace ::Microsoft::Terminal::Settings::Model; + +namespace winrt::Microsoft::Terminal::Settings::Model::implementation +{ + ElevatedState::ElevatedState(std::filesystem::path path) noexcept : + BaseApplicationState{ path } {} + + // Returns the application-global ElevatedState object. + Microsoft::Terminal::Settings::Model::ElevatedState ElevatedState::SharedInstance() + { + // TODO! place in a totally different file! and path! + static auto state = winrt::make_self(GetBaseSettingsPath() / stateFileName); + state->Reload(); + + // const auto testPath{ GetBaseSettingsPath() / L"test.json" }; + + // PSID pEveryoneSID = NULL; + // SID_IDENTIFIER_AUTHORITY SIDAuthWorld = SECURITY_NT_AUTHORITY; + // BOOL success = AllocateAndInitializeSid(&SIDAuthWorld, 1, SECURITY_LOCAL_SYSTEM_RID, 0, 0, 0, 0, 0, 0, 0, &pEveryoneSID); + + // EXPLICIT_ACCESS ea[1]; + // ZeroMemory(&ea, 1 * sizeof(EXPLICIT_ACCESS)); + // ea[0].grfAccessPermissions = KEY_READ; + // ea[0].grfAccessMode = SET_ACCESS; + // ea[0].grfInheritance = NO_INHERITANCE; + // ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID; + // ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; + // ea[0].Trustee.ptstrName = (LPTSTR)pEveryoneSID; + + // ACL acl; + // PACL pAcl = &acl; + // DWORD dwRes = SetEntriesInAcl(1, ea, NULL, &pAcl); + // dwRes; + + // SECURITY_DESCRIPTOR sd; + // success = InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION); + // success = SetSecurityDescriptorDacl(&sd, + // TRUE, // bDaclPresent flag + // pAcl, + // FALSE); + + // SECURITY_ATTRIBUTES sa; + // // Initialize a security attributes structure. + // sa.nLength = sizeof(SECURITY_ATTRIBUTES); + // sa.lpSecurityDescriptor = &sd; + // sa.bInheritHandle = FALSE; + // success; + + // wil::unique_hfile file{ CreateFileW(testPath.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr) }; + // THROW_LAST_ERROR_IF(!file); + + return *state; + } + + void ElevatedState::FromJson(const Json::Value& root) 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. +#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) state->name = JsonUtils::GetValueForKey>(root, key); + MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) +#undef MTSM_ELEVATED_STATE_GEN + } + Json::Value ElevatedState::ToJson() const noexcept + { + Json::Value root{ Json::objectValue }; + + { + auto state = _state.lock_shared(); +#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) JsonUtils::SetValueForKey(root, key, state->name); + MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) +#undef MTSM_ELEVATED_STATE_GEN + } + return root; + } + + // Generate all getter/setters +#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) \ + type ElevatedState::name() const noexcept \ + { \ + const auto state = _state.lock_shared(); \ + const auto& value = state->name; \ + return value ? *value : type{ __VA_ARGS__ }; \ + } \ + \ + void ElevatedState::name(const type& value) noexcept \ + { \ + { \ + auto state = _state.lock(); \ + state->name.emplace(value); \ + } \ + \ + _throttler(); \ + } + MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) +#undef MTSM_ELEVATED_STATE_GEN + + void ElevatedState::_writeFileContents(const std::string_view content) const + { + // 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(_path, content, true); + } + + std::optional ElevatedState::_readFileContents() const + { + return ReadUTF8FileIfExists(_path, true); + } +} diff --git a/src/cascadia/TerminalSettingsModel/ElevatedState.h b/src/cascadia/TerminalSettingsModel/ElevatedState.h new file mode 100644 index 00000000000..d0eb8381fbe --- /dev/null +++ b/src/cascadia/TerminalSettingsModel/ElevatedState.h @@ -0,0 +1,60 @@ +/*++ +Copyright (c) Microsoft Corporation +Licensed under the MIT license. + +Module Name: +- ElevatedState.h + +Abstract: +- If the CascadiaSettings class were AppData, then this class would be LocalAppData. + Put anything in here that you wouldn't want to be stored next to user-editable settings. +- Modify ElevatedState.idl and MTSM_ELEVATED_STATE_FIELDS to add new fields. +--*/ +#pragma once + +#include "BaseApplicationState.h" +#include "ElevatedState.g.h" +#include + +// This macro generates all getters and setters for ElevatedState. +// It provides X with the following arguments: +// (type, function name, JSON key, ...variadic construction arguments) +#define MTSM_ELEVATED_STATE_FIELDS(X) \ + X(Windows::Foundation::Collections::IVector, AllowedCommandlines, "allowedCommandlines") + +namespace winrt::Microsoft::Terminal::Settings::Model::implementation +{ + struct ElevatedState : ElevatedStateT, public BaseApplicationState + { + static Microsoft::Terminal::Settings::Model::ElevatedState SharedInstance(); + + ElevatedState(std::filesystem::path path) noexcept; + + void FromJson(const Json::Value& root) const noexcept override; + Json::Value ToJson() const noexcept override; + + // State getters/setters +#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) \ + type name() const noexcept; \ + void name(const type& value) noexcept; + MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) +#undef MTSM_ELEVATED_STATE_GEN + + private: + struct state_t + { +#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) std::optional name{ __VA_ARGS__ }; + MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) +#undef MTSM_ELEVATED_STATE_GEN + }; + til::shared_mutex _state; + + virtual std::optional _readFileContents() const override; + virtual void _writeFileContents(const std::string_view content) const override; + }; +} + +namespace winrt::Microsoft::Terminal::Settings::Model::factory_implementation +{ + BASIC_FACTORY(ElevatedState); +} diff --git a/src/cascadia/TerminalSettingsModel/ElevatedState.idl b/src/cascadia/TerminalSettingsModel/ElevatedState.idl new file mode 100644 index 00000000000..09e2d05b025 --- /dev/null +++ b/src/cascadia/TerminalSettingsModel/ElevatedState.idl @@ -0,0 +1,15 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +namespace Microsoft.Terminal.Settings.Model +{ + [default_interface] runtimeclass ElevatedState { + static ElevatedState SharedInstance(); + + void Reload(); + + String FilePath { get; }; + + Windows.Foundation.Collections.IVector AllowedCommandlines { get; set; }; + } +} diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index 81fbb6cee67..dff35897a3e 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -8,6 +8,8 @@ #include #include +#include + static constexpr std::string_view Utf8Bom{ u8"\uFEFF" }; static constexpr std::wstring_view UnpackagedSettingsFolderName{ L"Microsoft\\Windows Terminal\\" }; @@ -39,10 +41,89 @@ namespace Microsoft::Terminal::Settings::Model return baseSettingsPath; } + static bool _hasExpectedPermissions(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: + // * Everyone has permission to read + // * admins can do anything + // * no one else can do anything. + PACL pAcl{ nullptr }; // This doesn't need to be cleanup up apparently + + auto status = GetNamedSecurityInfo(path.c_str(), + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION, + nullptr, + nullptr, + &pAcl, + nullptr, + nullptr); + THROW_IF_WIN32_ERROR(status); + + PEXPLICIT_ACCESS pEA{ nullptr }; + DWORD count = 0; + status = GetExplicitEntriesFromAcl(pAcl, &count, &pEA); + THROW_IF_WIN32_ERROR(status); + + auto explicitAccessCleanup = wil::scope_exit([&]() { ::LocalFree(pEA); }); + + if (count != 2) + { + return false; + } + + // 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)); + + // 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)); + + bool hadExpectedPermissions = true; + + // Check that the permissions are what we'd expect them to be if only + // admins can write to the file. This is basically a mirror of what we + // set up in `WriteUTF8File`. + + // For grfAccessPermissions, GENERIC_ALL turns into STANDARD_RIGHTS_ALL, + // and GENERIC_READ -> READ_CONTROL + hadExpectedPermissions = hadExpectedPermissions && WI_AreAllFlagsSet(pEA[0].grfAccessPermissions, STANDARD_RIGHTS_ALL); + hadExpectedPermissions = hadExpectedPermissions && pEA[0].grfInheritance == NO_INHERITANCE; + hadExpectedPermissions = hadExpectedPermissions && pEA[0].Trustee.TrusteeForm == TRUSTEE_IS_SID; + // SIDs are void*'s that happen to convert to a wchar_t + hadExpectedPermissions = hadExpectedPermissions && *(pEA[0].Trustee.ptstrName) == *(LPWSTR)(adminGroupSid.get()); + + // Now check the other EXPLICIT_ACCESS + hadExpectedPermissions = hadExpectedPermissions && WI_IsFlagSet(pEA[1].grfAccessPermissions, READ_CONTROL); + hadExpectedPermissions = hadExpectedPermissions && pEA[1].grfInheritance == NO_INHERITANCE; + hadExpectedPermissions = hadExpectedPermissions && pEA[1].Trustee.TrusteeForm == TRUSTEE_IS_SID; + hadExpectedPermissions = hadExpectedPermissions && *(pEA[1].Trustee.ptstrName) == *(LPWSTR)(everyoneSid.get()); + + return hadExpectedPermissions; + } // 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) { + if (elevatedOnly) + { + const bool hadExpectedPermissions{ _hasExpectedPermissions(path) }; + if (!hadExpectedPermissions) + { + // 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 ""; + } + } + // From some casual observations we can determine that: // * ReadFile() always returns the requested amount of data (unless the file is smaller) // * It's unlikely that the file was changed between GetFileSize() and ReadFile() @@ -89,11 +170,11 @@ namespace 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) { @@ -106,9 +187,75 @@ namespace 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; + if (elevatedOnly) + { + // This is very vaguely taken from + // https://docs.microsoft.com/en-us/windows/win32/secauthz/creating-a-security-descriptor-for-a-new-object-in-c-- + // With using https://docs.microsoft.com/en-us/windows/win32/secauthz/well-known-sids + // to find out that + // * SECURITY_NT_AUTHORITY+SECURITY_LOCAL_SYSTEM_RID == NT AUTHORITY\SYSTEM + // * SECURITY_NT_AUTHORITY+SECURITY_BUILTIN_DOMAIN_RID+DOMAIN_ALIAS_RID_ADMINS == BUILTIN\Administrators + // * SECURITY_WORLD_SID_AUTHORITY+SECURITY_WORLD_RID == Everyone + // + // Raymond Chen recommended that I make this file only writable by + // 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)); + + // 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)); + + EXPLICIT_ACCESS ea[2]; + ZeroMemory(&ea, 2 * sizeof(EXPLICIT_ACCESS)); + // Grant Admins all permissions on this file + ea[0].grfAccessPermissions = GENERIC_ALL; + ea[0].grfAccessMode = SET_ACCESS; + ea[0].grfInheritance = NO_INHERITANCE; + ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID; + ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; + ea[0].Trustee.ptstrName = (LPWSTR)(adminGroupSid.get()); + + // Grant Everyone the permission or read this file + ea[1].grfAccessPermissions = GENERIC_READ; + ea[1].grfAccessMode = SET_ACCESS; + ea[1].grfInheritance = NO_INHERITANCE; + ea[1].Trustee.TrusteeForm = TRUSTEE_IS_SID; + ea[1].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; + ea[1].Trustee.ptstrName = (LPWSTR)(everyoneSid.get()); + + ACL acl; + PACL pAcl = &acl; + THROW_IF_WIN32_ERROR(SetEntriesInAcl(2, ea, nullptr, &pAcl)); + + SECURITY_DESCRIPTOR sd; + THROW_IF_WIN32_BOOL_FALSE(InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)); + THROW_IF_WIN32_BOOL_FALSE(SetSecurityDescriptorDacl(&sd, true, pAcl, false)); + + // Initialize a security attributes structure. + sa.nLength = sizeof(SECURITY_ATTRIBUTES); + sa.lpSecurityDescriptor = &sd; + sa.bInheritHandle = false; + } + + wil::unique_hfile file{ CreateFileW(path.c_str(), + GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, + elevatedOnly ? &sa : nullptr, + CREATE_ALWAYS, + FILE_ATTRIBUTE_NORMAL, + nullptr) }; THROW_LAST_ERROR_IF(!file); const auto fileSize = gsl::narrow(content.size()); @@ -121,7 +268,8 @@ namespace 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 d2e2eb53c7d..0426b4dd5ab 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.h +++ b/src/cascadia/TerminalSettingsModel/FileUtils.h @@ -4,8 +4,8 @@ namespace Microsoft::Terminal::Settings::Model { std::filesystem::path GetBaseSettingsPath(); - 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 58124d84bcb..1b26f7bae30 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/Microsoft.Terminal.Settings.ModelLib.vcxproj b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj index 202f078084e..ce12741d136 100644 --- a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj +++ b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj @@ -21,6 +21,7 @@ IconPathConverter.idl + ActionArgs.idl @@ -35,6 +36,9 @@ ApplicationState.idl + + ElevatedState.idl + CascadiaSettings.idl @@ -88,6 +92,7 @@ IconPathConverter.idl + Create @@ -107,6 +112,9 @@ ApplicationState.idl + + ElevatedState.idl + CascadiaSettings.idl @@ -156,6 +164,7 @@ + diff --git a/src/cascadia/TerminalSettingsModel/pch.h b/src/cascadia/TerminalSettingsModel/pch.h index d5473939c6d..25773a45d91 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 From c106f64bc7f07b46a315593d7b86a9efbde92120 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 14 Sep 2021 05:45:45 -0500 Subject: [PATCH 02/30] :facepalm: (cherry picked from commit 00c76475940db8ceee3c4e4b93964a2bfaa27432) --- patch.diff | 896 ----------------------------------------------------- 1 file changed, 896 deletions(-) delete mode 100644 patch.diff diff --git a/patch.diff b/patch.diff deleted file mode 100644 index 6f59399e38c..00000000000 --- a/patch.diff +++ /dev/null @@ -1,896 +0,0 @@ -diff --git a/src/cascadia/LocalTests_SettingsModel/pch.h b/src/cascadia/LocalTests_SettingsModel/pch.h -index 53f6145d2..01b4cdfe8 100644 ---- a/src/cascadia/LocalTests_SettingsModel/pch.h -+++ b/src/cascadia/LocalTests_SettingsModel/pch.h -@@ -61,6 +61,9 @@ Author(s): - // Manually include til after we include Windows.Foundation to give it winrt superpowers - #include "til.h" - -+#include -+#include -+ - // Common includes for most tests: - #include "../../inc/argb.h" - #include "../../inc/conattrs.hpp" -diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp -index 6d53a6ba7..38edbc2c9 100644 ---- a/src/cascadia/TerminalApp/AppLogic.cpp -+++ b/src/cascadia/TerminalApp/AppLogic.cpp -@@ -189,7 +210,7 @@ namespace winrt::TerminalApp::implementation - } - - AppLogic::AppLogic() : -- _reloadState{ std::chrono::milliseconds(100), []() { ApplicationState::SharedInstance().Reload(); } } -+ _reloadState{ std::chrono::milliseconds(100), []() { ApplicationState::SharedInstance().Reload(); ElevatedState::SharedInstance().Reload(); } } - { - // For your own sanity, it's better to do setup outside the ctor. - // If you do any setup in the ctor that ends up throwing an exception, -@@ -906,8 +927,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, -@@ -916,14 +935,17 @@ 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) { -+ [this, settingsPath](wil::FolderChangeEvent, PCWSTR fileModified) { -+ static const std::filesystem::path statePath{ std::wstring_view{ ApplicationState::SharedInstance().FilePath() } }; -+ static const std::filesystem::path elevatedStatePath{ std::wstring_view{ ElevatedState::SharedInstance().FilePath() } }; -+ - const auto modifiedBasename = std::filesystem::path{ fileModified }.filename(); - -- if (modifiedBasename == settingsBasename) -+ if (modifiedBasename == settingsPath.filename()) - { - _reloadSettings->Run(); - } -- else if (modifiedBasename == stateBasename) -+ else if (modifiedBasename == statePath.filename() || modifiedBasename == elevatedStatePath.filename()) - { - _reloadState(); - } -diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp -index 5a00ba2b4..ec7f5cd7a 100644 ---- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp -+++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp -@@ -60,38 +60,40 @@ using namespace ::Microsoft::Terminal::Settings::Model; - - namespace winrt::Microsoft::Terminal::Settings::Model::implementation - { -+ ApplicationState::ApplicationState(std::filesystem::path path) noexcept : -+ BaseApplicationState{ path } {} -+ - // Returns the application-global ApplicationState object. - Microsoft::Terminal::Settings::Model::ApplicationState ApplicationState::SharedInstance() - { - static auto state = winrt::make_self(GetBaseSettingsPath() / stateFileName); -+ state->Reload(); - return *state; - } - -- ApplicationState::ApplicationState(std::filesystem::path path) noexcept : -- _path{ std::move(path) }, -- _throttler{ std::chrono::seconds(1), [this]() { _write(); } } -+ void ApplicationState::FromJson(const Json::Value& root) const noexcept - { -- _read(); -- } -- -- // The destructor ensures that the last write is flushed to disk before returning. -- ApplicationState::~ApplicationState() -- { -- // 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(); -+ 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. -+#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) state->name = JsonUtils::GetValueForKey>(root, key); -+ MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) -+#undef MTSM_APPLICATION_STATE_GEN - } -- -- // Re-read the state.json from disk. -- void ApplicationState::Reload() const noexcept -+ Json::Value ApplicationState::ToJson() const noexcept - { -- _read(); -- } -+ Json::Value root{ Json::objectValue }; - -- // Returns the state.json path on the disk. -- winrt::hstring ApplicationState::FilePath() const noexcept -- { -- return winrt::hstring{ _path.wstring() }; -+ { -+ auto state = _state.lock_shared(); -+#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) 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 -@@ -115,58 +117,4 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation - MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) - #undef MTSM_APPLICATION_STATE_GEN - -- // Deserializes the state.json at _path 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 -- { -- const auto data = ReadUTF8FileIfExists(_path).value_or(std::string{}); -- if (data.empty()) -- { -- return; -- } -- -- std::string errs; -- std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; -- -- Json::Value root; -- if (!reader->parse(data.data(), data.data() + data.size(), &root, &errs)) -- { -- throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); -- } -- -- 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. --#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) state->name = JsonUtils::GetValueForKey>(root, key); -- MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) --#undef MTSM_APPLICATION_STATE_GEN -- } -- CATCH_LOG() -- -- // Serialized this ApplicationState (in `context`) into the state.json at _path. -- // * Errors are only logged. -- // * _state->_writeScheduled is set to false, signaling our -- // setters that _synchronize() needs to be called again. -- void ApplicationState::_write() const noexcept -- try -- { -- Json::Value root{ Json::objectValue }; -- -- { -- auto state = _state.lock_shared(); --#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) JsonUtils::SetValueForKey(root, key, state->name); -- MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) --#undef MTSM_APPLICATION_STATE_GEN -- } -- -- Json::StreamWriterBuilder wbuilder; -- const auto content = Json::writeString(wbuilder, root); -- WriteUTF8FileAtomic(_path, content); -- } -- CATCH_LOG() - } -diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h -index 71c6a576e..f7011289e 100644 ---- a/src/cascadia/TerminalSettingsModel/ApplicationState.h -+++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h -@@ -12,12 +12,11 @@ Abstract: - --*/ - #pragma once - -+#include "BaseApplicationState.h" - #include "ApplicationState.g.h" - #include "WindowLayout.g.h" - - #include --#include --#include - #include - - // This macro generates all getters and setters for ApplicationState. -@@ -40,18 +39,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation - friend ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait; - }; - -- struct ApplicationState : ApplicationStateT -+ struct ApplicationState : public BaseApplicationState, ApplicationStateT - { - static Microsoft::Terminal::Settings::Model::ApplicationState SharedInstance(); - - ApplicationState(std::filesystem::path path) noexcept; -- ~ApplicationState(); - -- // Methods -- void Reload() const noexcept; -- -- // General getters/setters -- winrt::hstring FilePath() const noexcept; -+ virtual void FromJson(const Json::Value& root) const noexcept override; -+ virtual Json::Value ToJson() const noexcept override; - - // State getters/setters - #define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) \ -@@ -67,13 +62,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation - MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) - #undef MTSM_APPLICATION_STATE_GEN - }; -- -- void _write() const noexcept; -- void _read() const noexcept; -- -- std::filesystem::path _path; - til::shared_mutex _state; -- til::throttled_func_trailing<> _throttler; - }; - } - -diff --git a/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp b/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp -new file mode 100644 -index 000000000..3b5c02446 ---- /dev/null -+++ b/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp -@@ -0,0 +1,92 @@ -+// Copyright (c) Microsoft Corporation. -+// Licensed under the MIT license. -+ -+#include "pch.h" -+#include "BaseApplicationState.h" -+#include "CascadiaSettings.h" -+ -+#include "JsonUtils.h" -+#include "FileUtils.h" -+ -+constexpr std::wstring_view stateFileName{ L"state.json" }; -+using namespace ::Microsoft::Terminal::Settings::Model; -+ -+BaseApplicationState::BaseApplicationState(std::filesystem::path path) noexcept : -+ _path{ std::move(path) }, -+ _throttler{ std::chrono::seconds(1), [this]() { _write(); } } -+{ -+ // DON'T _read() here! _read() will call FromJson, which is virtual, and -+ // needs to be implemented in a derived class. Classes that derive from -+ // BaseApplicationState should make sure to call Reload() after construction -+ // to ensure the data is loaded. -+} -+ -+// The destructor ensures that the last write is flushed to disk before returning. -+BaseApplicationState::~BaseApplicationState() -+{ -+ // 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(); -+} -+ -+// Re-read the state.json from disk. -+void BaseApplicationState::Reload() const noexcept -+{ -+ _read(); -+} -+ -+// Returns the state.json path on the disk. -+winrt::hstring BaseApplicationState::FilePath() const noexcept -+{ -+ return winrt::hstring{ _path.wstring() }; -+} -+ -+// Deserializes the state.json at _path 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 BaseApplicationState::_read() const noexcept -+try -+{ -+ const auto data = _readFileContents().value_or(std::string{}); -+ if (data.empty()) -+ { -+ return; -+ } -+ -+ std::string errs; -+ std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; -+ -+ Json::Value root; -+ if (!reader->parse(data.data(), data.data() + data.size(), &root, &errs)) -+ { -+ throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); -+ } -+ -+ FromJson(root); -+} -+CATCH_LOG() -+ -+// Serialized this ApplicationState (in `context`) into the state.json at _path. -+// * Errors are only logged. -+// * _state->_writeScheduled is set to false, signaling our -+// setters that _synchronize() needs to be called again. -+void BaseApplicationState::_write() const noexcept -+try -+{ -+ Json::Value root{ this->ToJson() }; -+ -+ Json::StreamWriterBuilder wbuilder; -+ const auto content = Json::writeString(wbuilder, root); -+ _writeFileContents(content); -+} -+CATCH_LOG() -+ -+std::optional BaseApplicationState::_readFileContents() const -+{ -+ return ReadUTF8FileIfExists(_path); -+} -+ -+void BaseApplicationState::_writeFileContents(const std::string_view content) const -+{ -+ WriteUTF8FileAtomic(_path, content); -+} -diff --git a/src/cascadia/TerminalSettingsModel/BaseApplicationState.h b/src/cascadia/TerminalSettingsModel/BaseApplicationState.h -new file mode 100644 -index 000000000..e684d3192 ---- /dev/null -+++ b/src/cascadia/TerminalSettingsModel/BaseApplicationState.h -@@ -0,0 +1,36 @@ -+/*++ -+Copyright (c) Microsoft Corporation -+Licensed under the MIT license. -+ -+Module Name: -+- ApplicationState.h -+ -+Abstract: -+- TODO! -+--*/ -+#pragma once -+ -+struct BaseApplicationState -+{ -+ BaseApplicationState(std::filesystem::path path) noexcept; -+ ~BaseApplicationState(); -+ -+ // Methods -+ void Reload() const noexcept; -+ -+ // General getters/setters -+ winrt::hstring FilePath() const noexcept; -+ -+ virtual void FromJson(const Json::Value& root) const noexcept = 0; -+ virtual Json::Value ToJson() const noexcept = 0; -+ -+protected: -+ virtual std::optional _readFileContents() const; -+ virtual void _writeFileContents(const std::string_view content) const; -+ -+ void _write() const noexcept; -+ void _read() const noexcept; -+ -+ std::filesystem::path _path; -+ til::throttled_func_trailing<> _throttler; -+}; -diff --git a/src/cascadia/TerminalSettingsModel/ElevatedState.cpp b/src/cascadia/TerminalSettingsModel/ElevatedState.cpp -new file mode 100644 -index 000000000..f7d72207e ---- /dev/null -+++ b/src/cascadia/TerminalSettingsModel/ElevatedState.cpp -@@ -0,0 +1,134 @@ -+// Copyright (c) Microsoft Corporation. -+// Licensed under the MIT license. -+ -+#include "pch.h" -+#include "ElevatedState.h" -+#include "CascadiaSettings.h" -+#include "ElevatedState.g.cpp" -+ -+#include "JsonUtils.h" -+#include "FileUtils.h" -+ -+#include -+ -+constexpr std::wstring_view stateFileName{ L"elevated-state.json" }; -+ -+using namespace ::Microsoft::Terminal::Settings::Model; -+ -+namespace winrt::Microsoft::Terminal::Settings::Model::implementation -+{ -+ ElevatedState::ElevatedState(std::filesystem::path path) noexcept : -+ BaseApplicationState{ path } {} -+ -+ // Returns the application-global ElevatedState object. -+ Microsoft::Terminal::Settings::Model::ElevatedState ElevatedState::SharedInstance() -+ { -+ // TODO! place in a totally different file! and path! -+ static auto state = winrt::make_self(GetBaseSettingsPath() / stateFileName); -+ state->Reload(); -+ -+ // const auto testPath{ GetBaseSettingsPath() / L"test.json" }; -+ -+ // PSID pEveryoneSID = NULL; -+ // SID_IDENTIFIER_AUTHORITY SIDAuthWorld = SECURITY_NT_AUTHORITY; -+ // BOOL success = AllocateAndInitializeSid(&SIDAuthWorld, 1, SECURITY_LOCAL_SYSTEM_RID, 0, 0, 0, 0, 0, 0, 0, &pEveryoneSID); -+ -+ // EXPLICIT_ACCESS ea[1]; -+ // ZeroMemory(&ea, 1 * sizeof(EXPLICIT_ACCESS)); -+ // ea[0].grfAccessPermissions = KEY_READ; -+ // ea[0].grfAccessMode = SET_ACCESS; -+ // ea[0].grfInheritance = NO_INHERITANCE; -+ // ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID; -+ // ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; -+ // ea[0].Trustee.ptstrName = (LPTSTR)pEveryoneSID; -+ -+ // ACL acl; -+ // PACL pAcl = &acl; -+ // DWORD dwRes = SetEntriesInAcl(1, ea, NULL, &pAcl); -+ // dwRes; -+ -+ // SECURITY_DESCRIPTOR sd; -+ // success = InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION); -+ // success = SetSecurityDescriptorDacl(&sd, -+ // TRUE, // bDaclPresent flag -+ // pAcl, -+ // FALSE); -+ -+ // SECURITY_ATTRIBUTES sa; -+ // // Initialize a security attributes structure. -+ // sa.nLength = sizeof(SECURITY_ATTRIBUTES); -+ // sa.lpSecurityDescriptor = &sd; -+ // sa.bInheritHandle = FALSE; -+ // success; -+ -+ // wil::unique_hfile file{ CreateFileW(testPath.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr) }; -+ // THROW_LAST_ERROR_IF(!file); -+ -+ return *state; -+ } -+ -+ void ElevatedState::FromJson(const Json::Value& root) 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. -+#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) state->name = JsonUtils::GetValueForKey>(root, key); -+ MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) -+#undef MTSM_ELEVATED_STATE_GEN -+ } -+ Json::Value ElevatedState::ToJson() const noexcept -+ { -+ Json::Value root{ Json::objectValue }; -+ -+ { -+ auto state = _state.lock_shared(); -+#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) JsonUtils::SetValueForKey(root, key, state->name); -+ MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) -+#undef MTSM_ELEVATED_STATE_GEN -+ } -+ return root; -+ } -+ -+ // Generate all getter/setters -+#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) \ -+ type ElevatedState::name() const noexcept \ -+ { \ -+ const auto state = _state.lock_shared(); \ -+ const auto& value = state->name; \ -+ return value ? *value : type{ __VA_ARGS__ }; \ -+ } \ -+ \ -+ void ElevatedState::name(const type& value) noexcept \ -+ { \ -+ { \ -+ auto state = _state.lock(); \ -+ state->name.emplace(value); \ -+ } \ -+ \ -+ _throttler(); \ -+ } -+ MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) -+#undef MTSM_ELEVATED_STATE_GEN -+ -+ void ElevatedState::_writeFileContents(const std::string_view content) const -+ { -+ // 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(_path, content, true); -+ } -+ -+ std::optional ElevatedState::_readFileContents() const -+ { -+ return ReadUTF8FileIfExists(_path, true); -+ } -+} -diff --git a/src/cascadia/TerminalSettingsModel/ElevatedState.h b/src/cascadia/TerminalSettingsModel/ElevatedState.h -new file mode 100644 -index 000000000..d0eb8381f ---- /dev/null -+++ b/src/cascadia/TerminalSettingsModel/ElevatedState.h -@@ -0,0 +1,60 @@ -+/*++ -+Copyright (c) Microsoft Corporation -+Licensed under the MIT license. -+ -+Module Name: -+- ElevatedState.h -+ -+Abstract: -+- If the CascadiaSettings class were AppData, then this class would be LocalAppData. -+ Put anything in here that you wouldn't want to be stored next to user-editable settings. -+- Modify ElevatedState.idl and MTSM_ELEVATED_STATE_FIELDS to add new fields. -+--*/ -+#pragma once -+ -+#include "BaseApplicationState.h" -+#include "ElevatedState.g.h" -+#include -+ -+// This macro generates all getters and setters for ElevatedState. -+// It provides X with the following arguments: -+// (type, function name, JSON key, ...variadic construction arguments) -+#define MTSM_ELEVATED_STATE_FIELDS(X) \ -+ X(Windows::Foundation::Collections::IVector, AllowedCommandlines, "allowedCommandlines") -+ -+namespace winrt::Microsoft::Terminal::Settings::Model::implementation -+{ -+ struct ElevatedState : ElevatedStateT, public BaseApplicationState -+ { -+ static Microsoft::Terminal::Settings::Model::ElevatedState SharedInstance(); -+ -+ ElevatedState(std::filesystem::path path) noexcept; -+ -+ void FromJson(const Json::Value& root) const noexcept override; -+ Json::Value ToJson() const noexcept override; -+ -+ // State getters/setters -+#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) \ -+ type name() const noexcept; \ -+ void name(const type& value) noexcept; -+ MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) -+#undef MTSM_ELEVATED_STATE_GEN -+ -+ private: -+ struct state_t -+ { -+#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) std::optional name{ __VA_ARGS__ }; -+ MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) -+#undef MTSM_ELEVATED_STATE_GEN -+ }; -+ til::shared_mutex _state; -+ -+ virtual std::optional _readFileContents() const override; -+ virtual void _writeFileContents(const std::string_view content) const override; -+ }; -+} -+ -+namespace winrt::Microsoft::Terminal::Settings::Model::factory_implementation -+{ -+ BASIC_FACTORY(ElevatedState); -+} -diff --git a/src/cascadia/TerminalSettingsModel/ElevatedState.idl b/src/cascadia/TerminalSettingsModel/ElevatedState.idl -new file mode 100644 -index 000000000..09e2d05b0 ---- /dev/null -+++ b/src/cascadia/TerminalSettingsModel/ElevatedState.idl -@@ -0,0 +1,15 @@ -+// Copyright (c) Microsoft Corporation. -+// Licensed under the MIT license. -+ -+namespace Microsoft.Terminal.Settings.Model -+{ -+ [default_interface] runtimeclass ElevatedState { -+ static ElevatedState SharedInstance(); -+ -+ void Reload(); -+ -+ String FilePath { get; }; -+ -+ Windows.Foundation.Collections.IVector AllowedCommandlines { get; set; }; -+ } -+} -diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp -index 81fbb6cee..dff35897a 100644 ---- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp -+++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp -@@ -8,6 +8,8 @@ - #include - #include - -+#include -+ - static constexpr std::string_view Utf8Bom{ u8"\uFEFF" }; - static constexpr std::wstring_view UnpackagedSettingsFolderName{ L"Microsoft\\Windows Terminal\\" }; - -@@ -39,10 +41,89 @@ namespace Microsoft::Terminal::Settings::Model - return baseSettingsPath; - } - -+ static bool _hasExpectedPermissions(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: -+ // * Everyone has permission to read -+ // * admins can do anything -+ // * no one else can do anything. -+ PACL pAcl{ nullptr }; // This doesn't need to be cleanup up apparently -+ -+ auto status = GetNamedSecurityInfo(path.c_str(), -+ SE_FILE_OBJECT, -+ DACL_SECURITY_INFORMATION, -+ nullptr, -+ nullptr, -+ &pAcl, -+ nullptr, -+ nullptr); -+ THROW_IF_WIN32_ERROR(status); -+ -+ PEXPLICIT_ACCESS pEA{ nullptr }; -+ DWORD count = 0; -+ status = GetExplicitEntriesFromAcl(pAcl, &count, &pEA); -+ THROW_IF_WIN32_ERROR(status); -+ -+ auto explicitAccessCleanup = wil::scope_exit([&]() { ::LocalFree(pEA); }); -+ -+ if (count != 2) -+ { -+ return false; -+ } -+ -+ // 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)); -+ -+ // 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)); -+ -+ bool hadExpectedPermissions = true; -+ -+ // Check that the permissions are what we'd expect them to be if only -+ // admins can write to the file. This is basically a mirror of what we -+ // set up in `WriteUTF8File`. -+ -+ // For grfAccessPermissions, GENERIC_ALL turns into STANDARD_RIGHTS_ALL, -+ // and GENERIC_READ -> READ_CONTROL -+ hadExpectedPermissions = hadExpectedPermissions && WI_AreAllFlagsSet(pEA[0].grfAccessPermissions, STANDARD_RIGHTS_ALL); -+ hadExpectedPermissions = hadExpectedPermissions && pEA[0].grfInheritance == NO_INHERITANCE; -+ hadExpectedPermissions = hadExpectedPermissions && pEA[0].Trustee.TrusteeForm == TRUSTEE_IS_SID; -+ // SIDs are void*'s that happen to convert to a wchar_t -+ hadExpectedPermissions = hadExpectedPermissions && *(pEA[0].Trustee.ptstrName) == *(LPWSTR)(adminGroupSid.get()); -+ -+ // Now check the other EXPLICIT_ACCESS -+ hadExpectedPermissions = hadExpectedPermissions && WI_IsFlagSet(pEA[1].grfAccessPermissions, READ_CONTROL); -+ hadExpectedPermissions = hadExpectedPermissions && pEA[1].grfInheritance == NO_INHERITANCE; -+ hadExpectedPermissions = hadExpectedPermissions && pEA[1].Trustee.TrusteeForm == TRUSTEE_IS_SID; -+ hadExpectedPermissions = hadExpectedPermissions && *(pEA[1].Trustee.ptstrName) == *(LPWSTR)(everyoneSid.get()); -+ -+ return hadExpectedPermissions; -+ } - // 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) - { -+ if (elevatedOnly) -+ { -+ const bool hadExpectedPermissions{ _hasExpectedPermissions(path) }; -+ if (!hadExpectedPermissions) -+ { -+ // 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 ""; -+ } -+ } -+ - // From some casual observations we can determine that: - // * ReadFile() always returns the requested amount of data (unless the file is smaller) - // * It's unlikely that the file was changed between GetFileSize() and ReadFile() -@@ -89,11 +170,11 @@ namespace 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) - { -@@ -106,9 +187,75 @@ namespace 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; -+ if (elevatedOnly) -+ { -+ // This is very vaguely taken from -+ // https://docs.microsoft.com/en-us/windows/win32/secauthz/creating-a-security-descriptor-for-a-new-object-in-c-- -+ // With using https://docs.microsoft.com/en-us/windows/win32/secauthz/well-known-sids -+ // to find out that -+ // * SECURITY_NT_AUTHORITY+SECURITY_LOCAL_SYSTEM_RID == NT AUTHORITY\SYSTEM -+ // * SECURITY_NT_AUTHORITY+SECURITY_BUILTIN_DOMAIN_RID+DOMAIN_ALIAS_RID_ADMINS == BUILTIN\Administrators -+ // * SECURITY_WORLD_SID_AUTHORITY+SECURITY_WORLD_RID == Everyone -+ // -+ // Raymond Chen recommended that I make this file only writable by -+ // 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)); -+ -+ // 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)); -+ -+ EXPLICIT_ACCESS ea[2]; -+ ZeroMemory(&ea, 2 * sizeof(EXPLICIT_ACCESS)); -+ // Grant Admins all permissions on this file -+ ea[0].grfAccessPermissions = GENERIC_ALL; -+ ea[0].grfAccessMode = SET_ACCESS; -+ ea[0].grfInheritance = NO_INHERITANCE; -+ ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID; -+ ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; -+ ea[0].Trustee.ptstrName = (LPWSTR)(adminGroupSid.get()); -+ -+ // Grant Everyone the permission or read this file -+ ea[1].grfAccessPermissions = GENERIC_READ; -+ ea[1].grfAccessMode = SET_ACCESS; -+ ea[1].grfInheritance = NO_INHERITANCE; -+ ea[1].Trustee.TrusteeForm = TRUSTEE_IS_SID; -+ ea[1].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; -+ ea[1].Trustee.ptstrName = (LPWSTR)(everyoneSid.get()); -+ -+ ACL acl; -+ PACL pAcl = &acl; -+ THROW_IF_WIN32_ERROR(SetEntriesInAcl(2, ea, nullptr, &pAcl)); -+ -+ SECURITY_DESCRIPTOR sd; -+ THROW_IF_WIN32_BOOL_FALSE(InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)); -+ THROW_IF_WIN32_BOOL_FALSE(SetSecurityDescriptorDacl(&sd, true, pAcl, false)); -+ -+ // Initialize a security attributes structure. -+ sa.nLength = sizeof(SECURITY_ATTRIBUTES); -+ sa.lpSecurityDescriptor = &sd; -+ sa.bInheritHandle = false; -+ } -+ -+ wil::unique_hfile file{ CreateFileW(path.c_str(), -+ GENERIC_WRITE, -+ FILE_SHARE_READ | FILE_SHARE_WRITE, -+ elevatedOnly ? &sa : nullptr, -+ CREATE_ALWAYS, -+ FILE_ATTRIBUTE_NORMAL, -+ nullptr) }; - THROW_LAST_ERROR_IF(!file); - - const auto fileSize = gsl::narrow(content.size()); -@@ -121,7 +268,8 @@ namespace 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 d2e2eb53c..0426b4dd5 100644 ---- a/src/cascadia/TerminalSettingsModel/FileUtils.h -+++ b/src/cascadia/TerminalSettingsModel/FileUtils.h -@@ -4,8 +4,8 @@ - namespace Microsoft::Terminal::Settings::Model - { - std::filesystem::path GetBaseSettingsPath(); -- 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 58124d84b..1b26f7bae 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/Microsoft.Terminal.Settings.ModelLib.vcxproj b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj -index 202f07808..ce12741d1 100644 ---- a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj -+++ b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj -@@ -21,6 +21,7 @@ - - IconPathConverter.idl - -+ - - - ActionArgs.idl -@@ -35,6 +36,9 @@ - - ApplicationState.idl - -+ -+ ElevatedState.idl -+ - - CascadiaSettings.idl - -@@ -88,6 +92,7 @@ - IconPathConverter.idl - - -+ - - Create - -@@ -107,6 +112,9 @@ - - ApplicationState.idl - -+ -+ ElevatedState.idl -+ - - CascadiaSettings.idl - -@@ -156,6 +164,7 @@ - - - -+ - - - -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 From 51e0473560aacf68884817308a8d14624c72808a Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 14 Sep 2021 05:47:19 -0500 Subject: [PATCH 03/30] minor nits (cherry picked from commit 306ad30753a204e936bd2152a2b09dee7cd23137) --- .../BaseApplicationState.cpp | 5 +++ .../TerminalSettingsModel/ElevatedState.cpp | 39 ------------------- 2 files changed, 5 insertions(+), 39 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp b/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp index 3b5c0244628..378043c8267 100644 --- a/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp @@ -47,6 +47,8 @@ winrt::hstring BaseApplicationState::FilePath() const noexcept void BaseApplicationState::_read() const noexcept try { + // Use the derived class's implementation of _readFileContents to get the + // actual contents of the file. const auto data = _readFileContents().value_or(std::string{}); if (data.empty()) { @@ -77,6 +79,9 @@ try Json::StreamWriterBuilder wbuilder; const auto content = Json::writeString(wbuilder, root); + + // Use the derived class's implementation of _writeFileContents to write the + // file to disk. _writeFileContents(content); } CATCH_LOG() diff --git a/src/cascadia/TerminalSettingsModel/ElevatedState.cpp b/src/cascadia/TerminalSettingsModel/ElevatedState.cpp index f7d72207ed2..de813657180 100644 --- a/src/cascadia/TerminalSettingsModel/ElevatedState.cpp +++ b/src/cascadia/TerminalSettingsModel/ElevatedState.cpp @@ -23,47 +23,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // Returns the application-global ElevatedState object. Microsoft::Terminal::Settings::Model::ElevatedState ElevatedState::SharedInstance() { - // TODO! place in a totally different file! and path! static auto state = winrt::make_self(GetBaseSettingsPath() / stateFileName); state->Reload(); - - // const auto testPath{ GetBaseSettingsPath() / L"test.json" }; - - // PSID pEveryoneSID = NULL; - // SID_IDENTIFIER_AUTHORITY SIDAuthWorld = SECURITY_NT_AUTHORITY; - // BOOL success = AllocateAndInitializeSid(&SIDAuthWorld, 1, SECURITY_LOCAL_SYSTEM_RID, 0, 0, 0, 0, 0, 0, 0, &pEveryoneSID); - - // EXPLICIT_ACCESS ea[1]; - // ZeroMemory(&ea, 1 * sizeof(EXPLICIT_ACCESS)); - // ea[0].grfAccessPermissions = KEY_READ; - // ea[0].grfAccessMode = SET_ACCESS; - // ea[0].grfInheritance = NO_INHERITANCE; - // ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID; - // ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; - // ea[0].Trustee.ptstrName = (LPTSTR)pEveryoneSID; - - // ACL acl; - // PACL pAcl = &acl; - // DWORD dwRes = SetEntriesInAcl(1, ea, NULL, &pAcl); - // dwRes; - - // SECURITY_DESCRIPTOR sd; - // success = InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION); - // success = SetSecurityDescriptorDacl(&sd, - // TRUE, // bDaclPresent flag - // pAcl, - // FALSE); - - // SECURITY_ATTRIBUTES sa; - // // Initialize a security attributes structure. - // sa.nLength = sizeof(SECURITY_ATTRIBUTES); - // sa.lpSecurityDescriptor = &sd; - // sa.bInheritHandle = FALSE; - // success; - - // wil::unique_hfile file{ CreateFileW(testPath.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr) }; - // THROW_LAST_ERROR_IF(!file); - return *state; } From da0cc7bae56998878062b0ea98d70a013f9c78cb Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 14 Sep 2021 05:59:54 -0500 Subject: [PATCH 04/30] todo! in the code --- src/cascadia/TerminalSettingsModel/BaseApplicationState.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/BaseApplicationState.h b/src/cascadia/TerminalSettingsModel/BaseApplicationState.h index e684d319268..4193c206f3c 100644 --- a/src/cascadia/TerminalSettingsModel/BaseApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/BaseApplicationState.h @@ -3,10 +3,12 @@ Copyright (c) Microsoft Corporation Licensed under the MIT license. Module Name: -- ApplicationState.h +- BaseApplicationState.h Abstract: -- TODO! +- This is the common core for both ApplicationState and ElevatedState. This + handles more of the mechanics of serializing these structures to/from json, as + well as the mechanics of loading the file. --*/ #pragma once From 4e69a32de765590e459cbb9fcb84b2adbe56377c Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 14 Sep 2021 06:01:24 -0500 Subject: [PATCH 05/30] bunch of new allowed words --- .github/actions/spelling/allow/allow.txt | 1 + .github/actions/spelling/allow/apis.txt | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/.github/actions/spelling/allow/allow.txt b/.github/actions/spelling/allow/allow.txt index eb9b2e27771..b8cceabfc0f 100644 --- a/.github/actions/spelling/allow/allow.txt +++ b/.github/actions/spelling/allow/allow.txt @@ -1,3 +1,4 @@ +admins apc calt ccmp diff --git a/.github/actions/spelling/allow/apis.txt b/.github/actions/spelling/allow/apis.txt index 64d16180143..db3fd07a5ad 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 @@ -19,6 +21,7 @@ comparand cstdint CXICON CYICON +Dacl dataobject dcomp DERR @@ -112,9 +115,12 @@ OSVERSIONINFOEXW otms OUTLINETEXTMETRICW overridable +PACL PAGESCROLL +PEXPLICIT PICKFOLDERS pmr +ptstr rcx REGCLS RETURNCMD From 6757452d6d051839771dbf1bdf4925c105184ba2 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 14 Sep 2021 12:14:50 -0500 Subject: [PATCH 06/30] fix the tests --- src/cascadia/LocalTests_SettingsModel/pch.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/pch.h b/src/cascadia/LocalTests_SettingsModel/pch.h index 01b4cdfe87f..53f6145d27e 100644 --- a/src/cascadia/LocalTests_SettingsModel/pch.h +++ b/src/cascadia/LocalTests_SettingsModel/pch.h @@ -61,9 +61,6 @@ Author(s): // Manually include til after we include Windows.Foundation to give it winrt superpowers #include "til.h" -#include -#include - // Common includes for most tests: #include "../../inc/argb.h" #include "../../inc/conattrs.hpp" From a4acdeb5f20dd85f9d32c4ff8d3cc59a5718bbb3 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 20 Sep 2021 11:34:11 -0500 Subject: [PATCH 07/30] blindly remove ElevatedState --- src/cascadia/TerminalApp/AppLogic.cpp | 20 +++--------- .../ApplicationState.cpp | 30 ++++++++++++++++- .../TerminalSettingsModel/ApplicationState.h | 14 +++++--- .../ApplicationState.idl | 3 ++ .../BaseApplicationState.cpp | 11 ------- .../BaseApplicationState.h | 4 +-- .../TerminalSettingsModel/ElevatedState.idl | 1 - ...crosoft.Terminal.Settings.ModelLib.vcxproj | 7 ---- src/types/inc/utils.hpp | 1 + src/types/utils.cpp | 32 +++++++++++++++++++ 10 files changed, 81 insertions(+), 42 deletions(-) diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index 79e041704d7..a1fca21e6a4 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -11,6 +11,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; @@ -134,19 +136,8 @@ static Documents::Run _BuildErrorRun(const winrt::hstring& text, const ResourceD // Return Value: // - true if the user is an administrator static bool _isUserAdmin() noexcept -try -{ - SID_IDENTIFIER_AUTHORITY ntAuthority{ SECURITY_NT_AUTHORITY }; - wil::unique_sid adminGroupSid{}; - THROW_IF_WIN32_BOOL_FALSE(AllocateAndInitializeSid(&ntAuthority, 2, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS, 0, 0, 0, 0, 0, 0, &adminGroupSid)); - BOOL b; - THROW_IF_WIN32_BOOL_FALSE(CheckTokenMembership(NULL, adminGroupSid.get(), &b)); - return !!b; -} -catch (...) { - LOG_CAUGHT_EXCEPTION(); - return false; + return Microsoft::Console::Utils::IsElevated(); } namespace winrt::TerminalApp::implementation @@ -189,7 +180,7 @@ namespace winrt::TerminalApp::implementation } AppLogic::AppLogic() : - _reloadState{ std::chrono::milliseconds(100), []() { ApplicationState::SharedInstance().Reload(); ElevatedState::SharedInstance().Reload(); } } + _reloadState{ std::chrono::milliseconds(100), []() { ApplicationState::SharedInstance().Reload(); } } { // For your own sanity, it's better to do setup outside the ctor. // If you do any setup in the ctor that ends up throwing an exception, @@ -916,7 +907,6 @@ namespace winrt::TerminalApp::implementation wil::FolderChangeEvents::FileName | wil::FolderChangeEvents::LastWriteTime, [this, settingsPath](wil::FolderChangeEvent, PCWSTR fileModified) { static const std::filesystem::path statePath{ std::wstring_view{ ApplicationState::SharedInstance().FilePath() } }; - static const std::filesystem::path elevatedStatePath{ std::wstring_view{ ElevatedState::SharedInstance().FilePath() } }; const auto modifiedBasename = std::filesystem::path{ fileModified }.filename(); @@ -924,7 +914,7 @@ namespace winrt::TerminalApp::implementation { _reloadSettings->Run(); } - else if (modifiedBasename == statePath.filename() || modifiedBasename == elevatedStatePath.filename()) + else if (modifiedBasename == statePath.filename()) { _reloadState(); } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index ec7f5cd7acf..73f3e770fb3 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" }; @@ -66,7 +69,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // Returns the application-global ApplicationState object. Microsoft::Terminal::Settings::Model::ApplicationState ApplicationState::SharedInstance() { - static auto state = winrt::make_self(GetBaseSettingsPath() / stateFileName); + static auto state = winrt::make_self(GetBaseSettingsPath() / (::Microsoft::Console::Utils::IsElevated() ? elevatedStateFileName : stateFileName)); state->Reload(); return *state; } @@ -117,4 +120,29 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) #undef MTSM_APPLICATION_STATE_GEN + void ApplicationState::_writeFileContents(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(_path, content, true); + } + else + { + WriteUTF8FileAtomic(_path, content); + } + } + + std::optional ApplicationState::_readFileContents() const + { + return ReadUTF8FileIfExists(_path, ::Microsoft::Console::Utils::IsElevated()); + } } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index f7011289e0f..cd4d245d013 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -24,11 +24,12 @@ Module Name: // (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") +#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") \ + X(Windows::Foundation::Collections::IVector, AllowedCommandlines, "allowedCommandlines") struct WindowLayout : WindowLayoutT { @@ -63,6 +64,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation #undef MTSM_APPLICATION_STATE_GEN }; til::shared_mutex _state; + + virtual std::optional _readFileContents() const override; + virtual void _writeFileContents(const std::string_view content) const override; }; } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.idl b/src/cascadia/TerminalSettingsModel/ApplicationState.idl index 91231a112a1..5d27f0e4c86 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.idl +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.idl @@ -32,5 +32,8 @@ namespace Microsoft.Terminal.Settings.Model 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/BaseApplicationState.cpp b/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp index 378043c8267..a45df586524 100644 --- a/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp @@ -8,7 +8,6 @@ #include "JsonUtils.h" #include "FileUtils.h" -constexpr std::wstring_view stateFileName{ L"state.json" }; using namespace ::Microsoft::Terminal::Settings::Model; BaseApplicationState::BaseApplicationState(std::filesystem::path path) noexcept : @@ -85,13 +84,3 @@ try _writeFileContents(content); } CATCH_LOG() - -std::optional BaseApplicationState::_readFileContents() const -{ - return ReadUTF8FileIfExists(_path); -} - -void BaseApplicationState::_writeFileContents(const std::string_view content) const -{ - WriteUTF8FileAtomic(_path, content); -} diff --git a/src/cascadia/TerminalSettingsModel/BaseApplicationState.h b/src/cascadia/TerminalSettingsModel/BaseApplicationState.h index 4193c206f3c..35bc54bc65a 100644 --- a/src/cascadia/TerminalSettingsModel/BaseApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/BaseApplicationState.h @@ -27,8 +27,8 @@ struct BaseApplicationState virtual Json::Value ToJson() const noexcept = 0; protected: - virtual std::optional _readFileContents() const; - virtual void _writeFileContents(const std::string_view content) const; + virtual std::optional _readFileContents() const = 0; + virtual void _writeFileContents(const std::string_view content) const = 0; void _write() const noexcept; void _read() const noexcept; diff --git a/src/cascadia/TerminalSettingsModel/ElevatedState.idl b/src/cascadia/TerminalSettingsModel/ElevatedState.idl index 09e2d05b025..77654383a44 100644 --- a/src/cascadia/TerminalSettingsModel/ElevatedState.idl +++ b/src/cascadia/TerminalSettingsModel/ElevatedState.idl @@ -10,6 +10,5 @@ namespace Microsoft.Terminal.Settings.Model String FilePath { get; }; - Windows.Foundation.Collections.IVector AllowedCommandlines { get; set; }; } } diff --git a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj index ce12741d136..de191507ea8 100644 --- a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj +++ b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj @@ -36,9 +36,6 @@ ApplicationState.idl - - ElevatedState.idl - CascadiaSettings.idl @@ -112,9 +109,6 @@ ApplicationState.idl - - ElevatedState.idl - CascadiaSettings.idl @@ -164,7 +158,6 @@ - diff --git a/src/types/inc/utils.hpp b/src/types/inc/utils.hpp index d37b6e076d8..d278902174c 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 323263382a3..283960db108 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; +} From 9b3b9e010928ddb16d8d37393c6516ade03c04c3 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 20 Sep 2021 12:42:41 -0500 Subject: [PATCH 08/30] remove baseapplicationstate and just merge it back in --- .../ApplicationState.cpp | 72 +++++++++++++- .../TerminalSettingsModel/ApplicationState.h | 22 +++-- .../BaseApplicationState.cpp | 86 ----------------- .../BaseApplicationState.h | 38 -------- .../TerminalSettingsModel/ElevatedState.cpp | 95 ------------------- .../TerminalSettingsModel/ElevatedState.h | 60 ------------ .../TerminalSettingsModel/ElevatedState.idl | 14 --- ...crosoft.Terminal.Settings.ModelLib.vcxproj | 4 +- 8 files changed, 87 insertions(+), 304 deletions(-) delete mode 100644 src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp delete mode 100644 src/cascadia/TerminalSettingsModel/BaseApplicationState.h delete mode 100644 src/cascadia/TerminalSettingsModel/ElevatedState.cpp delete mode 100644 src/cascadia/TerminalSettingsModel/ElevatedState.h delete mode 100644 src/cascadia/TerminalSettingsModel/ElevatedState.idl diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index 73f3e770fb3..b63c4b33b80 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -64,13 +64,81 @@ using namespace ::Microsoft::Terminal::Settings::Model; namespace winrt::Microsoft::Terminal::Settings::Model::implementation { ApplicationState::ApplicationState(std::filesystem::path path) noexcept : - BaseApplicationState{ path } {} + _path{ std::move(path) }, + _throttler{ std::chrono::seconds(1), [this]() { _write(); } } + { + _read(); + } + + // The destructor ensures that the last write is flushed to disk before returning. + ApplicationState::~ApplicationState() + { + // 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(); + } + + // Re-read the state.json from disk. + void ApplicationState::Reload() const noexcept + { + _read(); + } + + // Returns the state.json path on the disk. + winrt::hstring ApplicationState::FilePath() const noexcept + { + return winrt::hstring{ _path.wstring() }; + } + + // Deserializes the state.json at _path 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 + { + // Use the derived class's implementation of _readFileContents to get the + // actual contents of the file. + const auto data = _readFileContents().value_or(std::string{}); + if (data.empty()) + { + return; + } + + std::string errs; + std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; + + Json::Value root; + if (!reader->parse(data.data(), data.data() + data.size(), &root, &errs)) + { + throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); + } + + FromJson(root); + } + CATCH_LOG() + + // Serialized this ApplicationState (in `context`) into the state.json at _path. + // * Errors are only logged. + // * _state->_writeScheduled is set to false, signaling our + // setters that _synchronize() needs to be called again. + void ApplicationState::_write() const noexcept + try + { + Json::Value root{ this->ToJson() }; + + Json::StreamWriterBuilder wbuilder; + const auto content = Json::writeString(wbuilder, root); + + // Use the derived class's implementation of _writeFileContents to write the + // file to disk. + _writeFileContents(content); + } + CATCH_LOG() // Returns the application-global ApplicationState object. Microsoft::Terminal::Settings::Model::ApplicationState ApplicationState::SharedInstance() { static auto state = winrt::make_self(GetBaseSettingsPath() / (::Microsoft::Console::Utils::IsElevated() ? elevatedStateFileName : stateFileName)); - state->Reload(); return *state; } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index cd4d245d013..b3109076869 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -12,7 +12,6 @@ Module Name: --*/ #pragma once -#include "BaseApplicationState.h" #include "ApplicationState.g.h" #include "WindowLayout.g.h" @@ -40,14 +39,20 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation friend ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait; }; - struct ApplicationState : public BaseApplicationState, ApplicationStateT + struct ApplicationState : public ApplicationStateT { static Microsoft::Terminal::Settings::Model::ApplicationState SharedInstance(); ApplicationState(std::filesystem::path path) noexcept; + ~ApplicationState(); - virtual void FromJson(const Json::Value& root) const noexcept override; - virtual Json::Value ToJson() const noexcept override; + // Methods + void Reload() const noexcept; + void FromJson(const Json::Value& root) const noexcept; + Json::Value ToJson() const noexcept; + + // General getters/setters + winrt::hstring FilePath() const noexcept; // State getters/setters #define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) \ @@ -64,9 +69,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation #undef MTSM_APPLICATION_STATE_GEN }; til::shared_mutex _state; + std::filesystem::path _path; + til::throttled_func_trailing<> _throttler; + + void _write() const noexcept; + void _read() const noexcept; - virtual std::optional _readFileContents() const override; - virtual void _writeFileContents(const std::string_view content) const override; + std::optional _readFileContents() const; + void _writeFileContents(const std::string_view content) const; }; } diff --git a/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp b/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp deleted file mode 100644 index a45df586524..00000000000 --- a/src/cascadia/TerminalSettingsModel/BaseApplicationState.cpp +++ /dev/null @@ -1,86 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -#include "pch.h" -#include "BaseApplicationState.h" -#include "CascadiaSettings.h" - -#include "JsonUtils.h" -#include "FileUtils.h" - -using namespace ::Microsoft::Terminal::Settings::Model; - -BaseApplicationState::BaseApplicationState(std::filesystem::path path) noexcept : - _path{ std::move(path) }, - _throttler{ std::chrono::seconds(1), [this]() { _write(); } } -{ - // DON'T _read() here! _read() will call FromJson, which is virtual, and - // needs to be implemented in a derived class. Classes that derive from - // BaseApplicationState should make sure to call Reload() after construction - // to ensure the data is loaded. -} - -// The destructor ensures that the last write is flushed to disk before returning. -BaseApplicationState::~BaseApplicationState() -{ - // 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(); -} - -// Re-read the state.json from disk. -void BaseApplicationState::Reload() const noexcept -{ - _read(); -} - -// Returns the state.json path on the disk. -winrt::hstring BaseApplicationState::FilePath() const noexcept -{ - return winrt::hstring{ _path.wstring() }; -} - -// Deserializes the state.json at _path 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 BaseApplicationState::_read() const noexcept -try -{ - // Use the derived class's implementation of _readFileContents to get the - // actual contents of the file. - const auto data = _readFileContents().value_or(std::string{}); - if (data.empty()) - { - return; - } - - std::string errs; - std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; - - Json::Value root; - if (!reader->parse(data.data(), data.data() + data.size(), &root, &errs)) - { - throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); - } - - FromJson(root); -} -CATCH_LOG() - -// Serialized this ApplicationState (in `context`) into the state.json at _path. -// * Errors are only logged. -// * _state->_writeScheduled is set to false, signaling our -// setters that _synchronize() needs to be called again. -void BaseApplicationState::_write() const noexcept -try -{ - Json::Value root{ this->ToJson() }; - - Json::StreamWriterBuilder wbuilder; - const auto content = Json::writeString(wbuilder, root); - - // Use the derived class's implementation of _writeFileContents to write the - // file to disk. - _writeFileContents(content); -} -CATCH_LOG() diff --git a/src/cascadia/TerminalSettingsModel/BaseApplicationState.h b/src/cascadia/TerminalSettingsModel/BaseApplicationState.h deleted file mode 100644 index 35bc54bc65a..00000000000 --- a/src/cascadia/TerminalSettingsModel/BaseApplicationState.h +++ /dev/null @@ -1,38 +0,0 @@ -/*++ -Copyright (c) Microsoft Corporation -Licensed under the MIT license. - -Module Name: -- BaseApplicationState.h - -Abstract: -- This is the common core for both ApplicationState and ElevatedState. This - handles more of the mechanics of serializing these structures to/from json, as - well as the mechanics of loading the file. ---*/ -#pragma once - -struct BaseApplicationState -{ - BaseApplicationState(std::filesystem::path path) noexcept; - ~BaseApplicationState(); - - // Methods - void Reload() const noexcept; - - // General getters/setters - winrt::hstring FilePath() const noexcept; - - virtual void FromJson(const Json::Value& root) const noexcept = 0; - virtual Json::Value ToJson() const noexcept = 0; - -protected: - virtual std::optional _readFileContents() const = 0; - virtual void _writeFileContents(const std::string_view content) const = 0; - - void _write() const noexcept; - void _read() const noexcept; - - std::filesystem::path _path; - til::throttled_func_trailing<> _throttler; -}; diff --git a/src/cascadia/TerminalSettingsModel/ElevatedState.cpp b/src/cascadia/TerminalSettingsModel/ElevatedState.cpp deleted file mode 100644 index de813657180..00000000000 --- a/src/cascadia/TerminalSettingsModel/ElevatedState.cpp +++ /dev/null @@ -1,95 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -#include "pch.h" -#include "ElevatedState.h" -#include "CascadiaSettings.h" -#include "ElevatedState.g.cpp" - -#include "JsonUtils.h" -#include "FileUtils.h" - -#include - -constexpr std::wstring_view stateFileName{ L"elevated-state.json" }; - -using namespace ::Microsoft::Terminal::Settings::Model; - -namespace winrt::Microsoft::Terminal::Settings::Model::implementation -{ - ElevatedState::ElevatedState(std::filesystem::path path) noexcept : - BaseApplicationState{ path } {} - - // Returns the application-global ElevatedState object. - Microsoft::Terminal::Settings::Model::ElevatedState ElevatedState::SharedInstance() - { - static auto state = winrt::make_self(GetBaseSettingsPath() / stateFileName); - state->Reload(); - return *state; - } - - void ElevatedState::FromJson(const Json::Value& root) 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. -#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) state->name = JsonUtils::GetValueForKey>(root, key); - MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) -#undef MTSM_ELEVATED_STATE_GEN - } - Json::Value ElevatedState::ToJson() const noexcept - { - Json::Value root{ Json::objectValue }; - - { - auto state = _state.lock_shared(); -#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) JsonUtils::SetValueForKey(root, key, state->name); - MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) -#undef MTSM_ELEVATED_STATE_GEN - } - return root; - } - - // Generate all getter/setters -#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) \ - type ElevatedState::name() const noexcept \ - { \ - const auto state = _state.lock_shared(); \ - const auto& value = state->name; \ - return value ? *value : type{ __VA_ARGS__ }; \ - } \ - \ - void ElevatedState::name(const type& value) noexcept \ - { \ - { \ - auto state = _state.lock(); \ - state->name.emplace(value); \ - } \ - \ - _throttler(); \ - } - MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) -#undef MTSM_ELEVATED_STATE_GEN - - void ElevatedState::_writeFileContents(const std::string_view content) const - { - // 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(_path, content, true); - } - - std::optional ElevatedState::_readFileContents() const - { - return ReadUTF8FileIfExists(_path, true); - } -} diff --git a/src/cascadia/TerminalSettingsModel/ElevatedState.h b/src/cascadia/TerminalSettingsModel/ElevatedState.h deleted file mode 100644 index d0eb8381fbe..00000000000 --- a/src/cascadia/TerminalSettingsModel/ElevatedState.h +++ /dev/null @@ -1,60 +0,0 @@ -/*++ -Copyright (c) Microsoft Corporation -Licensed under the MIT license. - -Module Name: -- ElevatedState.h - -Abstract: -- If the CascadiaSettings class were AppData, then this class would be LocalAppData. - Put anything in here that you wouldn't want to be stored next to user-editable settings. -- Modify ElevatedState.idl and MTSM_ELEVATED_STATE_FIELDS to add new fields. ---*/ -#pragma once - -#include "BaseApplicationState.h" -#include "ElevatedState.g.h" -#include - -// This macro generates all getters and setters for ElevatedState. -// It provides X with the following arguments: -// (type, function name, JSON key, ...variadic construction arguments) -#define MTSM_ELEVATED_STATE_FIELDS(X) \ - X(Windows::Foundation::Collections::IVector, AllowedCommandlines, "allowedCommandlines") - -namespace winrt::Microsoft::Terminal::Settings::Model::implementation -{ - struct ElevatedState : ElevatedStateT, public BaseApplicationState - { - static Microsoft::Terminal::Settings::Model::ElevatedState SharedInstance(); - - ElevatedState(std::filesystem::path path) noexcept; - - void FromJson(const Json::Value& root) const noexcept override; - Json::Value ToJson() const noexcept override; - - // State getters/setters -#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) \ - type name() const noexcept; \ - void name(const type& value) noexcept; - MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) -#undef MTSM_ELEVATED_STATE_GEN - - private: - struct state_t - { -#define MTSM_ELEVATED_STATE_GEN(type, name, key, ...) std::optional name{ __VA_ARGS__ }; - MTSM_ELEVATED_STATE_FIELDS(MTSM_ELEVATED_STATE_GEN) -#undef MTSM_ELEVATED_STATE_GEN - }; - til::shared_mutex _state; - - virtual std::optional _readFileContents() const override; - virtual void _writeFileContents(const std::string_view content) const override; - }; -} - -namespace winrt::Microsoft::Terminal::Settings::Model::factory_implementation -{ - BASIC_FACTORY(ElevatedState); -} diff --git a/src/cascadia/TerminalSettingsModel/ElevatedState.idl b/src/cascadia/TerminalSettingsModel/ElevatedState.idl deleted file mode 100644 index 77654383a44..00000000000 --- a/src/cascadia/TerminalSettingsModel/ElevatedState.idl +++ /dev/null @@ -1,14 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -namespace Microsoft.Terminal.Settings.Model -{ - [default_interface] runtimeclass ElevatedState { - static ElevatedState SharedInstance(); - - void Reload(); - - String FilePath { get; }; - - } -} diff --git a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj index 8203476de91..97109637533 100644 --- a/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj +++ b/src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj @@ -21,7 +21,6 @@ IconPathConverter.idl - ActionArgs.idl @@ -93,7 +92,6 @@ IconPathConverter.idl - Create @@ -267,4 +265,4 @@ - \ No newline at end of file + From b1b1befeb9e1773576a40f8abeac079202409016 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 22 Sep 2021 10:03:07 -0500 Subject: [PATCH 09/30] this is a crazy idea that I hate but gotta start somewhere --- src/cascadia/TerminalApp/AppLogic.cpp | 1 + .../ApplicationState.cpp | 127 +++++++++++------- .../TerminalSettingsModel/ApplicationState.h | 41 +++--- 3 files changed, 104 insertions(+), 65 deletions(-) diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index 8e478a6dc32..78c6c82a3d2 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -909,6 +909,7 @@ namespace winrt::TerminalApp::implementation // actual file you wrote. So listen for that too. wil::FolderChangeEvents::FileName | wil::FolderChangeEvents::LastWriteTime, [this, settingsPath](wil::FolderChangeEvent, PCWSTR fileModified) { + // TODO! static const std::filesystem::path statePath{ std::wstring_view{ ApplicationState::SharedInstance().FilePath() } }; const auto modifiedBasename = std::filesystem::path{ fileModified }.filename(); diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index b63c4b33b80..507d721a777 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -63,8 +63,10 @@ using namespace ::Microsoft::Terminal::Settings::Model; namespace winrt::Microsoft::Terminal::Settings::Model::implementation { - ApplicationState::ApplicationState(std::filesystem::path path) noexcept : - _path{ std::move(path) }, + ApplicationState::ApplicationState(std::filesystem::path sharedPath, + std::filesystem::path elevatedPath) noexcept : + _sharedPath{ std::move(sharedPath) }, + _elevatedPath{ std::move(elevatedPath) }, _throttler{ std::chrono::seconds(1), [this]() { _write(); } } { _read(); @@ -87,33 +89,43 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // Returns the state.json path on the disk. winrt::hstring ApplicationState::FilePath() const noexcept { - return winrt::hstring{ _path.wstring() }; + return winrt::hstring{ _sharedPath.wstring() }; } + // TODO! // Deserializes the state.json at _path 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 { - // Use the derived class's implementation of _readFileContents to get the - // actual contents of the file. - const auto data = _readFileContents().value_or(std::string{}); - if (data.empty()) - { - return; - } - std::string errs; std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; - Json::Value root; - if (!reader->parse(data.data(), data.data() + data.size(), &root, &errs)) + const auto sharedData = _readSharedContents().value_or(std::string{}); + if (!sharedData.empty()) { - throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); - } + 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)); + } - FromJson(root); + FromJson(root, FileSource::Shared); + } + if (::Microsoft::Console::Utils::IsElevated()) + { + if (const auto elevatedData{ _readElevatedContents().value_or(std::string{}) }; !elevatedData.empty()) + { + Json::Value root; + if (!reader->parse(elevatedData.data(), elevatedData.data() + elevatedData.size(), &root, &errs)) + { + throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); + } + + FromJson(root, FileSource::Local); + } + } } CATCH_LOG() @@ -124,25 +136,22 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void ApplicationState::_write() const noexcept try { - Json::Value root{ this->ToJson() }; - Json::StreamWriterBuilder wbuilder; - const auto content = Json::writeString(wbuilder, root); - // Use the derived class's implementation of _writeFileContents to write the - // file to disk. - _writeFileContents(content); + _writeSharedContents(Json::writeString(wbuilder, ToJson(FileSource::Shared))); + _writeElevatedContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); } CATCH_LOG() // Returns the application-global ApplicationState object. Microsoft::Terminal::Settings::Model::ApplicationState ApplicationState::SharedInstance() { - static auto state = winrt::make_self(GetBaseSettingsPath() / (::Microsoft::Console::Utils::IsElevated() ? elevatedStateFileName : stateFileName)); + static auto state = winrt::make_self(GetBaseSettingsPath() / stateFileName, + GetBaseSettingsPath() / elevatedStateFileName); return *state; } - void ApplicationState::FromJson(const Json::Value& root) const noexcept + void ApplicationState::FromJson(const Json::Value& root, FileSource parseSource) const noexcept { auto state = _state.lock(); // GetValueForKey() comes in two variants: @@ -150,17 +159,24 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // * 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, ...) state->name = JsonUtils::GetValueForKey>(root, key); +#define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) \ + if (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() const noexcept + + Json::Value ApplicationState::ToJson(FileSource parseSource) const noexcept { Json::Value root{ Json::objectValue }; { auto state = _state.lock_shared(); -#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) JsonUtils::SetValueForKey(root, key, state->name); +#define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) \ + if (parseSource == source) \ + JsonUtils::SetValueForKey(root, key, state->name); + MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) #undef MTSM_APPLICATION_STATE_GEN } @@ -168,27 +184,42 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } // 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); \ - } \ - \ - _throttler(); \ +#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 - void ApplicationState::_writeFileContents(const std::string_view content) const + std::optional ApplicationState::_readSharedContents() const + { + return ReadUTF8FileIfExists(_sharedPath); + } + + std::optional ApplicationState::_readElevatedContents() const + { + return ::Microsoft::Console::Utils::IsElevated() ? ReadUTF8FileIfExists(_elevatedPath, true) : std::nullopt; + } + + void ApplicationState::_writeSharedContents(const std::string_view content) const + { + WriteUTF8FileAtomic(_sharedPath, content); + } + + void ApplicationState::_writeElevatedContents(const std::string_view content) const { if (::Microsoft::Console::Utils::IsElevated()) { @@ -201,16 +232,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // 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(_path, content, true); + WriteUTF8File(_elevatedPath, content, true); } else { - WriteUTF8FileAtomic(_path, content); + // do nothing } } - std::optional ApplicationState::_readFileContents() const - { - return ReadUTF8FileIfExists(_path, ::Microsoft::Console::Utils::IsElevated()); - } } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index b3109076869..5e36e364a06 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -23,12 +23,19 @@ Module Name: // (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") \ - X(Windows::Foundation::Collections::IVector, AllowedCommandlines, "allowedCommandlines") + enum FileSource + { + Shared, + Local, + // ElevatedOnly + }; + +#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 { @@ -43,20 +50,21 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { static Microsoft::Terminal::Settings::Model::ApplicationState SharedInstance(); - ApplicationState(std::filesystem::path path) noexcept; + ApplicationState(std::filesystem::path sharedPath, + std::filesystem::path elevatedPath) noexcept; ~ApplicationState(); // Methods void Reload() const noexcept; - void FromJson(const Json::Value& root) const noexcept; - Json::Value ToJson() const 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; // 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 @@ -64,19 +72,22 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: struct state_t { -#define MTSM_APPLICATION_STATE_GEN(type, name, key, ...) std::optional name{ __VA_ARGS__ }; +#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 _path; + std::filesystem::path _sharedPath; + std::filesystem::path _elevatedPath; til::throttled_func_trailing<> _throttler; void _write() const noexcept; void _read() const noexcept; - std::optional _readFileContents() const; - void _writeFileContents(const std::string_view content) const; + std::optional _readSharedContents() const; + void _writeSharedContents(const std::string_view content) const; + std::optional _readElevatedContents() const; + void _writeElevatedContents(const std::string_view content) const; }; } From 7e2e4eaf49d0c1be40396123c71b2a74b3deed2e Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 22 Sep 2021 11:23:33 -0500 Subject: [PATCH 10/30] I think this works ALMOST as I want * [x] delete all state, open terminal elevated - generated profiles go to the correct place, approved commandlines go to the elevated one. * [x] delete some dynamic profiles, open the terminal - they don't resurrect in either IL * [ ] GAH saving a window layout unelevated, then trying to save one elevated will blow away the unelevated one --- .../TerminalSettingsModel/ApplicationState.cpp | 13 ++++++++++--- .../TerminalSettingsModel/ApplicationState.h | 7 ++++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index 507d721a777..ee42da0480e 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -138,8 +138,15 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { Json::StreamWriterBuilder wbuilder; - _writeSharedContents(Json::writeString(wbuilder, ToJson(FileSource::Shared))); - _writeElevatedContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); + if (::Microsoft::Console::Utils::IsElevated()) + { + _writeSharedContents(Json::writeString(wbuilder, ToJson(FileSource::Shared))); + _writeElevatedContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); + } + else + { + _writeSharedContents(Json::writeString(wbuilder, ToJson(FileSource::Shared | FileSource::Local))); + } } CATCH_LOG() @@ -174,7 +181,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { auto state = _state.lock_shared(); #define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) \ - if (parseSource == source) \ + if (WI_IsFlagSet(parseSource, source)) \ JsonUtils::SetValueForKey(root, key, state->name); MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index 5e36e364a06..c507f550caa 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -23,12 +23,13 @@ Module Name: // (type, function name, JSON key, ...variadic construction arguments) namespace winrt::Microsoft::Terminal::Settings::Model::implementation { - enum FileSource + enum FileSource : int { - Shared, - Local, + Shared = 0x1, + Local = 0x2, // ElevatedOnly }; + DEFINE_ENUM_FLAG_OPERATORS(FileSource); #define MTSM_APPLICATION_STATE_FIELDS(X) \ X(FileSource::Shared, std::unordered_set, GeneratedProfiles, "generatedProfiles") \ From f3738f5c1ba67b9696f7b031491f761d00804acd Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 22 Sep 2021 12:07:34 -0500 Subject: [PATCH 11/30] Move window state, approvedCommandlines into `user-state.json` This fixes the issue I had in the last commit. It's a little weird, but gets rid of the muckiness of layering. Things that are local to one elevation level won't pollute the other, and we don't need to worry about layering or where they came from. Just write shared state to `state.json`, and window state to `elevated-state`/`user-state` --- .../ApplicationState.cpp | 49 ++++++++----------- .../TerminalSettingsModel/ApplicationState.h | 8 +-- 2 files changed, 25 insertions(+), 32 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index ee42da0480e..8d063eb2ae1 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -13,6 +13,7 @@ static constexpr std::wstring_view stateFileName{ L"state.json" }; static constexpr std::wstring_view elevatedStateFileName{ L"elevated-state.json" }; +static constexpr std::wstring_view unelevatedStateFileName{ L"user-state.json" }; static constexpr std::string_view TabLayoutKey{ "tabLayout" }; static constexpr std::string_view InitialPositionKey{ "initialPosition" }; @@ -63,10 +64,10 @@ using namespace ::Microsoft::Terminal::Settings::Model; namespace winrt::Microsoft::Terminal::Settings::Model::implementation { - ApplicationState::ApplicationState(std::filesystem::path sharedPath, - std::filesystem::path elevatedPath) noexcept : - _sharedPath{ std::move(sharedPath) }, - _elevatedPath{ std::move(elevatedPath) }, + ApplicationState::ApplicationState(const std::filesystem::path& stateRoot) noexcept : + _sharedPath{ stateRoot / stateFileName }, + _userPath{ stateRoot / unelevatedStateFileName }, + _elevatedPath{ stateRoot / elevatedStateFileName }, _throttler{ std::chrono::seconds(1), [this]() { _write(); } } { _read(); @@ -113,18 +114,15 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation FromJson(root, FileSource::Shared); } - if (::Microsoft::Console::Utils::IsElevated()) + if (const auto localData{ _readLocalContents().value_or(std::string{}) }; !localData.empty()) { - if (const auto elevatedData{ _readElevatedContents().value_or(std::string{}) }; !elevatedData.empty()) + Json::Value root; + if (!reader->parse(localData.data(), localData.data() + localData.size(), &root, &errs)) { - Json::Value root; - if (!reader->parse(elevatedData.data(), elevatedData.data() + elevatedData.size(), &root, &errs)) - { - throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); - } - - FromJson(root, FileSource::Local); + throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); } + + FromJson(root, FileSource::Local); } } CATCH_LOG() @@ -138,23 +136,16 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { Json::StreamWriterBuilder wbuilder; - if (::Microsoft::Console::Utils::IsElevated()) - { - _writeSharedContents(Json::writeString(wbuilder, ToJson(FileSource::Shared))); - _writeElevatedContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); - } - else - { - _writeSharedContents(Json::writeString(wbuilder, ToJson(FileSource::Shared | FileSource::Local))); - } + _writeSharedContents(Json::writeString(wbuilder, ToJson(FileSource::Shared))); + _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); } CATCH_LOG() // Returns the application-global ApplicationState object. Microsoft::Terminal::Settings::Model::ApplicationState ApplicationState::SharedInstance() { - static auto state = winrt::make_self(GetBaseSettingsPath() / stateFileName, - GetBaseSettingsPath() / elevatedStateFileName); + std::filesystem::path root{ GetBaseSettingsPath() }; + static auto state = winrt::make_self(root); return *state; } @@ -216,9 +207,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return ReadUTF8FileIfExists(_sharedPath); } - std::optional ApplicationState::_readElevatedContents() const + std::optional ApplicationState::_readLocalContents() const { - return ::Microsoft::Console::Utils::IsElevated() ? ReadUTF8FileIfExists(_elevatedPath, true) : std::nullopt; + return ::Microsoft::Console::Utils::IsElevated() ? + ReadUTF8FileIfExists(_elevatedPath, true) : + ReadUTF8FileIfExists(_userPath, false); } void ApplicationState::_writeSharedContents(const std::string_view content) const @@ -226,7 +219,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation WriteUTF8FileAtomic(_sharedPath, content); } - void ApplicationState::_writeElevatedContents(const std::string_view content) const + void ApplicationState::_writeLocalContents(const std::string_view content) const { if (::Microsoft::Console::Utils::IsElevated()) { @@ -243,7 +236,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } else { - // do nothing + WriteUTF8FileAtomic(_userPath, content); } } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index c507f550caa..f1e328efbe6 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -51,8 +51,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { static Microsoft::Terminal::Settings::Model::ApplicationState SharedInstance(); - ApplicationState(std::filesystem::path sharedPath, - std::filesystem::path elevatedPath) noexcept; + ApplicationState(const std::filesystem::path& stateRoot) noexcept; ~ApplicationState(); // Methods @@ -79,6 +78,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation }; til::shared_mutex _state; std::filesystem::path _sharedPath; + std::filesystem::path _userPath; std::filesystem::path _elevatedPath; til::throttled_func_trailing<> _throttler; @@ -87,8 +87,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::optional _readSharedContents() const; void _writeSharedContents(const std::string_view content) const; - std::optional _readElevatedContents() const; - void _writeElevatedContents(const std::string_view content) const; + std::optional _readLocalContents() const; + void _writeLocalContents(const std::string_view content) const; }; } From a6e044d91c24857b77a681298e5245531a53b5cf Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 22 Sep 2021 12:26:33 -0500 Subject: [PATCH 12/30] pr nits --- .../TerminalSettingsModel/FileUtils.cpp | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index dff35897a3e..89286de238c 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -75,8 +75,8 @@ namespace 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{}; + wil::unique_sid everyoneSid; + wil::unique_sid adminGroupSid; SID_IDENTIFIER_AUTHORITY SIDAuthNT = SECURITY_NT_AUTHORITY; SID_IDENTIFIER_AUTHORITY SIDAuthWorld = SECURITY_WORLD_SID_AUTHORITY; @@ -94,17 +94,17 @@ namespace Microsoft::Terminal::Settings::Model // For grfAccessPermissions, GENERIC_ALL turns into STANDARD_RIGHTS_ALL, // and GENERIC_READ -> READ_CONTROL - hadExpectedPermissions = hadExpectedPermissions && WI_AreAllFlagsSet(pEA[0].grfAccessPermissions, STANDARD_RIGHTS_ALL); - hadExpectedPermissions = hadExpectedPermissions && pEA[0].grfInheritance == NO_INHERITANCE; - hadExpectedPermissions = hadExpectedPermissions && pEA[0].Trustee.TrusteeForm == TRUSTEE_IS_SID; + hadExpectedPermissions &= WI_AreAllFlagsSet(pEA[0].grfAccessPermissions, STANDARD_RIGHTS_ALL); + hadExpectedPermissions &= pEA[0].grfInheritance == NO_INHERITANCE; + hadExpectedPermissions &= pEA[0].Trustee.TrusteeForm == TRUSTEE_IS_SID; // SIDs are void*'s that happen to convert to a wchar_t - hadExpectedPermissions = hadExpectedPermissions && *(pEA[0].Trustee.ptstrName) == *(LPWSTR)(adminGroupSid.get()); + hadExpectedPermissions &= *(pEA[0].Trustee.ptstrName) == *(LPWSTR)(adminGroupSid.get()); // Now check the other EXPLICIT_ACCESS - hadExpectedPermissions = hadExpectedPermissions && WI_IsFlagSet(pEA[1].grfAccessPermissions, READ_CONTROL); - hadExpectedPermissions = hadExpectedPermissions && pEA[1].grfInheritance == NO_INHERITANCE; - hadExpectedPermissions = hadExpectedPermissions && pEA[1].Trustee.TrusteeForm == TRUSTEE_IS_SID; - hadExpectedPermissions = hadExpectedPermissions && *(pEA[1].Trustee.ptstrName) == *(LPWSTR)(everyoneSid.get()); + hadExpectedPermissions &= WI_IsFlagSet(pEA[1].grfAccessPermissions, READ_CONTROL); + hadExpectedPermissions &= pEA[1].grfInheritance == NO_INHERITANCE; + hadExpectedPermissions &= pEA[1].Trustee.TrusteeForm == TRUSTEE_IS_SID; + hadExpectedPermissions &= *(pEA[1].Trustee.ptstrName) == *(LPWSTR)(everyoneSid.get()); return hadExpectedPermissions; } @@ -206,8 +206,8 @@ namespace 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{}; + wil::unique_sid everyoneSid; + wil::unique_sid adminGroupSid; SID_IDENTIFIER_AUTHORITY SIDAuthNT = SECURITY_NT_AUTHORITY; SID_IDENTIFIER_AUTHORITY SIDAuthWorld = SECURITY_WORLD_SID_AUTHORITY; @@ -217,8 +217,8 @@ namespace Microsoft::Terminal::Settings::Model // 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)); - EXPLICIT_ACCESS ea[2]; - ZeroMemory(&ea, 2 * sizeof(EXPLICIT_ACCESS)); + EXPLICIT_ACCESS ea[2]{}; + // Grant Admins all permissions on this file ea[0].grfAccessPermissions = GENERIC_ALL; ea[0].grfAccessMode = SET_ACCESS; From eee657b5026d665f75bd0f3273d2a2620a32b5ed Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 22 Sep 2021 14:35:02 -0500 Subject: [PATCH 13/30] fix hot reloading for this file --- src/cascadia/TerminalApp/AppLogic.cpp | 12 +++--- .../ApplicationState.cpp | 40 ++++++++++++++++--- .../TerminalSettingsModel/ApplicationState.h | 15 ++++--- .../ApplicationState.idl | 2 +- 4 files changed, 50 insertions(+), 19 deletions(-) diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index 78c6c82a3d2..e1d2aeee24c 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -908,17 +908,15 @@ 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, settingsPath](wil::FolderChangeEvent, PCWSTR fileModified) { - // TODO! - static const std::filesystem::path statePath{ std::wstring_view{ ApplicationState::SharedInstance().FilePath() } }; + [this, settingsBasename = settingsPath.filename()](wil::FolderChangeEvent, PCWSTR fileModified) { + static const auto appState{ ApplicationState::SharedInstance() }; + const winrt::hstring modifiedBasename{ std::filesystem::path{ fileModified }.filename().c_str() }; - const auto modifiedBasename = std::filesystem::path{ fileModified }.filename(); - - if (modifiedBasename == settingsPath.filename()) + if (modifiedBasename == settingsBasename) { _reloadSettings->Run(); } - else if (modifiedBasename == statePath.filename()) + else if (appState.IsStatePath(modifiedBasename)) { _reloadState(); } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index 8d063eb2ae1..2e58586a402 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -87,14 +87,16 @@ 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{ _sharedPath.wstring() }; + static const auto sharedPath{ _sharedPath.filename() }; + static const auto elevatedPath{ _elevatedPath.filename() }; + static const auto userPath{ _userPath.filename() }; + return filename == sharedPath || filename == elevatedPath || filename == userPath; } - // TODO! - // 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 @@ -103,6 +105,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::string errs; std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; + // First get shared state out of `state.json` into us const auto sharedData = _readSharedContents().value_or(std::string{}); if (!sharedData.empty()) { @@ -114,6 +117,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation FromJson(root, FileSource::Shared); } + + // Then, try and get anything in user-state/elevated-state if (const auto localData{ _readLocalContents().value_or(std::string{}) }; !localData.empty()) { Json::Value root; @@ -149,6 +154,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation 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(); @@ -202,11 +212,22 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation 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 cmdpal 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() ? @@ -214,11 +235,20 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation ReadUTF8FileIfExists(_userPath, 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()) diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index f1e328efbe6..396a1bfea29 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -18,19 +18,22 @@ Module Name: #include #include -// 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 { + // 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, - // ElevatedOnly + Local = 0x2 }; DEFINE_ENUM_FLAG_OPERATORS(FileSource); +// This macro generates all getters and setters for ApplicationState. +// It provides X with the following arguments: +// (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") \ @@ -60,7 +63,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation 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(source, type, name, key, ...) \ diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.idl b/src/cascadia/TerminalSettingsModel/ApplicationState.idl index 5d27f0e4c86..765f44b784d 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.idl +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.idl @@ -25,7 +25,7 @@ namespace Microsoft.Terminal.Settings.Model void Reload(); - String FilePath { get; }; + Boolean IsStatePath(String filename); Windows.Foundation.Collections.IVector PersistedWindowLayouts { get; set; }; From 5ff9a247d493311a618c9a46d4ac234d79e5499e Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 22 Sep 2021 14:49:46 -0500 Subject: [PATCH 14/30] okay so I guess that's not a word --- src/cascadia/TerminalSettingsModel/ApplicationState.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index 2e58586a402..7e56398882c 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -215,7 +215,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // 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 cmdpal commandlines. + // generated profiles, the command palette commandlines. std::optional ApplicationState::_readSharedContents() const { return ReadUTF8FileIfExists(_sharedPath); From 866832b665dadc1c95ecce063ead23b6eb32a833 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 30 Sep 2021 09:39:17 -0500 Subject: [PATCH 15/30] This seemingly works the way I'd expect, going to merge into the warning dialog and check --- src/cascadia/TerminalApp/TerminalPage.cpp | 3 - .../ApplicationState.cpp | 72 +++++++++++++------ .../TerminalSettingsModel/ApplicationState.h | 2 + 3 files changed, 53 insertions(+), 24 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 25140362de8..b80aa54d328 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -298,10 +298,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 1874dd24aaf..f1c31297cb8 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -13,7 +13,7 @@ static constexpr std::wstring_view stateFileName{ L"state.json" }; static constexpr std::wstring_view elevatedStateFileName{ L"elevated-state.json" }; -static constexpr std::wstring_view unelevatedStateFileName{ L"user-state.json" }; +// static constexpr std::wstring_view unelevatedStateFileName{ L"user-state.json" }; static constexpr std::string_view TabLayoutKey{ "tabLayout" }; static constexpr std::string_view InitialPositionKey{ "initialPosition" }; @@ -91,7 +91,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation ApplicationState::ApplicationState(const std::filesystem::path& stateRoot) noexcept : _sharedPath{ stateRoot / stateFileName }, - _userPath{ stateRoot / unelevatedStateFileName }, + // _userPath{ stateRoot / unelevatedStateFileName }, _elevatedPath{ stateRoot / elevatedStateFileName }, _throttler{ std::chrono::seconds(1), [this]() { _write(); } } { @@ -116,8 +116,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { static const auto sharedPath{ _sharedPath.filename() }; static const auto elevatedPath{ _elevatedPath.filename() }; - static const auto userPath{ _userPath.filename() }; - return filename == sharedPath || filename == elevatedPath || filename == userPath; + // static const auto userPath{ _userPath.filename() }; + return filename == sharedPath || filename == elevatedPath /* || filename == userPath*/; } // Deserializes the state.json and user-state (or elevated-state if @@ -140,19 +140,26 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); } - FromJson(root, FileSource::Shared); - } - - // Then, try and get anything in user-state/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)) + if (::Microsoft::Console::Utils::IsElevated()) { - throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); + // Only load shared properties if we're elevated + FromJson(root, FileSource::Shared); + // Then, try and get anything in user-state/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); } - - FromJson(root, FileSource::Local); } } CATCH_LOG() @@ -165,9 +172,28 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation try { Json::StreamWriterBuilder wbuilder; - - _writeSharedContents(Json::writeString(wbuilder, ToJson(FileSource::Shared))); - _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); + if (::Microsoft::Console::Utils::IsElevated()) + { + std::string errs; + std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; + Json::Value root; + 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)); + } + } + _writeSharedContents(Json::writeString(wbuilder, _toJsonWithBlob(root, FileSource::Shared))); + _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); + } + else + { + _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local | FileSource::Shared))); + } + // _writeSharedContents(Json::writeString(wbuilder, ToJson(FileSource::Shared))); + // _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); } CATCH_LOG() @@ -193,7 +219,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // 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(source, type, name, key, ...) \ - if (parseSource == source) \ + if (WI_IsFlagSet(parseSource, source)) \ state->name = JsonUtils::GetValueForKey>(root, key); MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) @@ -203,7 +229,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation 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(); #define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) \ @@ -257,7 +287,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { return ::Microsoft::Console::Utils::IsElevated() ? ReadUTF8FileIfExists(_elevatedPath, true) : - ReadUTF8FileIfExists(_userPath, false); + ReadUTF8FileIfExists(_sharedPath, false); } // Method Description: @@ -291,7 +321,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } else { - WriteUTF8FileAtomic(_userPath, content); + WriteUTF8FileAtomic(_sharedPath, content); } } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index ed6db33fe2d..80bde4cf103 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -91,6 +91,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _write() const noexcept; void _read() const noexcept; + 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; From b4e0496effa0a6dea5efb842b4784e377b62b017 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 30 Sep 2021 10:27:57 -0500 Subject: [PATCH 16/30] cleanup --- .../ApplicationState.cpp | 50 +++++++++++++++---- 1 file changed, 41 insertions(+), 9 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index f1c31297cb8..88c36697e4b 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -13,7 +13,6 @@ static constexpr std::wstring_view stateFileName{ L"state.json" }; static constexpr std::wstring_view elevatedStateFileName{ L"elevated-state.json" }; -// static constexpr std::wstring_view unelevatedStateFileName{ L"user-state.json" }; static constexpr std::string_view TabLayoutKey{ "tabLayout" }; static constexpr std::string_view InitialPositionKey{ "initialPosition" }; @@ -91,7 +90,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation ApplicationState::ApplicationState(const std::filesystem::path& stateRoot) noexcept : _sharedPath{ stateRoot / stateFileName }, - // _userPath{ stateRoot / unelevatedStateFileName }, _elevatedPath{ stateRoot / elevatedStateFileName }, _throttler{ std::chrono::seconds(1), [this]() { _write(); } } { @@ -116,8 +114,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { static const auto sharedPath{ _sharedPath.filename() }; static const auto elevatedPath{ _elevatedPath.filename() }; - // static const auto userPath{ _userPath.filename() }; - return filename == sharedPath || filename == elevatedPath /* || filename == userPath*/; + return filename == sharedPath || filename == elevatedPath; } // Deserializes the state.json and user-state (or elevated-state if @@ -130,7 +127,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::string errs; std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; - // First get shared state out of `state.json` into us + // First get shared state out of `state.json`. const auto sharedData = _readSharedContents().value_or(std::string{}); if (!sharedData.empty()) { @@ -140,11 +137,16 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation 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 user-state/elevated-state + + // Then, try and get anything in elevated-state if (const auto localData{ _readLocalContents().value_or(std::string{}) }; !localData.empty()) { Json::Value root; @@ -172,11 +174,30 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation try { 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()) { std::string errs; std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; Json::Value root; + + // First load the contents of state.json into a json blob. This will + // contain the Shared properties and teh unelevated instance's Local + // properties. const auto sharedData = _readSharedContents().value_or(std::string{}); if (!sharedData.empty()) { @@ -185,15 +206,18 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation 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))); + + // 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))); } - // _writeSharedContents(Json::writeString(wbuilder, ToJson(FileSource::Shared))); - // _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); } CATCH_LOG() @@ -218,8 +242,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // * 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)) \ + if (WI_IsFlagSet(parseSource, source)) \ state->name = JsonUtils::GetValueForKey>(root, key); MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) @@ -236,6 +264,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { { 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); From 48b20de4f4279cdffea710fcd1b456cf6c0766af Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 30 Sep 2021 11:15:35 -0500 Subject: [PATCH 17/30] Add some logging. Can't seem to get the crash to repro? --- .../ApplicationState.cpp | 12 +++++++ src/cascadia/WindowsTerminal/AppHost.cpp | 32 +++++++++++++++++-- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index acd876884b0..3023789af3f 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -102,9 +102,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. diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index c2b8dcda2a1..4111ef66e70 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -819,8 +819,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 occured when collecting or writing window state"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + } } co_return; @@ -841,6 +863,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()(); } } From aea37520b3cdb1c1752a6c8e0ff598991518ce28 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 30 Sep 2021 12:47:03 -0500 Subject: [PATCH 18/30] we want this --- src/cascadia/TerminalApp/AppLogic.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index 4efbb57a2f6..350426f9268 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -886,14 +886,19 @@ 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) { - static const auto appState{ ApplicationState::SharedInstance() }; + // 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. + const winrt::hstring modifiedBasename{ std::filesystem::path{ fileModified }.filename().c_str() }; if (modifiedBasename == settingsBasename) { _reloadSettings->Run(); } - else if (appState.IsStatePath(modifiedBasename)) + else if (ApplicationState::SharedInstance().IsStatePath(modifiedBasename)) { _reloadState(); } From ae99ce9c368666a0abf8b188e8050ff814c6d4ee Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 30 Sep 2021 12:48:09 -0500 Subject: [PATCH 19/30] teh --- src/cascadia/TerminalSettingsModel/ApplicationState.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index 9c8d4f58fbf..cd04852800b 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -208,7 +208,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Json::Value root; // First load the contents of state.json into a json blob. This will - // contain the Shared properties and teh unelevated instance's Local + // contain the Shared properties and the unelevated instance's Local // properties. const auto sharedData = _readSharedContents().value_or(std::string{}); if (!sharedData.empty()) From bf3c6e7029de817380b62ac56b45494dd5ffcc56 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 30 Sep 2021 12:53:08 -0500 Subject: [PATCH 20/30] spel is hard --- src/cascadia/WindowsTerminal/AppHost.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 4111ef66e70..298b9b4893b 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -839,7 +839,7 @@ winrt::Windows::Foundation::IAsyncAction AppHost::_SaveWindowLayouts() LOG_CAUGHT_EXCEPTION(); TraceLoggingWrite(g_hWindowsTerminalProvider, "AppHost_SaveWindowLayouts_Failed", - TraceLoggingDescription("An error occured when collecting or writing window state"), + TraceLoggingDescription("An error occurred when collecting or writing window state"), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingKeyword(TIL_KEYWORD_TRACE)); } From 4976a091a057a6ffd49a06ac764534931265f45e Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 4 Nov 2021 12:48:45 -0500 Subject: [PATCH 21/30] 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 0477503833c..6df50621024 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 845b37567e2..b509fdc3bc2 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 59919bba73c..1dc18a68e13 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; From fd849a5241fe7ccca39743f545c0085e23245ceb Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 8 Nov 2021 14:30:07 -0600 Subject: [PATCH 22/30] trying to do the thing eryksun mentioned. This seems to actually work to prevent non-admins from writing the file, and BOY is it simple. Still doesn't prevent the vim situation, but also do we need to prevent that footgun? probably --- src/cascadia/TerminalApp/AppLogic.cpp | 2 +- .../TerminalSettingsModel/FileUtils.cpp | 103 ++++++++++-------- 2 files changed, 60 insertions(+), 45 deletions(-) diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index 6df50621024..5fd2cb9c425 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -184,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 = Microsoft::Console::Utils::IsElevated(); + _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()]() { diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index b509fdc3bc2..608261c1d3b 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -9,6 +9,8 @@ #include #include +#include +#include static constexpr std::string_view Utf8Bom{ u8"\uFEFF" }; static constexpr std::wstring_view UnpackagedSettingsFolderName{ L"Microsoft\\Windows Terminal\\" }; @@ -53,6 +55,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model // - true if it had the expected permissions. False otherwise. static bool _hasElevatedOnlyPermissions(const std::filesystem::path& path) { + path; + return true; + /* // If we want to only open the file if it's elevated, check the // permissions on this file. We want to make sure that: // * Everyone has permission to read @@ -111,6 +116,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model hadExpectedPermissions &= *(pEA[1].Trustee.ptstrName) == *(LPWSTR)(everyoneSid.get()); return hadExpectedPermissions; + */ } // Tries to read a file somewhat atomically without locking it. // Strips the UTF8 BOM if it exists. @@ -198,53 +204,62 @@ namespace winrt::Microsoft::Terminal::Settings::Model SECURITY_ATTRIBUTES sa; if (elevatedOnly) { - // This is very vaguely taken from - // https://docs.microsoft.com/en-us/windows/win32/secauthz/creating-a-security-descriptor-for-a-new-object-in-c-- - // With using https://docs.microsoft.com/en-us/windows/win32/secauthz/well-known-sids - // to find out that - // * SECURITY_NT_AUTHORITY+SECURITY_LOCAL_SYSTEM_RID == NT AUTHORITY\SYSTEM - // * SECURITY_NT_AUTHORITY+SECURITY_BUILTIN_DOMAIN_RID+DOMAIN_ALIAS_RID_ADMINS == BUILTIN\Administrators - // * SECURITY_WORLD_SID_AUTHORITY+SECURITY_WORLD_RID == Everyone - // - // Raymond Chen recommended that I make this file only writable by - // SYSTEM, but if I did that, then even we can't write the file - // while elevated, which isn't what we want. - - // Create a SID for the BUILTIN\Administrators group. - 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. - const auto everyoneSid = wil::make_static_sid(SECURITY_WORLD_SID_AUTHORITY, SECURITY_WORLD_RID); - - EXPLICIT_ACCESS ea[2]{}; - - // Grant Admins all permissions on this file - ea[0].grfAccessPermissions = GENERIC_ALL; - ea[0].grfAccessMode = SET_ACCESS; - ea[0].grfInheritance = NO_INHERITANCE; - ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID; - ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; - ea[0].Trustee.ptstrName = (LPWSTR)(adminGroupSid.get()); - - // Grant Everyone the permission or read this file - ea[1].grfAccessPermissions = GENERIC_READ; - ea[1].grfAccessMode = SET_ACCESS; - ea[1].grfInheritance = NO_INHERITANCE; - ea[1].Trustee.TrusteeForm = TRUSTEE_IS_SID; - ea[1].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; - ea[1].Trustee.ptstrName = (LPWSTR)(everyoneSid.get()); - - ACL acl; - PACL pAcl = &acl; - THROW_IF_WIN32_ERROR(SetEntriesInAcl(2, ea, nullptr, &pAcl)); - - SECURITY_DESCRIPTOR sd; - THROW_IF_WIN32_BOOL_FALSE(InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)); - THROW_IF_WIN32_BOOL_FALSE(SetSecurityDescriptorDacl(&sd, true, pAcl, false)); + // // This is very vaguely taken from + // // https://docs.microsoft.com/en-us/windows/win32/secauthz/creating-a-security-descriptor-for-a-new-object-in-c-- + // // With using https://docs.microsoft.com/en-us/windows/win32/secauthz/well-known-sids + // // to find out that + // // * SECURITY_NT_AUTHORITY+SECURITY_LOCAL_SYSTEM_RID == NT AUTHORITY\SYSTEM + // // * SECURITY_NT_AUTHORITY+SECURITY_BUILTIN_DOMAIN_RID+DOMAIN_ALIAS_RID_ADMINS == BUILTIN\Administrators + // // * SECURITY_WORLD_SID_AUTHORITY+SECURITY_WORLD_RID == Everyone + // // + // // Raymond Chen recommended that I make this file only writable by + // // SYSTEM, but if I did that, then even we can't write the file + // // while elevated, which isn't what we want. + + // // Create a SID for the BUILTIN\Administrators group. + // 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. + // const auto everyoneSid = wil::make_static_sid(SECURITY_WORLD_SID_AUTHORITY, SECURITY_WORLD_RID); + + // EXPLICIT_ACCESS ea[2]{}; + + // // Grant Admins all permissions on this file + // ea[0].grfAccessPermissions = GENERIC_ALL; + // ea[0].grfAccessMode = SET_ACCESS; + // ea[0].grfInheritance = NO_INHERITANCE; + // ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID; + // ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; + // ea[0].Trustee.ptstrName = (LPWSTR)(adminGroupSid.get()); + + // // Grant Everyone the permission or read this file + // ea[1].grfAccessPermissions = GENERIC_READ; + // ea[1].grfAccessMode = SET_ACCESS; + // ea[1].grfInheritance = NO_INHERITANCE; + // ea[1].Trustee.TrusteeForm = TRUSTEE_IS_SID; + // ea[1].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; + // ea[1].Trustee.ptstrName = (LPWSTR)(everyoneSid.get()); + + // ACL acl; + // PACL pAcl = &acl; + // THROW_IF_WIN32_ERROR(SetEntriesInAcl(2, ea, nullptr, &pAcl)); + + // SECURITY_DESCRIPTOR sd; + // THROW_IF_WIN32_BOOL_FALSE(InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)); + // THROW_IF_WIN32_BOOL_FALSE(SetSecurityDescriptorDacl(&sd, true, pAcl, false)); + + // TODO!: I need to LocalFree this somehow. + PSECURITY_DESCRIPTOR psd; + unsigned long cb; + ConvertStringSecurityDescriptorToSecurityDescriptor(L"S:(ML;;NW;;;HI)", + SDDL_REVISION_1, + &psd, + &cb); // Initialize a security attributes structure. sa.nLength = sizeof(SECURITY_ATTRIBUTES); - sa.lpSecurityDescriptor = &sd; + // sa.lpSecurityDescriptor = &sd; + sa.lpSecurityDescriptor = psd; sa.bInheritHandle = false; } From 25b2675d8d803d790b9ed132677cab236c527838 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 9 Nov 2021 06:24:16 -0600 Subject: [PATCH 23/30] this works really quite well --- .../TerminalSettingsModel/FileUtils.cpp | 50 +++++++++++++++++-- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index 608261c1d3b..09b84859863 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -56,7 +56,45 @@ namespace winrt::Microsoft::Terminal::Settings::Model static bool _hasElevatedOnlyPermissions(const std::filesystem::path& path) { path; - return true; + wil::unique_sid sidOwner; + + PSID psidOwner{ nullptr }; + // wil::unique_hlocal_security_descriptor sd; + PSECURITY_DESCRIPTOR pSD{ nullptr }; // TODO! LocalFree me + auto status = GetNamedSecurityInfoW(path.c_str(), + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION, + &psidOwner, + nullptr, + nullptr, + nullptr, + // wil::out_param_ptr(sd)); + &pSD); + THROW_IF_WIN32_ERROR(status); + /*if (status == ERROR_FILE_NOT_FOUND) + { + return true; + } + if (LOG_IF_WIN32_ERROR(status)) + { + return false; + }*/ + + const auto adminGroupSid = wil::make_static_sid(SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS); + adminGroupSid; // for debugging; + + wil::unique_any_psid psidAdmins{ nullptr }; + ConvertStringSidToSidW(L"BA", wil::out_param_ptr(psidAdmins)); + // Compare the owner SID to the administrators SID via + // EqualSid(psidOwner, psidAdmins). This does a low-level memory + // comparison of the SIDs. + return EqualSid(psidOwner, psidAdmins.get()); + + // The psidOwner pointer references the security descriptor, so it + // doesn't have to be freed separate from pSD. + // LocalFree(pSD); // called when the unique_hlocal_security_descriptor exits scope + + // return true; /* // If we want to only open the file if it's elevated, check the // permissions on this file. We want to make sure that: @@ -202,6 +240,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model const bool elevatedOnly) { SECURITY_ATTRIBUTES sa; + // wil::unique_hlocal_security_descriptor sd; if (elevatedOnly) { // // This is very vaguely taken from @@ -248,18 +287,19 @@ namespace winrt::Microsoft::Terminal::Settings::Model // THROW_IF_WIN32_BOOL_FALSE(InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)); // THROW_IF_WIN32_BOOL_FALSE(SetSecurityDescriptorDacl(&sd, true, pAcl, false)); - // TODO!: I need to LocalFree this somehow. - PSECURITY_DESCRIPTOR psd; unsigned long cb; + PSECURITY_DESCRIPTOR pSD{ nullptr }; ConvertStringSecurityDescriptorToSecurityDescriptor(L"S:(ML;;NW;;;HI)", SDDL_REVISION_1, - &psd, + // wil::out_param_ptr(sd), + &pSD, &cb); // Initialize a security attributes structure. sa.nLength = sizeof(SECURITY_ATTRIBUTES); // sa.lpSecurityDescriptor = &sd; - sa.lpSecurityDescriptor = psd; + // sa.lpSecurityDescriptor = sd.addressof(); + sa.lpSecurityDescriptor = pSD; sa.bInheritHandle = false; } From b21287140dc8368e3b21ff56f66bdfd471afa5d6 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 9 Nov 2021 07:47:12 -0600 Subject: [PATCH 24/30] this is the right way to initialize the unique_hlocal_security_descriptor --- src/cascadia/TerminalSettingsModel/FileUtils.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index 09b84859863..06998daaabf 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -240,7 +240,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model const bool elevatedOnly) { SECURITY_ATTRIBUTES sa; - // wil::unique_hlocal_security_descriptor sd; + wil::unique_hlocal_security_descriptor sd; if (elevatedOnly) { // // This is very vaguely taken from @@ -288,18 +288,20 @@ namespace winrt::Microsoft::Terminal::Settings::Model // THROW_IF_WIN32_BOOL_FALSE(SetSecurityDescriptorDacl(&sd, true, pAcl, false)); unsigned long cb; - PSECURITY_DESCRIPTOR pSD{ nullptr }; + //PSECURITY_DESCRIPTOR pSD{ nullptr }; + ConvertStringSecurityDescriptorToSecurityDescriptor(L"S:(ML;;NW;;;HI)", SDDL_REVISION_1, - // wil::out_param_ptr(sd), - &pSD, + wil::out_param_ptr(sd), + // &pSD, + // sd.put(), &cb); // Initialize a security attributes structure. sa.nLength = sizeof(SECURITY_ATTRIBUTES); // sa.lpSecurityDescriptor = &sd; - // sa.lpSecurityDescriptor = sd.addressof(); - sa.lpSecurityDescriptor = pSD; + sa.lpSecurityDescriptor = sd.get(); + // sa.lpSecurityDescriptor = pSD; sa.bInheritHandle = false; } From 4f16dfb5fdb923c09fd997516cb5fa052f346132 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 9 Nov 2021 08:01:47 -0600 Subject: [PATCH 25/30] many comments --- .../TerminalSettingsModel/FileUtils.cpp | 97 ++++++++----------- 1 file changed, 43 insertions(+), 54 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index 06998daaabf..c336a416fed 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -45,10 +45,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model // 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. + // 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: // - path: the path to the file to check // Return Value: @@ -240,69 +239,59 @@ namespace winrt::Microsoft::Terminal::Settings::Model const bool elevatedOnly) { 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) { - // // This is very vaguely taken from - // // https://docs.microsoft.com/en-us/windows/win32/secauthz/creating-a-security-descriptor-for-a-new-object-in-c-- - // // With using https://docs.microsoft.com/en-us/windows/win32/secauthz/well-known-sids - // // to find out that - // // * SECURITY_NT_AUTHORITY+SECURITY_LOCAL_SYSTEM_RID == NT AUTHORITY\SYSTEM - // // * SECURITY_NT_AUTHORITY+SECURITY_BUILTIN_DOMAIN_RID+DOMAIN_ALIAS_RID_ADMINS == BUILTIN\Administrators - // // * SECURITY_WORLD_SID_AUTHORITY+SECURITY_WORLD_RID == Everyone - // // - // // Raymond Chen recommended that I make this file only writable by - // // SYSTEM, but if I did that, then even we can't write the file - // // while elevated, which isn't what we want. - - // // Create a SID for the BUILTIN\Administrators group. - // 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. - // const auto everyoneSid = wil::make_static_sid(SECURITY_WORLD_SID_AUTHORITY, SECURITY_WORLD_RID); - - // EXPLICIT_ACCESS ea[2]{}; - - // // Grant Admins all permissions on this file - // ea[0].grfAccessPermissions = GENERIC_ALL; - // ea[0].grfAccessMode = SET_ACCESS; - // ea[0].grfInheritance = NO_INHERITANCE; - // ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID; - // ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; - // ea[0].Trustee.ptstrName = (LPWSTR)(adminGroupSid.get()); - - // // Grant Everyone the permission or read this file - // ea[1].grfAccessPermissions = GENERIC_READ; - // ea[1].grfAccessMode = SET_ACCESS; - // ea[1].grfInheritance = NO_INHERITANCE; - // ea[1].Trustee.TrusteeForm = TRUSTEE_IS_SID; - // ea[1].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; - // ea[1].Trustee.ptstrName = (LPWSTR)(everyoneSid.get()); - - // ACL acl; - // PACL pAcl = &acl; - // THROW_IF_WIN32_ERROR(SetEntriesInAcl(2, ea, nullptr, &pAcl)); - - // SECURITY_DESCRIPTOR sd; - // THROW_IF_WIN32_BOOL_FALSE(InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)); - // THROW_IF_WIN32_BOOL_FALSE(SetSecurityDescriptorDacl(&sd, true, pAcl, false)); - + // 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 + // https://github.com/microsoft/terminal/pull/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; - //PSECURITY_DESCRIPTOR pSD{ nullptr }; - ConvertStringSecurityDescriptorToSecurityDescriptor(L"S:(ML;;NW;;;HI)", SDDL_REVISION_1, wil::out_param_ptr(sd), - // &pSD, - // sd.put(), &cb); // Initialize a security attributes structure. sa.nLength = sizeof(SECURITY_ATTRIBUTES); - // sa.lpSecurityDescriptor = &sd; sa.lpSecurityDescriptor = sd.get(); - // sa.lpSecurityDescriptor = pSD; 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 + // _hasElevatedOnlyPermissions 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(), From a93d17ef095e22a923f14ed0492a6f97fe8d2727 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 9 Nov 2021 08:24:18 -0600 Subject: [PATCH 26/30] I believe this is the rest of the comments --- .../TerminalSettingsModel/FileUtils.cpp | 125 ++++-------------- 1 file changed, 24 insertions(+), 101 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index c336a416fed..8bd6613dc24 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -54,106 +54,32 @@ namespace winrt::Microsoft::Terminal::Settings::Model // - true if it had the expected permissions. False otherwise. static bool _hasElevatedOnlyPermissions(const std::filesystem::path& path) { - path; - wil::unique_sid sidOwner; + // 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 }; - // wil::unique_hlocal_security_descriptor sd; - PSECURITY_DESCRIPTOR pSD{ nullptr }; // TODO! LocalFree me - auto status = GetNamedSecurityInfoW(path.c_str(), - SE_FILE_OBJECT, - OWNER_SECURITY_INFORMATION, - &psidOwner, - nullptr, - nullptr, - nullptr, - // wil::out_param_ptr(sd)); - &pSD); + // The psidOwner pointer references the security descriptor, so it + // doesn't have to be freed separate from sd. + const auto status = GetNamedSecurityInfoW(path.c_str(), + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION, + &psidOwner, + nullptr, + nullptr, + nullptr, + wil::out_param_ptr(sd)); THROW_IF_WIN32_ERROR(status); - /*if (status == ERROR_FILE_NOT_FOUND) - { - return true; - } - if (LOG_IF_WIN32_ERROR(status)) - { - return false; - }*/ - - const auto adminGroupSid = wil::make_static_sid(SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS); - adminGroupSid; // for debugging; wil::unique_any_psid psidAdmins{ nullptr }; - ConvertStringSidToSidW(L"BA", wil::out_param_ptr(psidAdmins)); + THROW_IF_WIN32_BOOL_FALSE( + ConvertStringSidToSidW(L"BA", wil::out_param_ptr(psidAdmins))); + // Compare the owner SID to the administrators SID via // EqualSid(psidOwner, psidAdmins). This does a low-level memory // comparison of the SIDs. return EqualSid(psidOwner, psidAdmins.get()); - - // The psidOwner pointer references the security descriptor, so it - // doesn't have to be freed separate from pSD. - // LocalFree(pSD); // called when the unique_hlocal_security_descriptor exits scope - - // return true; - /* - // If we want to only open the file if it's elevated, check the - // permissions on this file. We want to make sure that: - // * Everyone has permission to read - // * admins can do anything - // * no one else can do anything. - PACL pAcl{ nullptr }; // This doesn't need to be cleanup up apparently - - auto status = GetNamedSecurityInfo(path.c_str(), - SE_FILE_OBJECT, - DACL_SECURITY_INFORMATION, - nullptr, - nullptr, - &pAcl, - nullptr, - nullptr); - THROW_IF_WIN32_ERROR(status); - - PEXPLICIT_ACCESS pEA{ nullptr }; - DWORD count = 0; - status = GetExplicitEntriesFromAcl(pAcl, &count, &pEA); - THROW_IF_WIN32_ERROR(status); - - auto explicitAccessCleanup = wil::scope_exit([&]() { ::LocalFree(pEA); }); - - if (count != 2) - { - return false; - } - - // Now, get the Everyone and Admins SIDS so we can make sure they're - // the ones in this file. - // Create a SID for the BUILTIN\Administrators group. - 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. - const auto everyoneSid = wil::make_static_sid(SECURITY_WORLD_SID_AUTHORITY, SECURITY_WORLD_RID); - - bool hadExpectedPermissions = true; - - // Check that the permissions are what we'd expect them to be if only - // admins can write to the file. This is basically a mirror of what we - // set up in `WriteUTF8File`. - - // For grfAccessPermissions, GENERIC_ALL turns into STANDARD_RIGHTS_ALL, - // and GENERIC_READ -> READ_CONTROL - hadExpectedPermissions &= WI_AreAllFlagsSet(pEA[0].grfAccessPermissions, STANDARD_RIGHTS_ALL); - hadExpectedPermissions &= pEA[0].grfInheritance == NO_INHERITANCE; - hadExpectedPermissions &= pEA[0].Trustee.TrusteeForm == TRUSTEE_IS_SID; - // SIDs are void*'s that happen to convert to a wchar_t - hadExpectedPermissions &= *(pEA[0].Trustee.ptstrName) == *(LPWSTR)(adminGroupSid.get()); - - // Now check the other EXPLICIT_ACCESS - hadExpectedPermissions &= WI_IsFlagSet(pEA[1].grfAccessPermissions, READ_CONTROL); - hadExpectedPermissions &= pEA[1].grfInheritance == NO_INHERITANCE; - hadExpectedPermissions &= pEA[1].Trustee.TrusteeForm == TRUSTEE_IS_SID; - hadExpectedPermissions &= *(pEA[1].Trustee.ptstrName) == *(LPWSTR)(everyoneSid.get()); - - return hadExpectedPermissions; - */ } // Tries to read a file somewhat atomically without locking it. // Strips the UTF8 BOM if it exists. @@ -260,21 +186,18 @@ namespace winrt::Microsoft::Terminal::Settings::Model // on this file such that Builtin\Admins could read&write the file, // and all users could only read. // - // Big thanks to @eryksun in - // https://github.com/microsoft/terminal/pull/11222 for helping with - // this. - // - // This alternative method was chosen because it's considerably - // simpler. + // 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; - ConvertStringSecurityDescriptorToSecurityDescriptor(L"S:(ML;;NW;;;HI)", - SDDL_REVISION_1, - wil::out_param_ptr(sd), - &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); From db9cbf3fa84fe53f908eeea3ceb2a375619536db Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 10 Nov 2021 10:16:31 -0600 Subject: [PATCH 27/30] spell --- .github/actions/spelling/allow/apis.txt | 1 + .github/actions/spelling/allow/names.txt | 1 + src/cascadia/TerminalApp/AppLogic.cpp | 2 +- src/cascadia/TerminalSettingsModel/FileUtils.cpp | 3 --- 4 files changed, 3 insertions(+), 4 deletions(-) diff --git a/.github/actions/spelling/allow/apis.txt b/.github/actions/spelling/allow/apis.txt index 828b3ae3c0e..bd6283aad5c 100644 --- a/.github/actions/spelling/allow/apis.txt +++ b/.github/actions/spelling/allow/apis.txt @@ -131,6 +131,7 @@ 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 3635d37234e..2a13d67badd 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 69725187921..ebc92d5058a 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -898,7 +898,7 @@ namespace winrt::TerminalApp::implementation // 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 + // 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. // diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index 8bd6613dc24..86eccdb269f 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -76,9 +76,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model THROW_IF_WIN32_BOOL_FALSE( ConvertStringSidToSidW(L"BA", wil::out_param_ptr(psidAdmins))); - // Compare the owner SID to the administrators SID via - // EqualSid(psidOwner, psidAdmins). This does a low-level memory - // comparison of the SIDs. return EqualSid(psidOwner, psidAdmins.get()); } // Tries to read a file somewhat atomically without locking it. From 08cbd16d47b70804fffc3bf637bd99bc4ef72dd8 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 10 Nov 2021 10:17:41 -0600 Subject: [PATCH 28/30] the last of it? --- src/cascadia/TerminalSettingsModel/FileUtils.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index 86eccdb269f..94a3a2f143b 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -52,7 +52,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model // - 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) + static bool _isOwnedByAdministrators(const std::filesystem::path& path) { // If the file is owned by the administrators group, trust the // administrators instead of checking the DACL permissions. It's simpler @@ -84,7 +84,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model { if (elevatedOnly) { - const bool hadExpectedPermissions{ _hasElevatedOnlyPermissions(path) }; + const bool hadExpectedPermissions{ _isOwnedByAdministrators(path) }; if (!hadExpectedPermissions) { // delete the file. It's been compromised. @@ -204,7 +204,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model // 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 - // _hasElevatedOnlyPermissions check. + // _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 @@ -216,7 +216,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model wil::unique_hfile file{ CreateFileW(path.c_str(), GENERIC_WRITE, - FILE_SHARE_READ | FILE_SHARE_WRITE, + FILE_SHARE_READ | FILE_SHARE_DELETE, elevatedOnly ? &sa : nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, From 33e96e7e6686a060af75f7fc6de02de5b6d096ca Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 11 Nov 2021 12:56:05 -0600 Subject: [PATCH 29/30] mitigate a TOCTOU --- .../TerminalSettingsModel/FileUtils.cpp | 65 ++++++++++++------- 1 file changed, 42 insertions(+), 23 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/FileUtils.cpp b/src/cascadia/TerminalSettingsModel/FileUtils.cpp index 94a3a2f143b..4535c23be06 100644 --- a/src/cascadia/TerminalSettingsModel/FileUtils.cpp +++ b/src/cascadia/TerminalSettingsModel/FileUtils.cpp @@ -49,10 +49,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model // by the Builtin\Administrators group. If it's not, then it was likely // tampered with. // Arguments: - // - path: the path to the file to check + // - handle: a HANDLE to the file to check // Return Value: // - true if it had the expected permissions. False otherwise. - static bool _isOwnedByAdministrators(const std::filesystem::path& path) + static bool _isOwnedByAdministrators(const HANDLE& handle) { // If the file is owned by the administrators group, trust the // administrators instead of checking the DACL permissions. It's simpler @@ -62,14 +62,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model PSID psidOwner{ nullptr }; // The psidOwner pointer references the security descriptor, so it // doesn't have to be freed separate from sd. - const auto status = GetNamedSecurityInfoW(path.c_str(), - SE_FILE_OBJECT, - OWNER_SECURITY_INFORMATION, - &psidOwner, - nullptr, - nullptr, - nullptr, - wil::out_param_ptr(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 }; @@ -82,27 +82,46 @@ namespace winrt::Microsoft::Terminal::Settings::Model // Strips the UTF8 BOM if it exists. std::string ReadUTF8File(const std::filesystem::path& path, const bool elevatedOnly) { - if (elevatedOnly) - { - const bool hadExpectedPermissions{ _isOwnedByAdministrators(path) }; - if (!hadExpectedPermissions) - { - // 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 ""; - } - } - // From some casual observations we can determine that: // * ReadFile() always returns the requested amount of data (unless the file is smaller) // * It's unlikely that the file was changed between GetFileSize() and ReadFile() // -> 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); From 7f03d4d1ea9d75918a20e1cc724c57387a429413 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 11 Nov 2021 12:58:50 -0600 Subject: [PATCH 30/30] dustins nits --- src/cascadia/TerminalSettingsModel/ApplicationState.cpp | 1 + src/cascadia/TerminalSettingsModel/ApplicationState.h | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index 19e3da4df8c..d623ced2974 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -145,6 +145,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation try { LOG_LAST_ERROR_IF(!DeleteFile(_sharedPath.c_str())); + LOG_LAST_ERROR_IF(!DeleteFile(_elevatedPath.c_str())); *_state.lock() = {}; } CATCH_LOG() diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index bb9e06615c6..1c0943cfee2 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -86,7 +86,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation }; til::shared_mutex _state; std::filesystem::path _sharedPath; - std::filesystem::path _userPath; std::filesystem::path _elevatedPath; til::throttled_func_trailing<> _throttler;