Fix several integer conversions in process_ops

Fix UsersTest.test_sanity on Windows. uid and gid were returned as int (while they normally are unsigned int) and converted to signed integers in the table row. This is wrong because beyond uid and gid not being ints, they are taken from the RID part of the SID which in some cases, like for a Service SID, it can have a value higher than then maximum value of an int, so in the end the number shown in table is negative. Now they are returned as uint32_t and converted as BIGINTs for the table that uses them. Fix other functions return values and conversions depending on the meaning of the value. On Windows stick to its specific types where possible. Convert CRLF to LF on some of the files modified.
ccombs-sqsp · Jul 9, 2019 · 65aa1cf · 65aa1cf
1 parent f34afd2
commit 65aa1cf
Show file tree

Hide file tree

Showing 10 changed files with 255 additions and 243 deletions.
diff --git a/osquery/core/init.cpp b/osquery/core/init.cpp
@@ -208,8 +208,10 @@ volatile std::sig_atomic_t kExitCode{0};
 /// The saved thread ID for shutdown to short-circuit raising a signal.
 static std::thread::id kMainThreadId;
 
+#ifdef OSQUERY_WINDOWS
 /// Legacy thread ID to ensure that the windows service waits before exiting
-unsigned long kLegacyThreadId;
+DWORD kLegacyThreadId;
+#endif
 
 /// When no flagfile is provided via CLI, attempt to read flag 'defaults'.
 const std::string kBackupDefaultFlagfile{OSQUERY_HOME "osquery.flags.default"};
