From 0328876e742667284ffaf88f51ed8841cf4ff0d6 Mon Sep 17 00:00:00 2001 From: Stefano Bonicatti Date: Thu, 27 Jul 2023 22:42:23 +0200 Subject: [PATCH] unicode: Improve Windows unicode conversion functions - Add a wstringToString overload that takes a wchar_t pointer and the maximum amount of characters to scan for a null terminator in the provided pointer/buffer, to prevent overflowing. - Add a to_bytes overload, similar to the new wstringToString overload which permits to avoid to construct a std::wstring from a wchar_t pointer, therefore reducing overhead caused by allocations and copies, just to be able to do the conversion to a UTF8 std::string. A difference from the wstringToString overload is that the size is actually the expected size of the input string, not the max size of the buffer. - Changed the to_bytes and from_bytes conversions functions to always call the respective Windows API conversion functions with the precise size of the input string, to avoid the need to scan for the null terminator. When std::wstring and std::string are passed to wstringToString and stringToWstring, they are expected to be already of the correct size, so to terminate exactly just before the null terminator. For the other overloads, wstringToString and stringToWstring will scan the provided buffers for a null terminator. - Reduced overallocation to prepare the biggest needed output due to conversions between UTF8 -> UTF16 and UTF16 -> UTF8. The overallocation was considering sizes given to std::string and std::wstring to be always a factor of bytes, but that's not the case because the size of the strings is in characters. - Added checks to ensure that the strings are not going outside of the size limits imposed by the conversion and the Windows APIs. - User code changed to reflect the previous implementation changes, specifically to call the overload that provides the bounded scan where possible. Done some improvements around how many times conversions were done. - Added some additional tests for the unicode conversion functions to test special cases. --- osquery/filesystem/windows/fileops.cpp | 30 ++--- osquery/process/windows/process.cpp | 6 +- .../usersgroups/windows/users_service.cpp | 11 +- .../tables/system/windows/authenticode.cpp | 8 +- .../tables/system/windows/certificates.cpp | 30 ++--- osquery/tables/system/windows/drivers.cpp | 17 ++- .../tables/system/windows/ie_extensions.cpp | 2 +- osquery/tables/system/windows/intel_me.cpp | 11 +- osquery/tables/system/windows/kernel_info.cpp | 2 +- .../tables/system/windows/logged_in_users.cpp | 3 +- .../system/windows/ntfs_acl_permissions.cpp | 2 +- osquery/tables/system/windows/pipes.cpp | 10 +- osquery/tables/system/windows/prefetch.cpp | 23 ++-- osquery/tables/system/windows/processes.cpp | 11 +- osquery/tables/system/windows/registry.cpp | 54 ++++---- .../tables/system/windows/smbios_tables.cpp | 2 +- .../tables/system/windows/windows_crashes.cpp | 28 ++-- osquery/tables/utility/time.cpp | 5 +- osquery/utils/conversions/windows/strings.cpp | 122 ++++++++++++++---- osquery/utils/conversions/windows/strings.h | 24 +++- .../conversions/windows/tests/strings.cpp | 82 ++++++++++-- osquery/utils/pidfile/pidfile_windows.cpp | 6 +- osquery/utils/system/windows/env.cpp | 7 +- osquery/utils/system/windows/system.cpp | 4 +- .../system/windows/users_groups_helpers.cpp | 9 +- 25 files changed, 336 insertions(+), 173 deletions(-) diff --git a/osquery/filesystem/windows/fileops.cpp b/osquery/filesystem/windows/fileops.cpp index 842076eebf3..2a39c128710 100644 --- a/osquery/filesystem/windows/fileops.cpp +++ b/osquery/filesystem/windows/fileops.cpp @@ -117,7 +117,7 @@ Status windowsShortPathToLongPath(const std::string& shortPath, if (ret == 0) { return Status(GetLastError(), "Failed to convert short path to long path"); } - rLongPath = wstringToString(longPath); + rLongPath = wstringToString(longPath, ARRAYSIZE(longPath)); return Status::success(); } @@ -131,8 +131,7 @@ Status windowsGetVersionInfo(const std::string& path, if (verInfo == nullptr) { return Status(1, "Failed to malloc for version info"); } - auto err = GetFileVersionInfoW( - stringToWstring(path).c_str(), handle, verSize, verInfo.get()); + auto err = GetFileVersionInfoW(wpath.c_str(), handle, verSize, verInfo.get()); if (err == 0) { return Status(GetLastError(), "Failed to get file version info"); } @@ -1587,7 +1586,7 @@ boost::optional getHomeDirectory() { return value; } else if (SUCCEEDED(::SHGetFolderPathW( nullptr, CSIDL_PROFILE, nullptr, 0, profile.data()))) { - return wstringToString(profile.data()); + return wstringToString(profile.data(), profile.size()); } else { return boost::none; } @@ -1778,12 +1777,15 @@ Status platformStat(const fs::path& path, WINDOWS_STAT* wfile_stat) { // std::string, in which case, internal fs::path conversion to wstring will be // performed incorrectly. - if (PathIsDirectoryW(stringToWstring(path.string()).c_str())) { + std::string path_str = path.string(); + std::wstring path_wstr = stringToWstring(path_str); + + if (PathIsDirectoryW(path_wstr.c_str())) { FLAGS_AND_ATTRIBUTES |= FILE_FLAG_BACKUP_SEMANTICS; } // Get the handle of the file object. - auto file_handle = CreateFileW(stringToWstring(path.string()).c_str(), + auto file_handle = CreateFileW(path_wstr.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, @@ -1795,7 +1797,7 @@ Status platformStat(const fs::path& path, WINDOWS_STAT* wfile_stat) { if (file_handle == INVALID_HANDLE_VALUE) { CloseHandle(file_handle); return Status(-1, - "CreateFile failed for " + path.string() + " with " + + "CreateFile failed for " + path_str + " with " + std::to_string(GetLastError())); } @@ -1817,7 +1819,7 @@ Status platformStat(const fs::path& path, WINDOWS_STAT* wfile_stat) { if (ret != ERROR_SUCCESS) { CloseHandle(file_handle); return Status(-1, - "GetSecurityInfo failed for " + path.string() + " with " + + "GetSecurityInfo failed for " + path_str + " with " + std::to_string(GetLastError())); } @@ -1828,7 +1830,7 @@ Status platformStat(const fs::path& path, WINDOWS_STAT* wfile_stat) { CloseHandle(file_handle); LocalFree(security_descriptor); return Status(-1, - "GetFileInformationByHandle failed for " + path.string() + + "GetFileInformationByHandle failed for " + path_str + " with " + std::to_string(GetLastError())); } @@ -1908,7 +1910,7 @@ Status platformStat(const fs::path& path, WINDOWS_STAT* wfile_stat) { : wfile_stat->size = li.QuadPart; const char* drive_letter = nullptr; - auto drive_letter_index = PathGetDriveNumberW(path.wstring().c_str()); + auto drive_letter_index = PathGetDriveNumberW(path_wstr.c_str()); if (drive_letter_index != -1 && kDriveLetters.count(drive_letter_index)) { drive_letter = kDriveLetters.at(drive_letter_index).c_str(); @@ -1944,12 +1946,10 @@ Status platformStat(const fs::path& path, WINDOWS_STAT* wfile_stat) { (!ret) ? wfile_stat->ctime = -1 : wfile_stat->ctime = longIntToUnixtime(basic_info.ChangeTime); - windowsGetVersionInfo(wstringToString(path.wstring()), - wfile_stat->product_version, - wfile_stat->file_version); + windowsGetVersionInfo( + path_str, wfile_stat->product_version, wfile_stat->file_version); - windowsGetOriginalFilename(wstringToString(path.wstring()), - wfile_stat->original_filename); + windowsGetOriginalFilename(path_str, wfile_stat->original_filename); CloseHandle(file_handle); diff --git a/osquery/process/windows/process.cpp b/osquery/process/windows/process.cpp index 6193f72b03e..76e73bd4f5e 100644 --- a/osquery/process/windows/process.cpp +++ b/osquery/process/windows/process.cpp @@ -292,7 +292,7 @@ std::shared_ptr PlatformProcess::launchExtension( // of the string content. Also it is guaranteed that the buffer is // null-terminated. See // https://en.cppreference.com/w/cpp/string/basic_string/data - auto argv = argv_stream.str(); + auto argv_str = argv_stream.str(); // In POSIX, this environment variable is set to the child's process ID. But // that is not easily accomplishable on Windows and provides no value since @@ -305,11 +305,11 @@ std::shared_ptr PlatformProcess::launchExtension( // We are autoloading a Python extension, so pass off to our helper if (ext_path.extension().wstring() == L".ext") { - return launchTestPythonScript(wstringToString(argv)); + return launchTestPythonScript(wstringToString(argv_str)); } else { auto status = ::CreateProcess(nullptr, - argv.data(), + argv_str.data(), nullptr, nullptr, TRUE, diff --git a/osquery/system/usersgroups/windows/users_service.cpp b/osquery/system/usersgroups/windows/users_service.cpp index fe89298134e..fba26c2cfc6 100644 --- a/osquery/system/usersgroups/windows/users_service.cpp +++ b/osquery/system/usersgroups/windows/users_service.cpp @@ -105,20 +105,18 @@ std::optional> getRoamingProfileSids() { return {}; } - std::wstring key_name; - key_name.resize(max_key_length); + WCHAR key_name[max_key_length]{}; std::vector subkeys_names; // Process registry subkeys for (DWORD i = 0; i < subkeys_count; i++) { - ret_code = - RegEnumKeyW(registry_handle.get(), i, key_name.data(), max_key_length); + ret_code = RegEnumKeyW(registry_handle.get(), i, key_name, max_key_length); if (ret_code != ERROR_SUCCESS) { return std::nullopt; } - subkeys_names.emplace_back(wstringToString(key_name)); + subkeys_names.emplace_back(wstringToString(key_name, max_key_length)); } return subkeys_names; @@ -312,7 +310,8 @@ void UsersService::processRoamingProfiles( LocalFree(sid); if (ret != FALSE) { - new_user.username = wstringToString(account_name); + new_user.username = + wstringToString(account_name, ARRAYSIZE(account_name)); /* NOTE: This still keeps the old behavior where if getting the gid from the first local group or the primary group id fails, then we use the uid of the user. */ diff --git a/osquery/tables/system/windows/authenticode.cpp b/osquery/tables/system/windows/authenticode.cpp index fe443c93545..0c58fc17ed9 100644 --- a/osquery/tables/system/windows/authenticode.cpp +++ b/osquery/tables/system/windows/authenticode.cpp @@ -22,10 +22,10 @@ #include // clang-format on +#include #include #include #include -#include #include #include @@ -342,7 +342,7 @@ Status getCertificateInformation(SignatureInformation& signature_info, return false; } - value = wstringToString(buffer); + value = wstringToString(buffer.data(), buffer.size()); return true; }; @@ -536,5 +536,5 @@ QueryData genAuthenticode(QueryContext& context) { return results; } -} -} +} // namespace tables +} // namespace osquery diff --git a/osquery/tables/system/windows/certificates.cpp b/osquery/tables/system/windows/certificates.cpp index 1464b807948..ded67549578 100644 --- a/osquery/tables/system/windows/certificates.cpp +++ b/osquery/tables/system/windows/certificates.cpp @@ -228,7 +228,7 @@ std::string getUsernameFromSid(const std::string& sidString) { return ""; } - return wstringToString(uname.data()); + return wstringToString(uname.data(), uname.size()); } bool isValidSid(const std::string& maybeSid) { @@ -414,44 +414,44 @@ void addCertRow(PCCERT_CONTEXT certContext, r["username"] = username; r["store_id"] = storeId; r["sha1"] = fingerprint; - std::vector certBuff; - certBuff.resize(256, 0); - std::fill(certBuff.begin(), certBuff.end(), 0); + std::vector certBuff(256); CertGetNameString(certContext, CERT_NAME_SIMPLE_DISPLAY_TYPE, 0, nullptr, certBuff.data(), - static_cast(certBuff.size())); - r["common_name"] = wstringToString(certBuff.data()); + certBuff.size()); + r["common_name"] = wstringToString(certBuff.data(), certBuff.size()); auto subjSize = CertNameToStr(certContext->dwCertEncodingType, &(certContext->pCertInfo->Subject), CERT_SIMPLE_NAME_STR, nullptr, 0); - certBuff.resize(subjSize, 0); + certBuff.resize(subjSize); std::fill(certBuff.begin(), certBuff.end(), 0); subjSize = CertNameToStr(certContext->dwCertEncodingType, &(certContext->pCertInfo->Subject), CERT_SIMPLE_NAME_STR, certBuff.data(), subjSize); - r["subject"] = subjSize == 0 ? "" : wstringToString(certBuff.data()); + r["subject"] = + subjSize == 0 ? "" : wstringToString(certBuff.data(), certBuff.size()); auto issuerSize = CertNameToStr(certContext->dwCertEncodingType, &(certContext->pCertInfo->Issuer), CERT_SIMPLE_NAME_STR, nullptr, 0); - certBuff.resize(issuerSize, 0); + certBuff.resize(issuerSize); std::fill(certBuff.begin(), certBuff.end(), 0); issuerSize = CertNameToStr(certContext->dwCertEncodingType, &(certContext->pCertInfo->Issuer), CERT_SIMPLE_NAME_STR, certBuff.data(), issuerSize); - r["issuer"] = issuerSize == 0 ? "" : wstringToString(certBuff.data()); + r["issuer"] = + issuerSize == 0 ? "" : wstringToString(certBuff.data(), certBuff.size()); // TODO(#5654) 1: Find the right API calls to get whether a cert is for a CA r["ca"] = INTEGER(-1); @@ -513,7 +513,7 @@ void addCertRow(PCCERT_CONTEXT certContext, nullptr, &decodedBuffSize); - certBuff.resize(decodedBuffSize, 0); + certBuff.resize(decodedBuffSize); std::fill(certBuff.begin(), certBuff.end(), 0); auto decodeRet = CryptDecodeObjectEx(CERT_ENCODING, X509_AUTHORITY_KEY_ID2, @@ -543,21 +543,21 @@ void addCertRow(PCCERT_CONTEXT certContext, Status expandEnvironmentVariables(const std::string& src, std::string& dest) { auto srcWstring = stringToWstring(src); - auto srcW = srcWstring.c_str(); - auto expandedSize = ExpandEnvironmentStringsW(srcW, nullptr, 0); + auto expandedSize = ExpandEnvironmentStringsW(srcWstring.c_str(), nullptr, 0); if (expandedSize == 0) { return Status::failure("Unable to get expanded size"); } std::vector buf(expandedSize); - auto ret = ExpandEnvironmentStringsW(srcW, buf.data(), expandedSize); + auto ret = + ExpandEnvironmentStringsW(srcWstring.c_str(), buf.data(), expandedSize); if (ret == 0) { return Status::failure("Environment variable expansion failed"); } else if (ret != expandedSize) { return Status::failure("Partial data written"); } - dest = wstringToString(buf.data()); + dest = wstringToString(buf.data(), buf.size()); return Status::success(); } diff --git a/osquery/tables/system/windows/drivers.cpp b/osquery/tables/system/windows/drivers.cpp index bfb51a9c42b..a9b2b664150 100644 --- a/osquery/tables/system/windows/drivers.cpp +++ b/osquery/tables/system/windows/drivers.cpp @@ -17,13 +17,13 @@ #include // clang-format on +#include #include #include +#include #include -#include #include #include -#include #include #include @@ -75,7 +75,8 @@ static inline std::string kNormalizeImage(std::string& path) { if (path.find("system32") != std::string::npos) { path = boost::regex_replace(path, boost::regex("^.*?system32"), ""); } - boost::filesystem::path normalized_path(wstringToString(sys_root)); + boost::filesystem::path normalized_path( + wstringToString(sys_root.data(), sys_root.size())); normalized_path /= path; return normalized_path.string(); } @@ -161,7 +162,7 @@ Status getDeviceProperty(const device_infoset_t& infoset, } else if (dev_prop_type == DEVPROP_TYPE_INT32) { result = std::to_string(*(PINT32)drv_buff.get()); } else if (dev_prop_type == DEVPROP_TYPE_STRING) { - result = wstringToString((PWCHAR)drv_buff.get()); + result = wstringToString((PWCHAR)drv_buff.get(), buff_size / sizeof(WCHAR)); } else if (dev_prop_type == DEVPROP_TYPE_FILETIME) { result = std::to_string(osquery::filetimeToUnixtime(*(PFILETIME)drv_buff.get())); @@ -208,7 +209,9 @@ Status getDriverImagePath(const std::string& svc_name, std::string& result) { return Status(ret, "Failed to query registry value(length)"); } - auto buff = std::make_unique(buff_size / sizeof(WCHAR)); + const auto buff_chars = buff_size / sizeof(WCHAR); + + auto buff = std::make_unique(buff_chars); ret = RegGetValueW(hkey, nullptr, image_path_value, @@ -220,7 +223,7 @@ Status getDriverImagePath(const std::string& svc_name, std::string& result) { return Status(ret, "Failed to query registry value"); } - auto path = wstringToString(buff.get()); + auto path = wstringToString(buff.get(), buff_chars); result = kNormalizeImage(path); return Status::success(); } @@ -369,7 +372,7 @@ QueryData genDrivers(QueryContext& context) { << r["device_name"] << " with " << GetLastError(); r["inf"] = wstringToString(inf_name); } else { - r["inf"] = wstringToString(inf.data()); + r["inf"] = wstringToString(inf.data(), inf.size()); } } diff --git a/osquery/tables/system/windows/ie_extensions.cpp b/osquery/tables/system/windows/ie_extensions.cpp index e5a9ba2c3f0..0ca3aa02816 100644 --- a/osquery/tables/system/windows/ie_extensions.cpp +++ b/osquery/tables/system/windows/ie_extensions.cpp @@ -18,10 +18,10 @@ #include #include -#include #include "osquery/core/windows/wmi.h" #include "osquery/filesystem/fileops.h" #include "osquery/tables/system/windows/registry.h" +#include namespace osquery { namespace tables { diff --git a/osquery/tables/system/windows/intel_me.cpp b/osquery/tables/system/windows/intel_me.cpp index ed2fa772ee8..bffc934851e 100644 --- a/osquery/tables/system/windows/intel_me.cpp +++ b/osquery/tables/system/windows/intel_me.cpp @@ -28,8 +28,8 @@ #include #include -#include #include +#include // The AMT documentation can be found at the following address: // https://software.intel.com/sites/manageability/AMT_Implementation_and_Reference_Guide/default.htm @@ -333,15 +333,18 @@ osquery::Status getDeviceInterfacePath( std::to_string(err)); } - std::wstring path; - path.assign(device_details->DevicePath, buffer.size() - sizeof(DWORD)); + const auto device_path_size = + wcsnlen(device_details->DevicePath, + (buffer.size() - sizeof(DWORD)) / sizeof(WCHAR)); - if (std::wcslen(path.c_str()) == 0U) { + if (device_path_size == 0U) { return osquery::Status::failure( "Invalid path returned for the given device interface; the string is " "empty"); } + std::wstring path(device_details->DevicePath, device_path_size); + dev_interface_path = wstringToString(path); return osquery::Status::success(); } diff --git a/osquery/tables/system/windows/kernel_info.cpp b/osquery/tables/system/windows/kernel_info.cpp index 849ffa349a1..ff1a549ad50 100644 --- a/osquery/tables/system/windows/kernel_info.cpp +++ b/osquery/tables/system/windows/kernel_info.cpp @@ -39,7 +39,7 @@ void GetSystemDriveGUID(Row& r) { auto sysRoot = getSystemRoot().root_name().wstring() + L"\\"; if (GetVolumeNameForVolumeMountPoint( sysRoot.c_str(), static_cast(buf), 50)) { - r["device"] = SQL_TEXT(wstringToString(buf)); + r["device"] = SQL_TEXT(wstringToString(buf, ARRAYSIZE(buf))); } } diff --git a/osquery/tables/system/windows/logged_in_users.cpp b/osquery/tables/system/windows/logged_in_users.cpp index c288a6d5d34..d095326964e 100644 --- a/osquery/tables/system/windows/logged_in_users.cpp +++ b/osquery/tables/system/windows/logged_in_users.cpp @@ -77,7 +77,8 @@ QueryData genLoggedInUsers(QueryContext& context) { } const auto wtsSession = reinterpret_cast(sessionInfo); - r["user"] = SQL_TEXT(wstringToString(wtsSession->UserName)); + r["user"] = SQL_TEXT( + wstringToString(wtsSession->UserName, ARRAYSIZE(wtsSession->UserName))); r["type"] = SQL_TEXT(kSessionStates.at(pSessionInfo[i].State)); r["tty"] = pSessionInfo[i].pSessionName == nullptr ? "" diff --git a/osquery/tables/system/windows/ntfs_acl_permissions.cpp b/osquery/tables/system/windows/ntfs_acl_permissions.cpp index bdb55483b88..d2bd58ac620 100644 --- a/osquery/tables/system/windows/ntfs_acl_permissions.cpp +++ b/osquery/tables/system/windows/ntfs_acl_permissions.cpp @@ -139,7 +139,7 @@ std::string pSidToStrUserName(PSID psid) { VLOG(1) << "LookupAccountSid Error " << GetLastError(); return ""; } else { - return wstringToString(uname.data()); + return wstringToString(uname.data(), uname.size()); } } diff --git a/osquery/tables/system/windows/pipes.cpp b/osquery/tables/system/windows/pipes.cpp index 67ad5b28ca8..b3b8ddba37e 100644 --- a/osquery/tables/system/windows/pipes.cpp +++ b/osquery/tables/system/windows/pipes.cpp @@ -33,10 +33,12 @@ QueryData genPipes(QueryContext& context) { do { Row r; - r["name"] = wstringToString(findFileData.cFileName); + std::wstring filename = findFileData.cFileName; + + r["name"] = wstringToString(filename); unsigned long pid = 0; - auto pipePath = L"\\\\.\\pipe\\" + std::wstring(findFileData.cFileName); + auto pipePath = L"\\\\.\\pipe\\" + filename; auto pipeHandle = CreateFileW(pipePath.c_str(), GENERIC_READ, @@ -82,5 +84,5 @@ QueryData genPipes(QueryContext& context) { FindClose(findHandle); return results; } -} -} +} // namespace tables +} // namespace osquery diff --git a/osquery/tables/system/windows/prefetch.cpp b/osquery/tables/system/windows/prefetch.cpp index 5da3807bfca..74ae6c9a811 100644 --- a/osquery/tables/system/windows/prefetch.cpp +++ b/osquery/tables/system/windows/prefetch.cpp @@ -129,8 +129,10 @@ typedef struct _DIRECTORY_STRING { PrefetchHeader parseHeader(const PREFETCH_FILE_HEADER* header) { PrefetchHeader result; - if (header->FileName[ARRAYSIZE(header->FileName) - 1] == L'\0') { - result.filename = wstringToString(header->FileName); + const auto filename_array_size = ARRAYSIZE(header->FileName); + + if (header->FileName[filename_array_size - 1] == L'\0') { + result.filename = wstringToString(header->FileName, filename_array_size); } if (result.filename.empty()) { LOG(INFO) << "Did not find null-terminated filename for prefetch file"; @@ -192,13 +194,15 @@ PrefetchFileInfo parseFileInfo( std::vector filenames; auto next = (PWCHAR)(&data[0] + offset); while (*next != L'\0') { - auto length = wcsnlen_s(next, (size - total_length) / sizeof(WCHAR)); - if (length == 0 || length == (size - total_length) / sizeof(WCHAR)) { + const auto max_chars = (size - total_length) / sizeof(WCHAR); + + auto length = wcsnlen_s(next, max_chars); + if (length == 0 || length == max_chars) { // A null wide character was not found. break; } - auto filename = wstringToString(next); + auto filename = wstringToString(next, max_chars); filenames.emplace_back(std::move(filename)); total_length += (length + 1) * sizeof(WCHAR); if (total_length >= size) { @@ -267,14 +271,15 @@ PrefetchVolumeInfo parseVolumeInfo( (PDIRECTORY_STRING)(&data[0] + volume_offset + dir_offset); dir_offset += sizeof(DIRECTORY_STRING); - auto length = wcsnlen_s(prefetch_directory->Directory, - (volume_size - dir_offset) / sizeof(WCHAR)); - if (length == 0 || length == (volume_size - dir_offset) / sizeof(WCHAR)) { + const auto max_chars = (volume_size - dir_offset) / sizeof(WCHAR); + + auto length = wcsnlen_s(prefetch_directory->Directory, max_chars); + if (length == 0 || length == max_chars) { // A null wide character was not found. break; } - auto filename = wstringToString(prefetch_directory->Directory); + auto filename = wstringToString(prefetch_directory->Directory, max_chars); directories.emplace_back(std::move(filename)); dir_offset += (length + 1) * sizeof(WCHAR); } diff --git a/osquery/tables/system/windows/processes.cpp b/osquery/tables/system/windows/processes.cpp index 592d559157c..7e3ae3ca0a3 100644 --- a/osquery/tables/system/windows/processes.cpp +++ b/osquery/tables/system/windows/processes.cpp @@ -240,7 +240,7 @@ Status genMemoryMap(unsigned long pid, QueryData& results) { BIGINT(reinterpret_cast(mInfo.AllocationBase)); r["device"] = "-1"; r["inode"] = INTEGER(-1); - r["path"] = wstringToString(me.szExePath); + r["path"] = wstringToString(me.szExePath, ARRAYSIZE(me.szExePath)); r["pseudo"] = INTEGER(-1); results.push_back(r); } @@ -333,7 +333,7 @@ Status getProcessCommandLineLegacy(HANDLE proc, return Status::failure("Failed to read command line for " + std::to_string(pid)); } - out = wstringToString(command_line.data()); + out = wstringToString(command_line.data(), command_line.size()); return Status::success(); } @@ -390,7 +390,7 @@ Status getProcessCurrentDirectory(HANDLE proc, return Status::failure("Failed to read current working directory for " + std::to_string(pid)); } - out = wstringToString(current_directory.data()); + out = wstringToString(current_directory.data(), current_directory.size()); return Status::success(); } @@ -409,7 +409,7 @@ void getProcessPathInfo(HANDLE& proc, VLOG(1) << "Failed to lookup path information for process " << pid << " with " << GetLastError(); } else { - r["path"] = SQL_TEXT(wstringToString(path.data())); + r["path"] = SQL_TEXT(wstringToString(path.data(), path.size())); } { @@ -586,7 +586,8 @@ TableRows genProcesses(QueryContext& context) { auto r = make_table_row(); r["pid"] = BIGINT(pid); r["parent"] = BIGINT(proc.th32ParentProcessID); - r["name"] = SQL_TEXT(wstringToString(proc.szExeFile)); + r["name"] = + SQL_TEXT(wstringToString(proc.szExeFile, ARRAYSIZE(proc.szExeFile))); r["threads"] = INTEGER(proc.cntThreads); // Set default values for columns, in the event opening the process fails diff --git a/osquery/tables/system/windows/registry.cpp b/osquery/tables/system/windows/registry.cpp index 74c61c9c478..eb4df71c165 100644 --- a/osquery/tables/system/windows/registry.cpp +++ b/osquery/tables/system/windows/registry.cpp @@ -197,7 +197,7 @@ Status getUsernameFromKey(const std::string& key, std::string& rUsername) { if (!result) { return Status(GetLastError(), "Could not find sid"); } else { - rUsername = std::move(wstringToString(accntName)); + rUsername = wstringToString(accntName, ARRAYSIZE(accntName)); } } return Status::success(); @@ -279,11 +279,13 @@ Status queryKey(const std::string& keyPath, QueryData& results) { return Status(retCode, "Failed to enumerate registry key"); } + std::string keyName = wstringToString(achKey.get(), maxKeyLength); + Row r; r["key"] = keyPath; r["type"] = "subkey"; - r["name"] = wstringToString(achKey.get()); - r["path"] = keyPath + kRegSep + wstringToString(achKey.get()); + r["name"] = keyName; + r["path"] = keyPath + kRegSep + keyName; r["mtime"] = std::to_string(osquery::filetimeToUnixtime(ftLastWriteTime)); r["data"] = ""; results.push_back(r); @@ -335,11 +337,12 @@ Status queryKey(const std::string& keyPath, QueryData& results) { bpDataBuff[lpData - 1] = 0x00; } + std::string keyValue = wstringToString(achValue.get(), maxValueName); + Row r; r["key"] = keyPath; - r["name"] = ((achValue[0] == L'\0') ? "(Default)" - : wstringToString(achValue.get())); - r["path"] = keyPath + kRegSep + wstringToString(achValue.get()); + r["name"] = (achValue[0] == L'\0') ? "(Default)" : keyValue; + r["path"] = keyPath + kRegSep + keyValue; if (kRegistryTypes.count(lpType) > 0) { r["type"] = kRegistryTypes.at(lpType); } else { @@ -349,17 +352,11 @@ Status queryKey(const std::string& keyPath, QueryData& results) { r["data"] = ""; if (bpDataBuff != nullptr) { - /// REG_LINK is a Unicode string, which in Windows is wchar_t - std::string regLinkStr; - if (lpType == REG_LINK) { - regLinkStr = - wstringToString(reinterpret_cast(bpDataBuff.get())); - } - std::vector regBinary; std::string data; - std::vector multiSzStrs; - auto p = reinterpret_cast(bpDataBuff.get()); + auto wchar_buffer = + std::wstring_view(reinterpret_cast(bpDataBuff.get()), + cbMaxValueData / sizeof(WCHAR)); switch (lpType) { case REG_FULL_RESOURCE_DESCRIPTOR: @@ -378,21 +375,27 @@ Status queryKey(const std::string& keyPath, QueryData& results) { case REG_DWORD_BIG_ENDIAN: r["data"] = std::to_string(_byteswap_ulong(*((int*)bpDataBuff.get()))); break; - case REG_EXPAND_SZ: - r["data"] = - wstringToString(reinterpret_cast(bpDataBuff.get())); - break; + /// REG_LINK is a Unicode string, which in Windows is wchar_t case REG_LINK: - r["data"] = regLinkStr; + case REG_EXPAND_SZ: + r["data"] = wstringToString(wchar_buffer.data(), wchar_buffer.size()); break; - case REG_MULTI_SZ: - while (*p != 0x00) { - std::string s = wstringToString(p); - p += wcslen(p) + 1; + case REG_MULTI_SZ: { + std::vector multiSzStrs; + std::size_t processed_chars = 0; + while (processed_chars < wchar_buffer.size() && + wchar_buffer[processed_chars] != L'\0') { + auto chars_left = wchar_buffer.size() - processed_chars; + std::string s = + wstringToString(&wchar_buffer[processed_chars], chars_left); multiSzStrs.push_back(s); + + processed_chars += + wcsnlen(&wchar_buffer[processed_chars], chars_left) + 1; } r["data"] = boost::algorithm::join(multiSzStrs, ","); break; + } case REG_NONE: r["data"] = "(zero-length binary value)"; break; @@ -400,8 +403,7 @@ Status queryKey(const std::string& keyPath, QueryData& results) { r["data"] = std::to_string(*((unsigned long long*)bpDataBuff.get())); break; case REG_SZ: - r["data"] = - wstringToString(reinterpret_cast(bpDataBuff.get())); + r["data"] = wstringToString(wchar_buffer.data(), wchar_buffer.size()); break; default: break; diff --git a/osquery/tables/system/windows/smbios_tables.cpp b/osquery/tables/system/windows/smbios_tables.cpp index 01c1f4d852c..59972892e93 100644 --- a/osquery/tables/system/windows/smbios_tables.cpp +++ b/osquery/tables/system/windows/smbios_tables.cpp @@ -182,7 +182,7 @@ QueryData genMemoryDevices(QueryContext& context) { wmiResults[i].GetLong("DataWidth", dataWidth); r["data_width"] = INTEGER(dataWidth); std::wstring capacityWStr; - wmiResults[i].GetString(stringToWstring("Capacity"), capacityWStr); + wmiResults[i].GetString(L"Capacity", capacityWStr); r["size"] = INTEGER(getMemorySize(capacityWStr)); wmiResults[i].GetString("DeviceLocator", r["device_locator"]); wmiResults[i].GetString("BankLabel", r["bank_locator"]); diff --git a/osquery/tables/system/windows/windows_crashes.cpp b/osquery/tables/system/windows/windows_crashes.cpp index 8d1c3702518..f4a6ccc9152 100644 --- a/osquery/tables/system/windows/windows_crashes.cpp +++ b/osquery/tables/system/windows/windows_crashes.cpp @@ -447,8 +447,8 @@ Status logPEBInfo(IDebugClient5* client, } wchar_t curDir[MAX_PATH + 1] = {0}; data->ReadUnicodeStringVirtualWide( - curDirBufferAddr, sizeof(curDir), curDir, MAX_PATH + 1, nullptr); - r["current_directory"] = wstringToString(curDir); + curDirBufferAddr, sizeof(curDir), curDir, ARRAYSIZE(curDir), nullptr); + r["current_directory"] = wstringToString(curDir, ARRAYSIZE(curDir)); // Get CommandLine offset in ProcessParameters unsigned long cmdLineOffset = 0; @@ -464,13 +464,11 @@ Status logPEBInfo(IDebugClient5* client, &cmdLineBufferAddr) != S_OK) { return Status(1); } - wchar_t cmdLine[UNICODE_STRING_MAX_BYTES] = {0}; - data->ReadUnicodeStringVirtualWide(cmdLineBufferAddr, - sizeof(cmdLine), - cmdLine, - UNICODE_STRING_MAX_BYTES, - nullptr); - r["command_line"] = wstringToString(cmdLine); + + wchar_t cmdLine[UNICODE_STRING_MAX_CHARS] = {0}; + data->ReadUnicodeStringVirtualWide( + cmdLineBufferAddr, sizeof(cmdLine), cmdLine, ARRAYSIZE(cmdLine), nullptr); + r["command_line"] = wstringToString(cmdLine, ARRAYSIZE(cmdLine)); // Get Environment offset in ProcessParameters unsigned long envOffset = 0; @@ -489,13 +487,13 @@ Status logPEBInfo(IDebugClient5* client, // Loop through environment variables and log those of interest // The environment variables are stored in the following format: // Var1=Value1\0Var2=Value2\0Var3=Value3\0 ... VarN=ValueN\0\0 - wchar_t env[UNICODE_STRING_MAX_BYTES] = {0}; + wchar_t env[UNICODE_STRING_MAX_CHARS] = {0}; unsigned long bytesRead = 0; auto ret = data->ReadUnicodeStringVirtualWide( - envBufferAddr, sizeof(env), env, UNICODE_STRING_MAX_BYTES, &bytesRead); + envBufferAddr, sizeof(env), env, ARRAYSIZE(env), &bytesRead); while (ret == S_OK) { envBufferAddr += bytesRead; - auto envVar = wstringToString(env); + auto envVar = wstringToString(env, ARRAYSIZE(env)); auto pos = envVar.find('='); auto varName = envVar.substr(0, pos); auto varValue = envVar.substr(pos + 1, envVar.length()); @@ -507,7 +505,7 @@ Status logPEBInfo(IDebugClient5* client, } ret = data->ReadUnicodeStringVirtualWide( - envBufferAddr, sizeof(env), env, UNICODE_STRING_MAX_BYTES, &bytesRead); + envBufferAddr, sizeof(env), env, ARRAYSIZE(env), &bytesRead); } return Status::success(); @@ -654,5 +652,5 @@ QueryData genCrashLogs(QueryContext& context) { return results; } -} -} +} // namespace tables +} // namespace osquery diff --git a/osquery/tables/utility/time.cpp b/osquery/tables/utility/time.cpp index cd058f78950..dacc183f916 100644 --- a/osquery/tables/utility/time.cpp +++ b/osquery/tables/utility/time.cpp @@ -45,7 +45,9 @@ QueryData genTime(QueryContext& context) { TIME_ZONE_ID_INVALID) { LOG(ERROR) << "Failed to acquire the time"; } else { - local_timezone = wstringToString(time_zone_information.StandardName); + local_timezone = + wstringToString(time_zone_information.StandardName, + ARRAYSIZE(time_zone_information.StandardName)); } #else @@ -65,7 +67,6 @@ QueryData genTime(QueryContext& context) { char timezone[5] = {0}; strftime(timezone, sizeof(timezone), "%Z", &now); - char iso_8601[21] = {0}; strftime(iso_8601, sizeof(iso_8601), "%FT%TZ", &gmt); #ifdef WIN32 diff --git a/osquery/utils/conversions/windows/strings.cpp b/osquery/utils/conversions/windows/strings.cpp index 9d8f6283fa1..64e61c593dd 100644 --- a/osquery/utils/conversions/windows/strings.cpp +++ b/osquery/utils/conversions/windows/strings.cpp @@ -7,7 +7,7 @@ * SPDX-License-Identifier: (Apache-2.0 OR GPL-2.0-only) */ -#include +#include #include #include @@ -20,16 +20,44 @@ namespace osquery { +namespace { + +/* Maximum factor between UTF16 code units and UTF8 code units. + Surrogate pairs require 2 code units in UTF16, + and 4 code units (bytes) in UTF8, so the factor is 2. + Code points between U0800 to UFFFF require 1 code unit in UTF16, + but 3 code units (bytes) in UTF8, so the factor is 3. */ +static constexpr std::size_t Utf16Utf8Factor = 3; + +/* NOTE: There's no factor for UTF8 -> UTF16 because the worst case + is for ASCII, where 1 code unit in UTF8 (1 byte) becomes + 1 code unit in UTF16 (2 bytes). + In all other cases UTF16 is actually smaller. */ + +/* The MultiByteToWideChar and WideCharToMultiByte functions only support + int32_t count of characters, but C++ strings and wcslen/wcsnlen + can potentially overflow the count. So these are the actual safe maximum + number of characters that can be present in the input string, + to prevent overflowing. */ +static constexpr std::size_t kMaxUtf8Chars = + std::numeric_limits::max(); +static constexpr std::size_t kMaxUtf16Chars = + std::numeric_limits::max() / Utf16Utf8Factor; + // Helper object used by Wide/Narrow converter functions struct utf_converter { std::wstring from_bytes(const std::string& str) { std::wstring result; - if (str.length() > 0) { - result.resize(str.length() * 2); + + if (!str.empty() && str.length() <= kMaxUtf8Chars) { + std::int32_t utf16_chars = static_cast(str.length()); + std::int32_t utf8_chars = static_cast(str.length()); + + result.resize(utf16_chars); auto count = MultiByteToWideChar( - CP_UTF8, 0, str.c_str(), -1, &result[0], str.length() * 2); - result.resize(count - 1); + CP_UTF8, 0, str.c_str(), utf8_chars, &result[0], utf16_chars); + result.resize(count); } return result; @@ -37,48 +65,96 @@ struct utf_converter { std::string to_bytes(const std::wstring& str) { std::string result; - if (str.length() > 0) { - result.resize(str.length() * 4); + + if (str.length() > 0 && str.length() <= kMaxUtf16Chars) { + std::int32_t utf8_chars = + static_cast(str.length() * Utf16Utf8Factor); + + std::int32_t utf16_chars = static_cast(str.length()); + + result.resize(utf8_chars); auto count = WideCharToMultiByte(CP_UTF8, 0, str.c_str(), - -1, + utf16_chars, &result[0], - str.length() * 4, - NULL, - NULL); - result.resize(count - 1); + utf8_chars, + nullptr, + nullptr); + result.resize(count); } return result; } + + std::string to_bytes(const wchar_t* str, std::int32_t size) { + std::string result; + std::int32_t utf8_chars = size * Utf16Utf8Factor; + + result.resize(utf8_chars); + auto count = WideCharToMultiByte( + CP_UTF8, 0, str, size, &result[0], utf8_chars, nullptr, nullptr); + result.resize(count); + + return result; + } }; static utf_converter converter; -std::wstring stringToWstring(const std::string& src) { +} // namespace + +std::wstring stringToWstring(const char* src) { std::wstring utf16le_str; - try { - utf16le_str = converter.from_bytes(src); - } catch (std::exception /* e */) { - LOG(WARNING) << "Failed to convert string to wstring " << src; + + std::size_t size = strlen(src); + + if (size == 0 || size > kMaxUtf8Chars) { + return {}; } + utf16le_str = converter.from_bytes(src); + + return utf16le_str; +} + +std::wstring stringToWstring(const std::string& src) { + std::wstring utf16le_str; + utf16le_str = converter.from_bytes(src); + return utf16le_str; } std::string wstringToString(const std::wstring& src) { - std::string utf8_str = converter.to_bytes(src); - return utf8_str; + return converter.to_bytes(src); +} + +std::string wstringToString(const wchar_t* src, std::size_t max_chars) { + if (src == nullptr || max_chars == 0) { + return {}; + } + + std::size_t size = wcsnlen(src, max_chars); + + if (size == 0 || size > kMaxUtf16Chars) { + return {}; + } + + return converter.to_bytes(src, static_cast(size)); } std::string wstringToString(const wchar_t* src) { if (src == nullptr) { - return std::string(""); + return {}; + } + + std::size_t size = wcslen(src); + + if (size == 0 || size > kMaxUtf16Chars) { + return {}; } - std::string utf8_str = converter.to_bytes(src); - return utf8_str; + return converter.to_bytes(src, static_cast(size)); } std::string bstrToString(const BSTR src) { @@ -103,7 +179,7 @@ LONGLONG cimDatetimeToUnixtime(const std::string& src) { }); // Then load up our CIM Datetime string into said class - auto bSrcStr = SysAllocString(stringToWstring(src.c_str()).c_str()); + auto bSrcStr = SysAllocString(stringToWstring(src).c_str()); auto const bSrcStrManager = scope_guard::create([&bSrcStr]() { SysFreeString(bSrcStr); }); hres = pCimDateTime->put_Value(bSrcStr); diff --git a/osquery/utils/conversions/windows/strings.h b/osquery/utils/conversions/windows/strings.h index 1c07b2b5c73..c9045353469 100644 --- a/osquery/utils/conversions/windows/strings.h +++ b/osquery/utils/conversions/windows/strings.h @@ -18,25 +18,43 @@ namespace osquery { /** * @brief Windows helper function for converting narrow strings to wide - * + * The string must be null-terminated, otherwise it will overflow. + * @returns A wide string, constructed from a narrow string + */ +std::wstring stringToWstring(const char* src); + +/** + * @brief Windows helper function for converting narrow strings to wide + * The input string is assumed to only contain characters that need to be + * converted, no null terminator is necessary. Embedded nulls will be kept. * @returns A wide string, constructed from a narrow string */ std::wstring stringToWstring(const std::string& src); /** * @brief Windows helper function for converting wide strings to narrow - * + * The input string is assumed to only contain characters that need to be + * converted, no null terminator is necessary. Embedded nulls will be kept. * @returns A narrow string, constructed from a wide string */ std::string wstringToString(const std::wstring& src); /** * @brief Windows helper function for converting wide C-strings to narrow - * + * The string must be null-terminated, otherwise it will overflow. * @returns A narrow string, constructed from a wide C-string */ std::string wstringToString(const wchar_t* src); +/** + * @brief Windows helper function for converting wide C-strings and a maximum + * size to narrow. max_chars represents the maximum amount of characters that + * will be scanned to find a null-terminator. The null terminator does not need + * to exist; when it's not found, the size for the string will be max_chars. + * @returns A narrow string, constructed from a wide C-string and a size + */ +std::string wstringToString(const wchar_t* src, std::size_t max_chars); + /** * @brief Windows helper function to convert a CIM Datetime to Unix timestamp * diff --git a/osquery/utils/conversions/windows/tests/strings.cpp b/osquery/utils/conversions/windows/tests/strings.cpp index 058787335a5..035e8e8f5df 100644 --- a/osquery/utils/conversions/windows/tests/strings.cpp +++ b/osquery/utils/conversions/windows/tests/strings.cpp @@ -7,6 +7,9 @@ * SPDX-License-Identifier: (Apache-2.0 OR GPL-2.0-only) */ +#include +#include + #include #include @@ -32,37 +35,37 @@ class ConversionsTests : public testing::Test { }; TEST_F(ConversionsTests, test_string_to_wstring) { - std::string narrowString{"The quick brown fox jumps over the lazy dog"}; - auto wideString = stringToWstring(narrowString); + std::string narrow_string{"The quick brown fox jumps over the lazy dog"}; + auto wide_string = stringToWstring(narrow_string); std::wstring expected{L"The quick brown fox jumps over the lazy dog"}; - EXPECT_EQ(wideString, expected); + EXPECT_EQ(wide_string, expected); } TEST_F(ConversionsTests, test_cim_datetime_to_unixtime) { - std::string cimDateTime{"20190724000000.000000-000"}; - auto unixtime = cimDatetimeToUnixtime(cimDateTime); + std::string cim_date_time{"20190724000000.000000-000"}; + auto unixtime = cimDatetimeToUnixtime(cim_date_time); EXPECT_EQ(unixtime, 1563926400); } TEST_F(ConversionsTests, test_wstring_to_string) { - std::wstring wideString{L"The quick brown fox jumps over the lazy dog"}; - auto narrowString = wstringToString(wideString); + std::wstring wide_string{L"The quick brown fox jumps over the lazy dog"}; + auto narrow_string = wstringToString(wide_string); std::string expected{"The quick brown fox jumps over the lazy dog"}; - EXPECT_EQ(narrowString, expected); + EXPECT_EQ(narrow_string, expected); } TEST_F(ConversionsTests, test_string_to_wstring_extended) { - std::string narrowString{"fr\xc3\xb8tz-jorn"}; - auto wideString = stringToWstring(narrowString); + std::string narrow_string{"fr\xc3\xb8tz-jorn"}; + auto wide_string = stringToWstring(narrow_string); std::wstring expected{L"fr\x00f8tz-jorn"}; - EXPECT_EQ(wideString, expected); + EXPECT_EQ(wide_string, expected); } TEST_F(ConversionsTests, test_wstring_to_string_extended) { - std::wstring wideString{L"fr\x00f8tz-jorn"}; - auto narrowString = wstringToString(wideString); + std::wstring wide_string{L"fr\x00f8tz-jorn"}; + auto narrow_string = wstringToString(wide_string); std::string expected{"fr\xc3\xb8tz-jorn"}; - EXPECT_EQ(narrowString, expected); + EXPECT_EQ(narrow_string, expected); } TEST_F(ConversionsTests, test_swapendianiess) { @@ -72,4 +75,55 @@ TEST_F(ConversionsTests, test_swapendianiess) { EXPECT_EQ(swapendian, expected); } +TEST_F(ConversionsTests, test_string_to_wstring_embedded_nulls) { + /* A std::string is supposed to contain the entire data, + therefore supporting embedded nulls. + No additional scan of the string to find a null terminator + will be done for the conversion. */ + std::string narrow_string = "123\0\0\0123"; + auto wide_string = stringToWstring(narrow_string); + + std::wstring expected = L"123\0\0\0123"; + EXPECT_EQ(wide_string, expected); + + // Using a char* does the scan instead + expected = L"123"; + wide_string = stringToWstring(narrow_string.c_str()); + EXPECT_EQ(wide_string, expected); +} + +TEST_F(ConversionsTests, test_wstring_to_string_embedded_nulls) { + /* A std::wstring is supposed to contain the entire data, + therefore supporting embedded nulls. + No additional scan of the string to find a null terminator + will be done for the conversion. */ + std::wstring wide_string = L"123\0\0\0123"; + auto narrow_string = wstringToString(wide_string); + + std::string expected = "123\0\0\0123"; + EXPECT_EQ(narrow_string, expected); + + // Using a char* does the scan instead + expected = "123"; + narrow_string = wstringToString(wide_string.c_str()); + EXPECT_EQ(narrow_string, expected); +} + +TEST_F(ConversionsTests, test_wstring_to_string_max_conversion_factor) { + // Surrogate pairs + std::wstring wide_string = L"\U0001F600\U0001F601"; + auto narrow_string = wstringToString(wide_string); + + std::string expected = u8"\U0001F600\U0001F601"; + + EXPECT_EQ(narrow_string, expected); + + // U0800 to U0FFF + wide_string = L"\u0800\u0801"; + narrow_string = wstringToString(wide_string); + + expected = u8"\u0800\u0801"; + EXPECT_EQ(narrow_string, expected); +} + } // namespace osquery diff --git a/osquery/utils/pidfile/pidfile_windows.cpp b/osquery/utils/pidfile/pidfile_windows.cpp index 015d05c4def..accb28b515a 100644 --- a/osquery/utils/pidfile/pidfile_windows.cpp +++ b/osquery/utils/pidfile/pidfile_windows.cpp @@ -49,8 +49,10 @@ std::string getCurrentPID() noexcept { Expected Pidfile::createFile( const std::string& path) noexcept { + auto path_wstr = stringToWstring(path); + auto file_handle = - CreateFileW(stringToWstring(path).c_str(), + CreateFileW(path_wstr.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, @@ -60,7 +62,7 @@ Expected Pidfile::createFile( if (file_handle == INVALID_HANDLE_VALUE) { file_handle = - CreateFileW(stringToWstring(path).c_str(), + CreateFileW(path_wstr.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, diff --git a/osquery/utils/system/windows/env.cpp b/osquery/utils/system/windows/env.cpp index 3523f457b71..f0eb6a1940f 100644 --- a/osquery/utils/system/windows/env.cpp +++ b/osquery/utils/system/windows/env.cpp @@ -55,7 +55,7 @@ boost::optional getEnvVar(const std::string& name) { return boost::none; } - // It is always possible that between the first GetEnvironmentVariableA call + // It is always possible that between the first GetEnvironmentVariableW call // and this one, a change was made to our target environment variable that // altered the size. Currently, we ignore this scenario and fail if the // returned size is greater than what we expect. @@ -72,12 +72,11 @@ boost::optional getEnvVar(const std::string& name) { return boost::none; } - return wstringToString(buf.data()); + return wstringToString(buf.data(), buf.size()); } boost::optional expandEnvString(const std::string& input) { - std::vector buf; - buf.assign(kInitialBufferSize, L'\0'); + std::vector buf(kInitialBufferSize); if (input.size() > kEnvironmentExpansionMax) { VLOG(1) << "Not expanding environment string larger than " diff --git a/osquery/utils/system/windows/system.cpp b/osquery/utils/system/windows/system.cpp index db865e9d496..ad5d482564d 100644 --- a/osquery/utils/system/windows/system.cpp +++ b/osquery/utils/system/windows/system.cpp @@ -19,7 +19,7 @@ std::string getHostname() { if (0 == GetComputerNameW(nullptr, &size)) { std::vector computer_name(size, 0); GetComputerNameW(computer_name.data(), &size); - return wstringToString(computer_name.data()); + return wstringToString(computer_name.data(), computer_name.size()); } return {}; @@ -30,7 +30,7 @@ std::string getFqdn() { if (0 == GetComputerNameExW(ComputerNameDnsFullyQualified, nullptr, &size)) { std::vector fqdn(size, 0); GetComputerNameExW(ComputerNameDnsFullyQualified, fqdn.data(), &size); - return wstringToString(fqdn.data()); + return wstringToString(fqdn.data(), fqdn.size()); } return {}; diff --git a/osquery/utils/system/windows/users_groups_helpers.cpp b/osquery/utils/system/windows/users_groups_helpers.cpp index be976fb2eda..ef32e0b75b0 100644 --- a/osquery/utils/system/windows/users_groups_helpers.cpp +++ b/osquery/utils/system/windows/users_groups_helpers.cpp @@ -324,11 +324,8 @@ std::string getUserHomeDir(const std::string& sid) { } DWORD value_type; - DWORD value_data_length; - std::wstring value_data; - value_data.resize(max_value_data_length); - - value_data_length = max_value_data_length; + DWORD value_data_length = max_value_data_length; + std::wstring value_data(max_value_data_length / sizeof(WCHAR), L'\0'); ret = RegQueryValueExW(registry_handle.get(), kProfileValueName.c_str(), @@ -350,6 +347,8 @@ std::string getUserHomeDir(const std::string& sid) { return {}; } + value_data.resize(value_data_length / sizeof(WCHAR)); + return wstringToString(value_data); } } // namespace osquery