From 353599a0a1015887c400ad3c0fb0198d978729c6 Mon Sep 17 00:00:00 2001 From: Yohei Yukawa Date: Mon, 14 Oct 2024 09:47:52 -0700 Subject: [PATCH] Make `config1.db` readable from AppContainer apps This attempts to avoid the deadlock issue discussed in #1076. Our current hypothesis to explain #1076 is that "broadFileSystemAccess" capability given to SearchHost.exe is playing an interesting role. When TIP DLL calls CreateFileW API to open "config1.db", the process itself does not have sufficient permission to open it. Then "broadFileSystemAccess" capability will take place and Windows.Storage.OneCore.dll will attempts brokered file access after initializing the thread with RoInitialize() when not yet initialized. If this happens in a certain situation, TSF runtime gets confused and may start re-initializing Mozc TIP. 1. TSF calls back into Mozc TIP 2. Mozc TIP calls CreateFileW() 3. The system invokes RoInitialize() before returning from CreateFileW() 4. TSF calls back into Mozc TIP before returning from RoInitialize() 5. Now Mozc TIP is handling a reentrant callback. Given that whether the target app has "broadFileSystemAccess" capability or not is completely out of Mozc TIP's control, the safest option would be to ensure that the target file/dir is always accessible to AppContainer processes. As the first step, this commit makes 'config1.db' readable from AppContainer processes. There are two cases when the file permission of 'config1.db' is updated. A. When the installer is installing a new version of Mozc. B. When 'config1.db' is updated by mozc_server or mozc_tool. If this commit is not sufficient to address #1076, we then need to take care of other cases such as calling GetFileAttributes() against the user profile directory. --- src/base/BUILD.bazel | 4 +- src/base/config_file_stream.cc | 24 ++++++++++- src/base/config_file_stream.h | 4 ++ src/base/win32/win_sandbox.cc | 28 +++++++++---- src/base/win32/win_sandbox.h | 16 ++++---- src/config/config_handler.cc | 15 +++++++ src/config/config_handler.h | 4 ++ src/win32/base/BUILD.bazel | 1 - src/win32/base/config_snapshot.cc | 44 ++------------------- src/win32/base/config_snapshot.h | 6 +-- src/win32/custom_action/custom_action.cc | 31 +++++++++++++-- src/win32/custom_action/custom_action.def | 1 + src/win32/custom_action/custom_action.h | 3 ++ src/win32/installer/installer_64bit.wxs | 4 +- src/win32/installer/installer_oss_64bit.wxs | 4 +- src/win32/tip/BUILD.bazel | 1 + src/win32/tip/tip_private_context.cc | 2 +- src/win32/tip/tip_private_context.h | 1 + 18 files changed, 122 insertions(+), 71 deletions(-) diff --git a/src/base/BUILD.bazel b/src/base/BUILD.bazel index 2d71de3db..cc25b3798 100644 --- a/src/base/BUILD.bazel +++ b/src/base/BUILD.bazel @@ -738,7 +738,9 @@ mozc_cc_library( "@com_google_absl//absl/log:check", "@com_google_absl//absl/status", "@com_google_absl//absl/strings", - ], + ] + mozc_select( + windows = ["//base/win32:win_sandbox"], + ), ) mozc_cc_test( diff --git a/src/base/config_file_stream.cc b/src/base/config_file_stream.cc index 3b507a3f4..6e98a965c 100644 --- a/src/base/config_file_stream.cc +++ b/src/base/config_file_stream.cc @@ -38,6 +38,10 @@ #include #include +#ifdef _WIN32 +#include +#endif // _WIN32 + #include "absl/container/flat_hash_map.h" #include "absl/log/check.h" #include "absl/log/log.h" @@ -52,7 +56,7 @@ #include "base/system_util.h" #ifdef _WIN32 -#include +#include "win32/win_sandbox.h" #endif // _WIN32 namespace mozc { @@ -218,4 +222,22 @@ std::string ConfigFileStream::GetFileName(const absl::string_view filename) { void ConfigFileStream::ClearOnMemoryFiles() { Singleton::get()->clear(); } + +#ifdef _WIN32 +// Check the file permission of "config1.db" if exists to ensure that +// "ALL APPLICATION PACKAGES" have read access to it. +void ConfigFileStream::FixupFilePermission(absl::string_view filename) { + const std::string path = ConfigFileStream::GetFileName(filename); + if (path.empty()) { + return; + } + const absl::Status status = FileUtil::FileExists(path); + if (status.ok()) { + WinSandbox::EnsureAllApplicationPackagesPermisssion( + win32::Utf8ToWide(path), + WinSandbox::AppContainerVisibilityType::kConfigFile); + } +} +#endif // _WIN32 + } // namespace mozc diff --git a/src/base/config_file_stream.h b/src/base/config_file_stream.h index 75b771016..daecc64e0 100644 --- a/src/base/config_file_stream.h +++ b/src/base/config_file_stream.h @@ -107,6 +107,10 @@ class ConfigFileStream { // Clear all memory:// files. This is a utility method for testing. static void ClearOnMemoryFiles(); +#ifdef _WIN32 + static void FixupFilePermission(absl::string_view filename); +#endif // _WIN32 + private: // This function is deprecated. Use OpenReadText or OpenReadBinary instead. // TODO(yukawa): Move this function to anonymous namespace in diff --git a/src/base/win32/win_sandbox.cc b/src/base/win32/win_sandbox.cc index c8109399b..6d1746a99 100644 --- a/src/base/win32/win_sandbox.cc +++ b/src/base/win32/win_sandbox.cc @@ -1015,6 +1015,23 @@ bool SetTokenIntegrityLevel(HANDLE token, return (result != FALSE); } +constexpr ACCESS_MASK GetAccessMask( + WinSandbox::AppContainerVisibilityType type) { + const ACCESS_MASK kBaseType = + FILE_READ_DATA | FILE_READ_EA | READ_CONTROL | SYNCHRONIZE; + + switch (type) { + case WinSandbox::AppContainerVisibilityType::kProgramFiles: + // As of Windows 10 Anniversary Update, following access masks (==0x1200a9) + // are specified to files under Program Files by default. + return FILE_EXECUTE | kBaseType; + case WinSandbox::AppContainerVisibilityType::kConfigFile: + return FILE_GENERIC_READ | FILE_READ_ATTRIBUTES | kBaseType; + default: + return kBaseType; + } +} + } // namespace std::vector WinSandbox::GetSidsToDisable(HANDLE effective_token, @@ -1232,7 +1249,7 @@ wil::unique_process_handle WinSandbox::GetRestrictedTokenHandleForImpersonation( } bool WinSandbox::EnsureAllApplicationPackagesPermisssion( - zwstring_view file_name) { + zwstring_view file_name, AppContainerVisibilityType type) { // Get "All Application Packages" group SID. const ATL::CSid all_application_packages( Sid(WinBuiltinAnyPackageSid).GetPSID()); @@ -1243,10 +1260,7 @@ bool WinSandbox::EnsureAllApplicationPackagesPermisssion( return false; } - // As of Windows 10 Anniversary Update, following access masks (==0x1200a9) - // are specified to files under Program Files by default. - const ACCESS_MASK kDesiredMask = - FILE_READ_DATA | FILE_READ_EA | FILE_EXECUTE | READ_CONTROL | SYNCHRONIZE; + const ACCESS_MASK desired_mask = GetAccessMask(type); // Check if the desired ACE is already specified or not. for (UINT i = 0; i < dacl.GetAceCount(); ++i) { @@ -1256,14 +1270,14 @@ bool WinSandbox::EnsureAllApplicationPackagesPermisssion( dacl.GetAclEntry(i, &ace_sid, &access_mask, &ace_type); if (ace_sid == all_application_packages && ace_type == ACCESS_ALLOWED_ACE_TYPE && - (access_mask & kDesiredMask) == kDesiredMask) { + (access_mask & desired_mask) == desired_mask) { // This is the desired ACE. There is nothing to do. return true; } } // We are here because there is no desired ACE. Hence we do add it. - if (!dacl.AddAllowedAce(all_application_packages, kDesiredMask, + if (!dacl.AddAllowedAce(all_application_packages, desired_mask, ACCESS_ALLOWED_ACE_TYPE)) { return false; } diff --git a/src/base/win32/win_sandbox.h b/src/base/win32/win_sandbox.h index ca17aeb58..ec9bba950 100644 --- a/src/base/win32/win_sandbox.h +++ b/src/base/win32/win_sandbox.h @@ -185,15 +185,15 @@ class WinSandbox { HANDLE effective_token, TokenLevel security_level, IntegrityLevel integrity_level); + enum class AppContainerVisibilityType { + kProgramFiles = 0, + kConfigFile = 1, + }; + // Returns true |file_name| already has or is updated to have an ACE - // (Access Control Entry) for "All Application Packages" group to have the - // following access masks: - // - FILE_READ_DATA - // - FILE_READ_EA - // - FILE_EXECUTE - // - READ_CONTROL - // - SYNCHRONIZE - static bool EnsureAllApplicationPackagesPermisssion(zwstring_view file_name); + // (Access Control Entry) for "All Application Packages" group. + static bool EnsureAllApplicationPackagesPermisssion( + zwstring_view file_name, AppContainerVisibilityType type); protected: // Returns SDDL for given |shareble_object_type|. diff --git a/src/config/config_handler.cc b/src/config/config_handler.cc index c8dc6e346..9b67a5220 100644 --- a/src/config/config_handler.cc +++ b/src/config/config_handler.cc @@ -53,6 +53,11 @@ #include "base/vlog.h" #include "protocol/config.pb.h" +#ifdef _WIN32 +#include "base/win32/wide_char.h" +#include "base/win32/win_sandbox.h" +#endif // _WIN32 + namespace mozc { namespace config { namespace { @@ -179,6 +184,9 @@ void ConfigHandlerImpl::SetConfig(const Config &config) { MOZC_VLOG(1) << "Setting new config: " << filename_; ConfigFileStream::AtomicUpdate(filename_, output_config.SerializeAsString()); +#ifdef _WIN32 + ConfigFileStream::FixupFilePermission(filename_); +#endif // _WIN32 #ifdef DEBUG std::string debug_content = absl::StrCat( @@ -309,5 +317,12 @@ Config::SessionKeymap ConfigHandler::GetDefaultKeyMap() { } } +#ifdef _WIN32 +// static +void ConfigHandler::FixupFilePermission() { + ConfigFileStream::FixupFilePermission(GetConfigFileName()); +} +#endif // _WIN32 + } // namespace config } // namespace mozc diff --git a/src/config/config_handler.h b/src/config/config_handler.h index 304670ead..6a47ece03 100644 --- a/src/config/config_handler.h +++ b/src/config/config_handler.h @@ -86,6 +86,10 @@ class ConfigHandler { // Get default keymap for each platform. static Config::SessionKeymap GetDefaultKeyMap(); + +#ifdef _WIN32 + static void FixupFilePermission(); +#endif // _WIN32 }; } // namespace config diff --git a/src/win32/base/BUILD.bazel b/src/win32/base/BUILD.bazel index 7cefdb7d0..9890d891c 100644 --- a/src/win32/base/BUILD.bazel +++ b/src/win32/base/BUILD.bazel @@ -53,7 +53,6 @@ mozc_cc_library( target_compatible_with = ["@platforms//os:windows"], deps = [ "//base:system_util", - "//client:client_interface", "//config:config_handler", "//protocol:config_cc_proto", "//session:key_info_util", diff --git a/src/win32/base/config_snapshot.cc b/src/win32/base/config_snapshot.cc index 1676a5398..ae882574b 100644 --- a/src/win32/base/config_snapshot.cc +++ b/src/win32/base/config_snapshot.cc @@ -31,8 +31,6 @@ #include -#include "base/win32/win_util.h" -#include "client/client_interface.h" #include "config/config_handler.h" #include "protocol/config.pb.h" @@ -40,7 +38,6 @@ namespace mozc { namespace win32 { namespace { -using ::mozc::client::ClientInterface; using ::mozc::config::Config; constexpr size_t kMaxDirectModeKeys = 128; @@ -53,25 +50,7 @@ struct StaticConfigSnapshot { KeyInformation direct_mode_keys[kMaxDirectModeKeys]; }; -bool GetConfigSnapshotForSandboxedProcess(ClientInterface *client, - Config *config) { - if (client == nullptr) { - return false; - } - // config1.db is likely to be inaccessible from sandboxed processes. - // So retrieve the config data from the server process. - // CAVEATS: ConfigHandler::GetConfig always returns true even when it fails - // to load the config file due to the sandbox. b/10449414 - if (!client->CheckVersionOrRestartServer()) { - return false; - } - if (!client->GetConfig(config)) { - return false; - } - return true; -} - -StaticConfigSnapshot GetConfigSnapshotForNonSandboxedProcess() { +StaticConfigSnapshot GetConfigSnapshotImpl() { Config config; // config1.db should be readable in this case. config::ConfigHandler::GetConfig(&config); @@ -102,26 +81,9 @@ ConfigSnapshot::Info::Info() use_mode_indicator(false) {} // static -bool ConfigSnapshot::Get(client::ClientInterface *client, Info *info) { - // A temporary workaround for b/24793812. Always reload config if the - // process is sandboxed. - // TODO(yukawa): Cache the result once it succeeds. - if (WinUtil::IsProcessSandboxed()) { - Config config; - if (!GetConfigSnapshotForSandboxedProcess(client, &config)) { - return false; - } - info->use_kana_input = (config.preedit_method() == Config::KANA); - info->use_keyboard_to_change_preedit_method = - config.use_keyboard_to_change_preedit_method(); - info->use_mode_indicator = config.use_mode_indicator(); - info->direct_mode_keys = KeyInfoUtil::ExtractSortedDirectModeKeys(config); - return true; - } - +bool ConfigSnapshot::Get(Info *info) { // Note: Thread-safety is not required. - static const StaticConfigSnapshot cached_snapshot = - GetConfigSnapshotForNonSandboxedProcess(); + static const StaticConfigSnapshot cached_snapshot = GetConfigSnapshotImpl(); info->use_kana_input = cached_snapshot.use_kana_input; info->use_keyboard_to_change_preedit_method = cached_snapshot.use_keyboard_to_change_preedit_method; diff --git a/src/win32/base/config_snapshot.h b/src/win32/base/config_snapshot.h index af62024ba..8c85e8596 100644 --- a/src/win32/base/config_snapshot.h +++ b/src/win32/base/config_snapshot.h @@ -35,10 +35,6 @@ #include "session/key_info_util.h" namespace mozc { -namespace client { -class ClientInterface; -} // namespace client - namespace win32 { class ConfigSnapshot { @@ -55,7 +51,7 @@ class ConfigSnapshot { ConfigSnapshot(const ConfigSnapshot &) = delete; ConfigSnapshot &operator=(const ConfigSnapshot &) = delete; - static bool Get(client::ClientInterface *client, Info *info); + static bool Get(Info *info); }; } // namespace win32 diff --git a/src/win32/custom_action/custom_action.cc b/src/win32/custom_action/custom_action.cc index 3056aaebf..55e311226 100644 --- a/src/win32/custom_action/custom_action.cc +++ b/src/win32/custom_action/custom_action.cc @@ -57,6 +57,7 @@ #include "base/win32/win_util.h" #include "client/client.h" #include "client/client_interface.h" +#include "config/config_handler.h" #include "renderer/renderer_client.h" #include "win32/base/input_dll.h" #include "win32/base/omaha_util.h" @@ -209,19 +210,23 @@ BOOL APIENTRY DllMain(HMODULE module, DWORD ul_reason_for_call, UINT __stdcall EnsureAllApplicationPackagesPermisssions(MSIHANDLE msi_handle) { DEBUG_BREAK_FOR_DEBUGGER(); if (!mozc::WinSandbox::EnsureAllApplicationPackagesPermisssion( - GetMozcComponentPath(mozc::kMozcServerName))) { + GetMozcComponentPath(mozc::kMozcServerName), + mozc::WinSandbox::AppContainerVisibilityType::kProgramFiles)) { return ERROR_INSTALL_FAILURE; } if (!mozc::WinSandbox::EnsureAllApplicationPackagesPermisssion( - GetMozcComponentPath(mozc::kMozcRenderer))) { + GetMozcComponentPath(mozc::kMozcRenderer), + mozc::WinSandbox::AppContainerVisibilityType::kProgramFiles)) { return ERROR_INSTALL_FAILURE; } if (!mozc::WinSandbox::EnsureAllApplicationPackagesPermisssion( - GetMozcComponentPath(mozc::kMozcTIP32))) { + GetMozcComponentPath(mozc::kMozcTIP32), + mozc::WinSandbox::AppContainerVisibilityType::kProgramFiles)) { return ERROR_INSTALL_FAILURE; } if (!mozc::WinSandbox::EnsureAllApplicationPackagesPermisssion( - GetMozcComponentPath(mozc::kMozcTIP64))) { + GetMozcComponentPath(mozc::kMozcTIP64), + mozc::WinSandbox::AppContainerVisibilityType::kProgramFiles)) { return ERROR_INSTALL_FAILURE; } return ERROR_SUCCESS; @@ -328,6 +333,24 @@ UINT __stdcall EnableTipProfile(MSIHANDLE msi_handle) { return ERROR_SUCCESS; } +UINT __stdcall FixupConfigFilePermission(MSIHANDLE msi_handle) { + bool is_service = false; + if (::mozc::WinUtil::IsServiceAccount(&is_service) && is_service) { + // Do nothing if this is a service account. + return ERROR_SUCCESS; + } + + // Check the file permission of "config1.db" if exists to ensure that + // "ALL APPLICATION PACKAGES" have read access to it. + // See https://github.com/google/mozc/issues/1076 for details. + ::mozc::config::ConfigHandler::FixupFilePermission(); + + // Return always ERROR_SUCCESS regardless of the result, as not being able + // to fixup the permission is not problematic enough to block installation + // and upgrading. + return ERROR_SUCCESS; +} + UINT __stdcall SaveCustomActionData(MSIHANDLE msi_handle) { DEBUG_BREAK_FOR_DEBUGGER(); // store the CHANNEL value specified in the command line argument for diff --git a/src/win32/custom_action/custom_action.def b/src/win32/custom_action/custom_action.def index fc094a7a0..7261f3982 100644 --- a/src/win32/custom_action/custom_action.def +++ b/src/win32/custom_action/custom_action.def @@ -14,5 +14,6 @@ EXPORTS InitialInstallation InitialInstallationCommit EnableTipProfile + FixupConfigFilePermission WriteApValue WriteApValueRollback diff --git a/src/win32/custom_action/custom_action.h b/src/win32/custom_action/custom_action.h index 8265dce32..486c37585 100644 --- a/src/win32/custom_action/custom_action.h +++ b/src/win32/custom_action/custom_action.h @@ -76,6 +76,9 @@ UINT __stdcall InitialInstallationCommit(MSIHANDLE msi_handle); // Enable TIP profile for the calling user unless it's a service account. UINT __stdcall EnableTipProfile(MSIHANDLE msi_handle); +// Update config1.db file access controll to make it readable to AppContainer. +UINT __stdcall FixupConfigFilePermission(MSIHANDLE msi_handle); + // Saves omaha's ap value for WriteApValue, WriteApValueRollback, and // RestoreServiceState. // Since they are executed as deferred customs actions and most properties diff --git a/src/win32/installer/installer_64bit.wxs b/src/win32/installer/installer_64bit.wxs index f2f0248f6..208b662d7 100644 --- a/src/win32/installer/installer_64bit.wxs +++ b/src/win32/installer/installer_64bit.wxs @@ -145,6 +145,7 @@ + @@ -196,7 +197,8 @@ - + + diff --git a/src/win32/installer/installer_oss_64bit.wxs b/src/win32/installer/installer_oss_64bit.wxs index fd36905b2..21cb2e832 100644 --- a/src/win32/installer/installer_oss_64bit.wxs +++ b/src/win32/installer/installer_oss_64bit.wxs @@ -126,6 +126,7 @@ + @@ -172,7 +173,8 @@ - + + diff --git a/src/win32/tip/BUILD.bazel b/src/win32/tip/BUILD.bazel index abcbc8755..752c71ce1 100644 --- a/src/win32/tip/BUILD.bazel +++ b/src/win32/tip/BUILD.bazel @@ -436,6 +436,7 @@ mozc_cc_library( tags = MOZC_TAGS.WIN_ONLY, target_compatible_with = ["@platforms//os:windows"], deps = [ + "//client:client_interface", "//protocol:commands_cc_proto", "//win32/base:config_snapshot", "//win32/base:deleter", diff --git a/src/win32/tip/tip_private_context.cc b/src/win32/tip/tip_private_context.cc index 5f33f2663..4e548be51 100644 --- a/src/win32/tip/tip_private_context.cc +++ b/src/win32/tip/tip_private_context.cc @@ -88,7 +88,7 @@ void TipPrivateContext::EnsureInitialized() { // Try to reflect the current config to the IME behavior. ConfigSnapshot::Info snapshot; - if (ConfigSnapshot::Get(state_->client_.get(), &snapshot)) { + if (ConfigSnapshot::Get(&snapshot)) { auto *behavior = &state_->input_behavior_; behavior->prefer_kana_input = snapshot.use_kana_input; behavior->use_romaji_key_to_toggle_input_style = diff --git a/src/win32/tip/tip_private_context.h b/src/win32/tip/tip_private_context.h index 9f4fa9b42..312639d5a 100644 --- a/src/win32/tip/tip_private_context.h +++ b/src/win32/tip/tip_private_context.h @@ -34,6 +34,7 @@ #include +#include "client/client_interface.h" #include "protocol/commands.pb.h" #include "win32/base/config_snapshot.h" #include "win32/base/deleter.h"