windows/certificates: Fix enumeration bugs, add columns (osquery#5631)

* Initial implementation * Use case insensitive comparisons for all service names Fixes a bug where certificates for services that correspond to Local Service or Network Service may not have their sids appear correctly. This is because the services table is inconsistent with its user_account column. * Make service name cache query-local Previously, the service name cache existed for the lifetime of the osquery process, which made it susceptible to stale reads if a service restarted under a different user during osquery's lifetime. Now the cache is created for each query. Also refactor it to directly map to the sid, rather than the account name, which removes the need to translate from account name to sid every row. * Fix reference to destroyed object Previously, getCurrentUserInfo took a reference to data from a local vector, whose data is free'd after the function. This refactors the code to use a unique_ptr (similar to how getSidFromUsername) does it.
ccombs-sqsp · Jul 16, 2019 · a60b940 · a60b940
1 parent 31e35ae
commit a60b940
Show file tree

Hide file tree

Showing 10 changed files with 475 additions and 46 deletions.
diff --git a/osquery/filesystem/fileops.h b/osquery/filesystem/fileops.h
@@ -151,6 +151,8 @@ std::string getFileAttribStr(unsigned long);
 
 Status platformStat(const boost::filesystem::path&, WINDOWS_STAT*);
 
+std::unique_ptr<BYTE[]> getCurrentUserInfo();
+
 /**
  * @brief Stores information about the last Windows async request
  *

diff --git a/osquery/filesystem/windows/fileops.cpp b/osquery/filesystem/windows/fileops.cpp
@@ -625,39 +625,49 @@ bool PlatformFile::isSpecialFile() const {
   return (GetFileType(handle_) != FILE_TYPE_DISK);
 }
 
-static Status isUserCurrentUser(PSID user) {
-  if (!IsValidSid(user)) {
-    return Status(-1, "Invalid SID");
-  }
-
+std::unique_ptr<BYTE[]> getCurrentUserInfo() {
   HANDLE token = INVALID_HANDLE_VALUE;
   if (!OpenProcessToken(GetCurrentProcess(), TOKEN_READ, &token)) {
-    return Status(-1, "OpenProcessToken failed");
+    VLOG(1) << "OpenProcessToken failed";
+    return nullptr;
   }
 
   unsigned long size = 0;
   auto ret = GetTokenInformation(token, TokenUser, nullptr, 0, &size);
   if (ret || (!ret && GetLastError() != ERROR_INSUFFICIENT_BUFFER)) {
     CloseHandle(token);
-
-    return Status(
-        -1,
-        "GetTokenInformation failed (" + std::to_string(GetLastError()) + ")");
+    VLOG(1) << "GetTokenInformation failed (" << GetLastError() << ")";
+    return nullptr;
   }
 
-  /// Obtain the user SID behind the token handle
-  std::vector<char> tuBuff(size, '\0');
-  ret = GetTokenInformation(token, TokenUser, tuBuff.data(), size, &size);
+  /// Obtain the TOKEN_USER behind the token handle
+  auto ptoken_user = std::make_unique<BYTE[]>(size);
+
+  ret = GetTokenInformation(token, TokenUser, ptoken_user.get(), size, &size);
   CloseHandle(token);
 
   if (!ret) {
-    return Status(
-        -1,
-        "GetTokenInformation failed (" + std::to_string(GetLastError()) + ")");
+    VLOG(1) << "GetTokenInformation failed (" << GetLastError() << ")";
+    return nullptr;
+  }
+
+  return ptoken_user;
+}
+
+static Status isUserCurrentUser(PSID user) {
+  if (!IsValidSid(user)) {
+    return Status(-1, "Invalid SID");
+  }
+
+  auto ptuSmartPtr = getCurrentUserInfo();
+
+  if (!ptuSmartPtr) {
+    return Status::failure(-1, "Accessing current user info failed");
   }
 
   /// Determine if the current user SID matches that of the specified user
-  PTOKEN_USER ptu = reinterpret_cast<PTOKEN_USER>(tuBuff.data());
+  PTOKEN_USER ptu = reinterpret_cast<PTOKEN_USER>(ptuSmartPtr.get());
+
   if (EqualSid(user, ptu->User.Sid)) {
     return Status::success();
   }

diff --git a/osquery/tables/system/BUCK b/osquery/tables/system/BUCK
@@ -64,6 +64,7 @@ osquery_cxx_library(
             WINDOWS,
             [
                 "windows/registry.h",
+                "windows/certificates.h",
             ],
         ),
     ],

diff --git a/osquery/tables/system/CMakeLists.txt b/osquery/tables/system/CMakeLists.txt
@@ -293,6 +293,7 @@ function(generateOsqueryTablesSystemSystemtable)
   elseif(DEFINED PLATFORM_WINDOWS)
     list(APPEND platform_public_header_files
       windows/registry.h
+      windows/certificates.h
     )
   endif()
 
@@ -331,6 +332,8 @@ function(generateOsqueryTablesSystemSystemtable)
       osquery_tables_system_tests_startupitemstests-test
       PROPERTIES ENVIRONMENT "TEST_CONF_FILES_DIR=${TEST_CONFIGS_DIR}"
     )
+  elseif(DEFINED PLATFORM_WINDOWS)
+    add_test(NAME osquery_tables_system_tests_certificatestests-test COMMAND osquery_tables_system_tests_certificatestests-test)
   endif()
 
   add_test(NAME osquery_tables_system_tests_systemtablestests-test COMMAND osquery_tables_system_tests_systemtablestests-test)

diff --git a/osquery/tables/system/tests/BUCK b/osquery/tables/system/tests/BUCK
@@ -6,7 +6,7 @@
 
 load("//tools/build_defs/oss/osquery:cxx.bzl", "osquery_cxx_test")
 load("//tools/build_defs/oss/osquery:native.bzl", "osquery_target")
-load("//tools/build_defs/oss/osquery:platforms.bzl", "LINUX", "MACOSX", "POSIX")
+load("//tools/build_defs/oss/osquery:platforms.bzl", "LINUX", "MACOSX", "POSIX", "WINDOWS")
 
 osquery_cxx_test(
     name = "md_tables_tests",
@@ -271,6 +271,12 @@ osquery_cxx_test(
                 "darwin/certificates_tests.cpp",
             ],
         ),
+        (
+            WINDOWS,
+            [
+                "windows/certificates_tests.cpp",
+            ],
+        ),
     ],
     visibility = ["PUBLIC"],
     deps = [

diff --git a/osquery/tables/system/tests/CMakeLists.txt b/osquery/tables/system/tests/CMakeLists.txt
@@ -24,7 +24,7 @@ function(osqueryTablesSystemTestsMain)
   if(DEFINED PLATFORM_MACOS)
     generateOsqueryTablesSystemTestsAppstestsTest()
     generateOsqueryTablesSystemTestsAsltestsTest()
-    generateOsqueryTablesSystemTestsCertificatestestsTest()
+    generateOsqueryTablesSystemTestsMacOsCertificatestestsTest()
     generateOsqueryTablesSystemTestsExtendedattributestestsTest()
     generateOsqueryTablesSystemTestsFirewalltestsTest()
     generateOsqueryTablesSystemTestsLaunchdtestsTest()
@@ -34,6 +34,10 @@ function(osqueryTablesSystemTestsMain)
     generateOsqueryTablesSystemTestsStartupitemstestsTest()
     generateOsqueryTablesSystemTestsSignaturetestsTest()
   endif()
+
+  if(DEFINED PLATFORM_WINDOWS)
+    generateOsqueryTablesSystemTestsWindowsCertificatestestsTest()
+  endif()
 endfunction()
 
 function(generateOsqueryTablesSystemTestsMdtablestestsTest)
@@ -227,7 +231,7 @@ function(generateOsqueryTablesSystemTestsAsltestsTest)
   )
 endfunction()
 
-function(generateOsqueryTablesSystemTestsCertificatestestsTest)
+function(generateOsqueryTablesSystemTestsMacOsCertificatestestsTest)
   add_osquery_executable(osquery_tables_system_tests_certificatestests-test darwin/certificates_tests.cpp)
 
   target_link_libraries(osquery_tables_system_tests_certificatestests-test PRIVATE
@@ -403,4 +407,23 @@ function(generateOsqueryTablesSystemTestsSignaturetestsTest)
   )
 endfunction()
 
+function(generateOsqueryTablesSystemTestsWindowsCertificatestestsTest)
+  add_osquery_executable(osquery_tables_system_tests_certificatestests-test windows/certificates_tests.cpp)
+
+  target_link_libraries(osquery_tables_system_tests_certificatestests-test PRIVATE
+    global_cxx_settings
+    osquery_config_tests_testutils
+    osquery_core
+    osquery_core_sql
+    osquery_database
+    osquery_filesystem
+    osquery_remote_tests_remotetestutils
+    osquery_tables_system_systemtable
+    osquery_utils
+    osquery_utils_conversions
+    plugins_database_ephemeral
+    thirdparty_googletest
+  )
+endfunction()
+
 osqueryTablesSystemTestsMain()
diff --git a/osquery/tables/system/tests/windows/certificates_tests.cpp b/osquery/tables/system/tests/windows/certificates_tests.cpp
@@ -0,0 +1,110 @@
+/**
+ *  Copyright (c) 2014-present, Facebook, Inc.
+ *  All rights reserved.
+ *
+ *  This source code is licensed in accordance with the terms specified in
+ *  the LICENSE file found in the root directory of this source tree.
+ */
+
+#include <gtest/gtest.h>
+
+#include <osquery/database.h>
+#include <osquery/flags.h>
+#include <osquery/registry.h>
+#include <osquery/system.h>
+#include <osquery/tables.h>
+
+#include <osquery/tables/system/windows/certificates.h>
+
+namespace osquery {
+
+DECLARE_bool(disable_database);
+
+namespace tables {
+
+class CertificatesTablesTest : public testing::Test {
+ protected:
+  void SetUp() override {
+    Initializer::platformSetup();
+    registryAndPluginInit();
+
+    FLAGS_disable_database = true;
+    DatabasePlugin::setAllowOpen(true);
+    DatabasePlugin::initPlugin();
+  }
+};
+
+TEST_F(CertificatesTablesTest, test_only_store_non_special_case) {
+  LPCWSTR input = L"My";
+  std::string storeLocation = "CurrentService";
+  std::string serviceNameOrUserId, sid, storeName;
+  ServiceNameMap cache;
+
+  parseSystemStoreString(
+      input, storeLocation, cache, serviceNameOrUserId, sid, storeName);
+
+  EXPECT_EQ(serviceNameOrUserId, "");
+  EXPECT_EQ(sid, "");
+  EXPECT_EQ(storeName, "Personal");
+}
+
+TEST_F(CertificatesTablesTest, test_service) {
+  LPCWSTR input = L"RpcSs\\My"; // This service should always exist
+  std::string storeLocation = "Services";
+  std::string serviceNameOrUserId, sid, storeName;
+  ServiceNameMap cache;
+
+  parseSystemStoreString(
+      input, storeLocation, cache, serviceNameOrUserId, sid, storeName);
+
+  EXPECT_EQ(serviceNameOrUserId, "RpcSs");
+  EXPECT_EQ(sid, kNetworkService);
+  EXPECT_EQ(storeName, "Personal");
+}
+
+TEST_F(CertificatesTablesTest, test_user_default) {
+  LPCWSTR input = L".DEFAULT\\My";
+  std::string storeLocation = "Users";
+  std::string serviceNameOrUserId, sid, storeName;
+  ServiceNameMap cache;
+
+  parseSystemStoreString(
+      input, storeLocation, cache, serviceNameOrUserId, sid, storeName);
+
+  EXPECT_EQ(serviceNameOrUserId, ".DEFAULT");
+  EXPECT_EQ(sid, kLocalSystem);
+  EXPECT_EQ(storeName, "Personal");
+}
+
+TEST_F(CertificatesTablesTest, test_user_sid) {
+  LPCWSTR input = L"S-1-5-18\\Root";
+  std::string storeLocation = "Users";
+  std::string serviceNameOrUserId, sid, storeName;
+  ServiceNameMap cache;
+
+  parseSystemStoreString(
+      input, storeLocation, cache, serviceNameOrUserId, sid, storeName);
+
+  EXPECT_EQ(serviceNameOrUserId, "S-1-5-18");
+  EXPECT_EQ(sid, kLocalSystem);
+  EXPECT_EQ(storeName, "Trusted Root Certification Authorities");
+}
+
+TEST_F(CertificatesTablesTest, test_user_classes) {
+  LPCWSTR input =
+      L"S-1-5-21-2821152761-3909955410-1545212275-1001_Classes\\Root";
+  std::string storeLocation = "Users";
+  std::string serviceNameOrUserId, sid, storeName;
+  ServiceNameMap cache;
+
+  parseSystemStoreString(
+      input, storeLocation, cache, serviceNameOrUserId, sid, storeName);
+
+  EXPECT_EQ(serviceNameOrUserId,
+            "S-1-5-21-2821152761-3909955410-1545212275-1001_Classes");
+  EXPECT_EQ(sid, "S-1-5-21-2821152761-3909955410-1545212275-1001");
+  EXPECT_EQ(storeName, "Trusted Root Certification Authorities");
+}
+
+} // namespace tables
+} // namespace osquery
-Original file line number
+Diff line change
@@ Expand Up / @@ -151,6 +151,8 @@ std::string getFileAttribStr(unsigned long); @@
     Status platformStat(const boost::filesystem::path&, WINDOWS_STAT*);
+    std::unique_ptr<BYTE[]> getCurrentUserInfo();
     /**
      * @brief Stores information about the last Windows async request
      *
@@ Expand Down @@