@@ -295,8 +297,10 @@ Initializer::Initializer(int& argc,
   // The 'main' thread is that which executes the initializer.
   kMainThreadId = std::this_thread::get_id();
 
+#ifdef OSQUERY_WINDOWS
   // Maintain a legacy thread id for Windows service stops.
-  kLegacyThreadId = platformGetTid();
+  kLegacyThreadId = static_cast<DWORD>(platformGetTid());
+#endif
 
 #ifndef WIN32
   // Set the max number of open files.

diff --git a/osquery/filesystem/windows/fileops.cpp b/osquery/filesystem/windows/fileops.cpp
@@ -35,8 +35,8 @@ namespace errc = boost::system::errc;
 
 namespace osquery {
 
-int getUidFromSid(PSID sid);
-int getGidFromSid(PSID sid);
+uint32_t getUidFromSid(PSID sid);
+uint32_t getGidFromSid(PSID sid);
 
 /*
  * Avoid having the same right being used in multiple CHMOD_* macros. Doing so

diff --git a/osquery/process/posix/process_ops.cpp b/osquery/process/posix/process_ops.cpp
@@ -26,7 +26,7 @@ namespace osquery {
 
 DECLARE_uint64(alarm_timeout);
 
-int platformGetUid() {
+uint32_t platformGetUid() {
   return ::getuid();
 }
 
@@ -67,7 +67,7 @@ int platformGetPid() {
   return static_cast<int>(getpid());
 }
 
-int platformGetTid() {
+uint64_t platformGetTid() {
   return std::hash<std::thread::id>()(std::this_thread::get_id());
 }
 

diff --git a/osquery/process/process.h b/osquery/process/process.h
@@ -28,8 +28,10 @@ namespace osquery {
 /// Constant for an invalid process
 const auto kInvalidPid = (PlatformPidType)-1;
 
+#ifdef OSQUERY_WINDOWS
 /// Used by Windows to wait on the main execution thread
-extern unsigned long kLegacyThreadId;
+extern DWORD kLegacyThreadId;
+#endif
 
 /**
  * @brief Categories of process states adapted to be platform agnostic
@@ -206,7 +208,7 @@ class SecurityDescriptor {
 #endif
 
 /// Returns the current user's ID (UID on POSIX systems and RID for Windows)
-int platformGetUid();
+uint32_t platformGetUid();
 
 inline void sleepFor(size_t msec) {
   std::chrono::milliseconds mduration(msec);
@@ -265,7 +267,7 @@ int platformGetPid();
  * On Windows, returns the value of GetCurrentThreadId
  * and on posix platforms returns gettid()
  */
-int platformGetTid();
+uint64_t platformGetTid();
 
 /**
  * @brief Allows for platform specific exit logic

diff --git a/osquery/process/windows/process_ops.cpp b/osquery/process/windows/process_ops.cpp
@@ -23,23 +23,24 @@ std::string psidToString(PSID sid) {
   return std::string(sidOut);
 }
 
-int getUidFromSid(PSID sid) {
-  unsigned long const uid_default = -1;
+uint32_t getUidFromSid(PSID sid) {
+  auto const uid_default = static_cast<uint32_t>(-1);
   LPTSTR sidString;
   if (ConvertSidToStringSid(sid, &sidString) == 0) {
     VLOG(1) << "getUidFromSid failed ConvertSidToStringSid error " +
                    std::to_string(GetLastError());
     LocalFree(sidString);
     return uid_default;
   }
+
   auto toks = osquery::split(sidString, "-");
 
   if (toks.size() < 1) {
     LocalFree(sidString);
     return uid_default;
   }
 
-  auto uid_exp = tryTo<unsigned long int>(toks.at(toks.size() - 1), 10);
+  auto uid_exp = tryTo<uint32_t>(toks.at(toks.size() - 1), 10);
 
   if (uid_exp.isError()) {
     LocalFree(sidString);
@@ -51,38 +52,38 @@ int getUidFromSid(PSID sid) {
   return uid_exp.take();
 }
 
-int getGidFromSid(PSID sid) {
+uint32_t getGidFromSid(PSID sid) {
   auto eUse = SidTypeUnknown;
-  unsigned long unameSize = 0;
-  unsigned long domNameSize = 1;
+  DWORD unameSize = 0;
+  DWORD domNameSize = 1;
 
   // LookupAccountSid first gets the size of the username buff required.
   LookupAccountSidW(
       nullptr, sid, nullptr, &unameSize, nullptr, &domNameSize, &eUse);
   std::vector<wchar_t> uname(unameSize);
   std::vector<wchar_t> domName(domNameSize);
-  auto ret = LookupAccountSidW(nullptr,
-                               sid,
-                               uname.data(),
-                               &unameSize,
-                               domName.data(),
-                               &domNameSize,
-                               &eUse);
-
-  if (ret == 0) {
-    return -1;
+  auto accountFound = LookupAccountSidW(nullptr,
+                                        sid,
+                                        uname.data(),
+                                        &unameSize,
+                                        domName.data(),
+                                        &domNameSize,
+                                        &eUse);
+
+  if (accountFound == 0) {
+    return static_cast<uint32_t>(-1);
   }
   // USER_INFO_3 struct contains the RID (uid) of our user
-  unsigned long userInfoLevel = 3;
-  unsigned char* userBuff = nullptr;
-  unsigned long gid = -1;
-  ret = NetUserGetInfo(nullptr, uname.data(), userInfoLevel, &userBuff);
+  DWORD userInfoLevel = 3;
+  LPBYTE userBuff = nullptr;
+  auto gid = static_cast<uint32_t>(-1);
+  auto ret = NetUserGetInfo(nullptr, uname.data(), userInfoLevel, &userBuff);
 
   if (ret == NERR_UserNotFound) {
     LPTSTR sidString;
     ConvertSidToStringSid(sid, &sidString);
     auto toks = osquery::split(sidString, "-");
-    gid = tryTo<unsigned long int>(toks.at(toks.size() - 1), 10).takeOr(gid);
+    gid = tryTo<uint32_t>(toks.at(toks.size() - 1), 10).takeOr(gid);
     LocalFree(sidString);
 
   } else if (ret == NERR_Success) {
@@ -101,8 +102,8 @@ std::unique_ptr<BYTE[]> getSidFromUsername(std::wstring accountName) {
 
   // Call LookupAccountNameW() once to retrieve the necessary buffer sizes for
   // the SID (in bytes) and the domain name (in TCHARS):
-  unsigned long sidBufferSize = 0;
-  unsigned long domainNameSize = 0;
+  DWORD sidBufferSize = 0;
+  DWORD domainNameSize = 0;
   auto eSidType = SidTypeUnknown;
   auto ret = LookupAccountNameW(nullptr,
                                 accountName.c_str(),
@@ -146,24 +147,25 @@ std::unique_ptr<BYTE[]> getSidFromUsername(std::wstring accountName) {
   return sidBuffer;
 }
 
-unsigned long getRidFromSid(PSID sid) {
+DWORD getRidFromSid(PSID sid) {
   BYTE* countPtr = GetSidSubAuthorityCount(sid);
-  unsigned long indexOfRid = static_cast<unsigned long>(*countPtr - 1);
-  unsigned long* ridPtr = GetSidSubAuthority(sid, indexOfRid);
+  DWORD indexOfRid = static_cast<DWORD>(*countPtr - 1);
+  DWORD* ridPtr = GetSidSubAuthority(sid, indexOfRid);
   return *ridPtr;
 }
 
-int platformGetUid() {
+uint32_t platformGetUid() {
+  auto gid_default = static_cast<uint32_t>(-1);
   auto token = INVALID_HANDLE_VALUE;
   if (!::OpenProcessToken(::GetCurrentProcess(), TOKEN_QUERY, &token)) {
-    return -1;
+    return gid_default;
   }
 
   unsigned long nbytes = 0;
   ::GetTokenInformation(token, TokenUser, nullptr, 0, &nbytes);
   if (nbytes == 0) {
     ::CloseHandle(token);
-    return -1;
+    return gid_default;
   }
 
   std::vector<char> tu_buffer;
@@ -176,7 +178,7 @@ int platformGetUid() {
                                       &nbytes);
   ::CloseHandle(token);
   if (status == 0) {
-    return -1;
+    return gid_default;
   }
 
   auto tu = PTOKEN_USER(tu_buffer.data());
@@ -206,7 +208,7 @@ void* platformModuleGetSymbol(ModuleHandle module, const std::string& symbol) {
 }
 
 std::string platformModuleGetError() {
-  return std::string("GetLastError() = " + ::GetLastError());
+  return std::string("GetLastError() = " + std::to_string(::GetLastError()));
 }
 
 bool platformModuleClose(ModuleHandle module) {
@@ -246,11 +248,11 @@ int platformGetPid() {
   return static_cast<int>(GetCurrentProcessId());
 }
 
-int platformGetTid() {
-  return static_cast<int>(GetCurrentThreadId());
+uint64_t platformGetTid() {
+  return GetCurrentThreadId();
 }
 
 void platformMainThreadExit(int excode) {
-  ExitThread(excode);
+  ExitThread(static_cast<DWORD>(excode));
 }
 }
diff --git a/osquery/process/windows/process_ops.h b/osquery/process/windows/process_ops.h
@@ -41,6 +41,6 @@ std::unique_ptr<BYTE[]> getSidFromUsername(std::wstring accountName);
  *
  * @returns the RID represented as an unsigned long integer.
  */
-unsigned long getRidFromSid(PSID sidPtr);
+DWORD getRidFromSid(PSID sidPtr);
 
 } // namespace osquery