From b3c4c49e36a8d1308253ce4d9ff361df4a71fb42 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Thu, 10 Oct 2019 18:51:10 +0000 Subject: [PATCH 1/2] bug: remove racy code to detect GCE on Windows The code used to detect if a program was running on Windows by launching a powershell script (which requires powershell to be installed), writing the output to a file with a fixed name (which requires write permissions in the local directory), and then reading the file (which may have changed), and then deleting it (which may fail and introduces a race with any other program running in the same directory). This version reads a key from the Windows registry. That could fail if the application does not have permissions to read the registry, but at least does not crash when it does, and it is not inheritently racy. --- .../alts/check_gcp_environment_windows.cc | 104 ++++++++---------- .../check_gcp_environment_windows_test.cc | 43 ++++++-- 2 files changed, 78 insertions(+), 69 deletions(-) diff --git a/src/core/lib/security/credentials/alts/check_gcp_environment_windows.cc b/src/core/lib/security/credentials/alts/check_gcp_environment_windows.cc index 55efe0e9dd6..ebbdc1803d6 100644 --- a/src/core/lib/security/credentials/alts/check_gcp_environment_windows.cc +++ b/src/core/lib/security/credentials/alts/check_gcp_environment_windows.cc @@ -31,80 +31,70 @@ #include #include -#define GRPC_ALTS_EXPECT_NAME_GOOGLE "Google" -#define GRPC_ALTS_WINDOWS_CHECK_COMMAND "powershell.exe" -#define GRPC_ALTS_WINDOWS_CHECK_COMMAND_ARGS \ - "(Get-WmiObject -Class Win32_BIOS).Manufacturer" -#define GRPC_ALTS_WINDOWS_CHECK_BIOS_FILE "windows_bios.data" - -const size_t kBiosDataBufferSize = 256; - -static bool g_compute_engine_detection_done = false; -static bool g_is_on_compute_engine = false; -static gpr_mu g_mu; -static gpr_once g_once = GPR_ONCE_INIT; - namespace grpc_core { namespace internal { -bool check_bios_data(const char* bios_data_file) { - char* bios_data = read_bios_file(bios_data_file); - bool result = !strcmp(bios_data, GRPC_ALTS_EXPECT_NAME_GOOGLE); - remove(GRPC_ALTS_WINDOWS_CHECK_BIOS_FILE); - gpr_free(bios_data); - return result; +bool check_bios_data(const char*) { + return false; } -} // namespace internal -} // namespace grpc_core +bool check_windows_registry_product_name( + HKEY root_key, const char* reg_key_path, const char* reg_key_name) { + const size_t kProductNameBufferSize = 256; + char const expected_substr[] = "Google"; -static void init_mu(void) { gpr_mu_init(&g_mu); } + // Get the size of the string first to allocate our buffer. This includes + // enough space for the trailing NUL character that will be included. + DWORD buffer_size{}; + auto rc = ::RegGetValueA( + root_key, reg_key_path, reg_key_name, + RRF_RT_REG_SZ, + nullptr, // We know the type will be REG_SZ. + nullptr, // We're only fetching the size; no buffer given yet. + &buffer_size); // Fetch the size in bytes of the value, if it exists. + if (rc != 0) { + return false; + } -static bool run_powershell() { - SECURITY_ATTRIBUTES sa; - sa.nLength = sizeof(sa); - sa.lpSecurityDescriptor = NULL; - sa.bInheritHandle = TRUE; - HANDLE h = CreateFile(_T(GRPC_ALTS_WINDOWS_CHECK_BIOS_FILE), GENERIC_WRITE, - FILE_SHARE_WRITE | FILE_SHARE_READ, &sa, OPEN_ALWAYS, - FILE_ATTRIBUTE_NORMAL, NULL); - if (h == INVALID_HANDLE_VALUE) { - gpr_log(GPR_ERROR, "CreateFile failed (%d).", GetLastError()); + if (buffer_size > kProductNameBufferSize) { return false; } - PROCESS_INFORMATION pi; - STARTUPINFO si; - DWORD flags = CREATE_NO_WINDOW; - ZeroMemory(&pi, sizeof(pi)); - ZeroMemory(&si, sizeof(si)); - si.cb = sizeof(si); - si.dwFlags |= STARTF_USESTDHANDLES; - si.hStdInput = NULL; - si.hStdError = h; - si.hStdOutput = h; - TCHAR cmd[kBiosDataBufferSize]; - _sntprintf(cmd, kBiosDataBufferSize, _T("%s %s"), - _T(GRPC_ALTS_WINDOWS_CHECK_COMMAND), - _T(GRPC_ALTS_WINDOWS_CHECK_COMMAND_ARGS)); - if (!CreateProcess(NULL, cmd, NULL, NULL, TRUE, flags, NULL, NULL, &si, - &pi)) { - gpr_log(GPR_ERROR, "CreateProcess failed (%d).\n", GetLastError()); + + // Retrieve the product name string. + char buffer[kProductNameBufferSize]; + buffer_size = kProductNameBufferSize; + rc = ::RegGetValueA( + root_key, reg_key_path, reg_key_name, + RRF_RT_REG_SZ, + nullptr, // We know the type will be REG_SZ. + static_cast(buffer), // Fetch the string value this time. + &buffer_size); // The string size in bytes, not including trailing NUL. + if (rc != 0) { return false; } - WaitForSingleObject(pi.hProcess, INFINITE); - CloseHandle(pi.hProcess); - CloseHandle(pi.hThread); - CloseHandle(h); - return true; + + return strstr(buffer, expected_substr) != nullptr; } +} // namespace internal +} // namespace grpc_core + +static bool g_compute_engine_detection_done = false; +static bool g_is_on_compute_engine = false; +static gpr_mu g_mu; +static gpr_once g_once = GPR_ONCE_INIT; + +static void init_mu(void) { gpr_mu_init(&g_mu); } + bool grpc_alts_is_running_on_gcp() { + char const reg_key_path[] = "SYSTEM\\HardwareConfig\\Current\\"; + char const reg_key_name[] = "SystemProductName"; + gpr_once_init(&g_once, init_mu); gpr_mu_lock(&g_mu); if (!g_compute_engine_detection_done) { - g_is_on_compute_engine = - run_powershell() && - grpc_core::internal::check_bios_data(GRPC_ALTS_WINDOWS_CHECK_BIOS_FILE); + g_is_on_compute_engine = grpc_core::internal::check_windows_registry_product_name( + HKEY_LOCAL_MACHINE, reg_key_path, reg_key_name); g_compute_engine_detection_done = true; } gpr_mu_unlock(&g_mu); diff --git a/test/core/security/check_gcp_environment_windows_test.cc b/test/core/security/check_gcp_environment_windows_test.cc index 2b9407e8cad..ded369f66c0 100644 --- a/test/core/security/check_gcp_environment_windows_test.cc +++ b/test/core/security/check_gcp_environment_windows_test.cc @@ -29,23 +29,42 @@ #include #include "src/core/lib/gpr/tmpfile.h" +namespace grpc_core { +namespace internal { + +bool check_windows_registry_product_name( + HKEY root_key, const char* reg_key_path, const char* reg_key_name); + +} // namespace internal +} // namespace grpc_core + static bool check_bios_data_windows_test(const char* data) { - /* Create a file with contents data. */ - char* filename = nullptr; - FILE* fp = gpr_tmpfile("check_gcp_environment_test", &filename); - GPR_ASSERT(filename != nullptr); - GPR_ASSERT(fp != nullptr); - GPR_ASSERT(fwrite(data, 1, strlen(data), fp) == strlen(data)); - fclose(fp); - bool result = grpc_core::internal::check_bios_data( - reinterpret_cast(filename)); - /* Cleanup. */ - remove(filename); - gpr_free(filename); + char const reg_key_path[] = "SYSTEM\\HardwareConfig\\Current\\"; + char const reg_key_name[] = "grpcTestValueName"; + // Modify the registry for the current user to contain the + // test value. We cannot use the system registry because the + // user may not have privileges to change it. + auto rc = RegSetKeyValueA( + HKEY_CURRENT_USER, reg_key_path, reg_key_name, REG_SZ, + reinterpret_cast(data), + static_cast(strlen(data))); + if (rc != 0) { + return false; + } + + auto result = grpc_core::internal::check_windows_registry_product_name( + HKEY_CURRENT_USER, reg_key_path, reg_key_name); + + (void)RegDeleteKeyValueA(HKEY_CURRENT_USER, reg_key_path, reg_key_name); + return result; } static void test_gcp_environment_check_success() { + // This is the only value observed in production. + GPR_ASSERT(check_bios_data_windows_test("Google Compute Engine")); + // Be generous and accept other values that were accepted by the previous + // implementation. GPR_ASSERT(check_bios_data_windows_test("Google")); GPR_ASSERT(check_bios_data_windows_test("Google\n")); GPR_ASSERT(check_bios_data_windows_test("Google\r")); From 2144c719fcf9d73a3a97f2f378fb178635328d4f Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Thu, 24 Oct 2019 13:24:33 -0400 Subject: [PATCH 2/2] Run clang_format_all_the_things.sh --- .../alts/check_gcp_environment_windows.cc | 24 +++++++++---------- .../check_gcp_environment_windows_test.cc | 16 ++++++------- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/core/lib/security/credentials/alts/check_gcp_environment_windows.cc b/src/core/lib/security/credentials/alts/check_gcp_environment_windows.cc index ebbdc1803d6..59432cff361 100644 --- a/src/core/lib/security/credentials/alts/check_gcp_environment_windows.cc +++ b/src/core/lib/security/credentials/alts/check_gcp_environment_windows.cc @@ -34,12 +34,11 @@ namespace grpc_core { namespace internal { -bool check_bios_data(const char*) { - return false; -} +bool check_bios_data(const char*) { return false; } -bool check_windows_registry_product_name( - HKEY root_key, const char* reg_key_path, const char* reg_key_name) { +bool check_windows_registry_product_name(HKEY root_key, + const char* reg_key_path, + const char* reg_key_name) { const size_t kProductNameBufferSize = 256; char const expected_substr[] = "Google"; @@ -47,8 +46,7 @@ bool check_windows_registry_product_name( // enough space for the trailing NUL character that will be included. DWORD buffer_size{}; auto rc = ::RegGetValueA( - root_key, reg_key_path, reg_key_name, - RRF_RT_REG_SZ, + root_key, reg_key_path, reg_key_name, RRF_RT_REG_SZ, nullptr, // We know the type will be REG_SZ. nullptr, // We're only fetching the size; no buffer given yet. &buffer_size); // Fetch the size in bytes of the value, if it exists. @@ -64,10 +62,9 @@ bool check_windows_registry_product_name( char buffer[kProductNameBufferSize]; buffer_size = kProductNameBufferSize; rc = ::RegGetValueA( - root_key, reg_key_path, reg_key_name, - RRF_RT_REG_SZ, - nullptr, // We know the type will be REG_SZ. - static_cast(buffer), // Fetch the string value this time. + root_key, reg_key_path, reg_key_name, RRF_RT_REG_SZ, + nullptr, // We know the type will be REG_SZ. + static_cast(buffer), // Fetch the string value this time. &buffer_size); // The string size in bytes, not including trailing NUL. if (rc != 0) { return false; @@ -93,8 +90,9 @@ bool grpc_alts_is_running_on_gcp() { gpr_once_init(&g_once, init_mu); gpr_mu_lock(&g_mu); if (!g_compute_engine_detection_done) { - g_is_on_compute_engine = grpc_core::internal::check_windows_registry_product_name( - HKEY_LOCAL_MACHINE, reg_key_path, reg_key_name); + g_is_on_compute_engine = + grpc_core::internal::check_windows_registry_product_name( + HKEY_LOCAL_MACHINE, reg_key_path, reg_key_name); g_compute_engine_detection_done = true; } gpr_mu_unlock(&g_mu); diff --git a/test/core/security/check_gcp_environment_windows_test.cc b/test/core/security/check_gcp_environment_windows_test.cc index ded369f66c0..320735e904b 100644 --- a/test/core/security/check_gcp_environment_windows_test.cc +++ b/test/core/security/check_gcp_environment_windows_test.cc @@ -32,8 +32,9 @@ namespace grpc_core { namespace internal { -bool check_windows_registry_product_name( - HKEY root_key, const char* reg_key_path, const char* reg_key_name); +bool check_windows_registry_product_name(HKEY root_key, + const char* reg_key_path, + const char* reg_key_name); } // namespace internal } // namespace grpc_core @@ -44,17 +45,16 @@ static bool check_bios_data_windows_test(const char* data) { // Modify the registry for the current user to contain the // test value. We cannot use the system registry because the // user may not have privileges to change it. - auto rc = RegSetKeyValueA( - HKEY_CURRENT_USER, reg_key_path, reg_key_name, REG_SZ, - reinterpret_cast(data), - static_cast(strlen(data))); + auto rc = RegSetKeyValueA(HKEY_CURRENT_USER, reg_key_path, reg_key_name, + REG_SZ, reinterpret_cast(data), + static_cast(strlen(data))); if (rc != 0) { return false; } auto result = grpc_core::internal::check_windows_registry_product_name( - HKEY_CURRENT_USER, reg_key_path, reg_key_name); - + HKEY_CURRENT_USER, reg_key_path, reg_key_name); + (void)RegDeleteKeyValueA(HKEY_CURRENT_USER, reg_key_path, reg_key_name); return result;