Skip to content

Commit

Permalink
Make config1.db readable from AppContainer apps
Browse files Browse the repository at this point in the history
This attempts to avoid the deadlock issue discussed in google#1076.

Our current hypothesis to explain google#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 google#1076, we then need to take
care of other cases such as calling GetFileAttributes() against the
user profile directory.
  • Loading branch information
yukawa committed Oct 14, 2024
1 parent 1d74ac4 commit 6babeef
Show file tree
Hide file tree
Showing 18 changed files with 122 additions and 71 deletions.
4 changes: 3 additions & 1 deletion src/base/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
24 changes: 23 additions & 1 deletion src/base/config_file_stream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@
#include <string>
#include <utility>

#ifdef _WIN32
#include <windows.h>
#endif // _WIN32

#include "absl/container/flat_hash_map.h"
#include "absl/log/check.h"
#include "absl/log/log.h"
Expand All @@ -52,7 +56,7 @@
#include "base/system_util.h"

#ifdef _WIN32
#include <windows.h>
#include "win32/win_sandbox.h"
#endif // _WIN32

namespace mozc {
Expand Down Expand Up @@ -218,4 +222,22 @@ std::string ConfigFileStream::GetFileName(const absl::string_view filename) {
void ConfigFileStream::ClearOnMemoryFiles() {
Singleton<OnMemoryFileMap>::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 auto 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
4 changes: 4 additions & 0 deletions src/base/config_file_stream.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
28 changes: 21 additions & 7 deletions src/base/win32/win_sandbox.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<Sid> WinSandbox::GetSidsToDisable(HANDLE effective_token,
Expand Down Expand Up @@ -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());
Expand All @@ -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) {
Expand All @@ -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;
}
Expand Down
16 changes: 8 additions & 8 deletions src/base/win32/win_sandbox.h
Original file line number Diff line number Diff line change
Expand Up @@ -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|.
Expand Down
15 changes: 15 additions & 0 deletions src/config/config_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -309,5 +317,12 @@ Config::SessionKeymap ConfigHandler::GetDefaultKeyMap() {
}
}

#ifdef _WIN32
// static
void ConfigHandler::FixupFilePermission() {
ConfigFileStream::FixupFilePermission(GetConfigFileName());
}
#endif // _WIN32

} // namespace config
} // namespace mozc
4 changes: 4 additions & 0 deletions src/config/config_handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion src/win32/base/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
44 changes: 3 additions & 41 deletions src/win32/base/config_snapshot.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,13 @@

#include <algorithm>

#include "base/win32/win_util.h"
#include "client/client_interface.h"
#include "config/config_handler.h"
#include "protocol/config.pb.h"

namespace mozc {
namespace win32 {
namespace {

using ::mozc::client::ClientInterface;
using ::mozc::config::Config;

constexpr size_t kMaxDirectModeKeys = 128;
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
6 changes: 1 addition & 5 deletions src/win32/base/config_snapshot.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,6 @@
#include "session/key_info_util.h"

namespace mozc {
namespace client {
class ClientInterface;
} // namespace client

namespace win32 {

class ConfigSnapshot {
Expand All @@ -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
Expand Down
31 changes: 27 additions & 4 deletions src/win32/custom_action/custom_action.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions src/win32/custom_action/custom_action.def
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,6 @@ EXPORTS
InitialInstallation
InitialInstallationCommit
EnableTipProfile
FixupConfigFilePermission
WriteApValue
WriteApValueRollback
3 changes: 3 additions & 0 deletions src/win32/custom_action/custom_action.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion src/win32/installer/installer_64bit.wxs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@
<CustomAction Id="InitialInstallation" DllEntry="InitialInstallation" Execute="deferred" Impersonate="no" BinaryRef="GoogleIMEJaInstallerHelper.dll" />
<CustomAction Id="InitialInstallationCommit" DllEntry="InitialInstallationCommit" Execute="commit" Impersonate="no" BinaryRef="GoogleIMEJaInstallerHelper.dll" />
<CustomAction Id="EnableTipProfile" DllEntry="EnableTipProfile" Execute="commit" Impersonate="yes" BinaryRef="GoogleIMEJaInstallerHelper.dll" />
<CustomAction Id="FixupConfigFilePermission" DllEntry="FixupConfigFilePermission" Execute="commit" Impersonate="yes" BinaryRef="GoogleIMEJaInstallerHelper.dll" />
<CustomAction Id="HideCancelButton" DllEntry="HideCancelButton" Return="ignore" BinaryRef="GoogleIMEJaInstallerHelper.dll" />
<CustomAction Id="SaveCustomActionData" DllEntry="SaveCustomActionData" BinaryRef="GoogleIMEJaInstallerHelper.dll" />
<CustomAction Id="RestoreServiceState" DllEntry="RestoreServiceState" Impersonate="no" Execute="deferred" Return="ignore" BinaryRef="GoogleIMEJaInstallerHelper.dll" />
Expand Down Expand Up @@ -196,7 +197,8 @@
<Custom Action="RegisterTIPRollback64" Before="RegisterTIP64" Condition="(NOT (REMOVE=&quot;ALL&quot;)) AND (VersionNT64) AND (NOT UPGRADING)" />
<Custom Action="RegisterTIP64" Before="EnsureAllApplicationPackagesPermisssions" Condition="(NOT (REMOVE=&quot;ALL&quot;)) AND (VersionNT64)" />
<Custom Action="EnsureAllApplicationPackagesPermisssions" Before="InitialInstallationCommit" Condition="(NOT (REMOVE=&quot;ALL&quot;))" />
<Custom Action="InitialInstallationCommit" Before="EnableTipProfile" Condition="(NOT (REMOVE=&quot;ALL&quot;)) AND (NOT UPGRADING)" />
<Custom Action="InitialInstallationCommit" Before="FixupConfigFilePermission" Condition="(NOT (REMOVE=&quot;ALL&quot;)) AND (NOT UPGRADING)" />
<Custom Action="FixupConfigFilePermission" Before="EnableTipProfile" Condition="NOT (REMOVE=&quot;ALL&quot;)" />
<Custom Action="EnableTipProfile" Before="InstallFinalize" Condition="(NOT (REMOVE=&quot;ALL&quot;)) AND (NOT UPGRADING)" />
<!-- show reboot dialog to execute pending file opartions -->
<?if ($(var.VSConfigurationName) = "Release") ?>
Expand Down
Loading

0 comments on commit 6babeef

Please sign in to comment.