fix(adhoc-sweep-fixes): CU-86akhf8u2 34 review findings across 27 files

osquery/core/windows/wmi.cpp

@@ -238,7 +238,7 @@ Status WmiResultItem::GetUnsignedLong(const std::string& name,
     VariantClear(&value);
     return Status::failure("Invalid data type returned.");
   }
-  ret = value.lVal;
+  ret = value.ulVal;
   VariantClear(&value);
   return Status::success();
 }
@@ -255,7 +255,7 @@ Status WmiResultItem::GetLongLong(const std::string& name,
     VariantClear(&value);
     return Status::failure("Invalid data type returned.");
   }
-  ret = value.lVal;
+  ret = value.llVal;
   VariantClear(&value);
   return Status::success();
 }
@@ -272,7 +272,7 @@ Status WmiResultItem::GetUnsignedLongLong(const std::string& name,
     VariantClear(&value);
     return Status::failure("Invalid data type returned.");
   }
-  ret = value.lVal;
+  ret = value.ullVal;
   VariantClear(&value);
   return Status::success();
 }
@@ -598,3 +598,4 @@ Status WmiRequest::ExecMethod(const WmiResultItem& object,
 }
 
 } // namespace osquery
+

osquery/events/darwin/scnetwork.cpp

@@ -48,9 +48,14 @@ void SCNetworkEventPublisher::addTarget(
     const SCNetworkReachabilityRef& target) {
   targets_.push_back(target);
 
+  // Keep a stable, heap-allocated copy of the subscription context pointer
+  // alive for the lifetime of the callback registration.
+  auto sc_holder = new SCNetworkSubscriptionContextRef(sc);
+  subscription_refs_.push_back(sc_holder);
+
   // Assign a context (the subscription context) to the target.
   SCNetworkReachabilityContext* context = new SCNetworkReachabilityContext();
-  context->info = (void*)≻
+  context->info = (void*)sc_holder;
   context->retain = nullptr;
   context->release = nullptr;
   contexts_.push_back(context);
@@ -98,6 +103,11 @@ void SCNetworkEventPublisher::clearAll() {
   }
   contexts_.clear();
 
+  for (auto& sc_holder : subscription_refs_) {
+    delete sc_holder;
+  }
+  subscription_refs_.clear();
+
   target_names_.clear();
   target_addresses_.clear();
 }
@@ -183,3 +193,4 @@ Status SCNetworkEventPublisher::run() {
   return Status::success();
 }
 };
+

osquery/events/linux/inotify.cpp

@@ -69,6 +69,8 @@ Status INotifyEventPublisher::setUp() {
   WriteLock lock(scratch_mutex_);
   scratch_ = (char*)malloc(kINotifyBufferSize);
   if (scratch_ == nullptr) {
+    ::close(inotify_handle_);
+    inotify_handle_ = -1;
     return Status(1, "Could not allocate scratch space");
   }
   return Status::success();
@@ -486,3 +488,4 @@ bool INotifyEventPublisher::isPathMonitored(const std::string& path) const {
   return (path_iterator != path_descriptors_.end());
 }
 }
+

osquery/events/windows/etw/etw_provider_config.cpp

@@ -22,7 +22,7 @@ Status EtwProviderConfig::isValid() const {
     return Status::failure("Empty list of Events to handle");
   }
 
-  if (getPostProcessor() == nullptr) {
+  if (getPreProcessor() == nullptr) {
     return Status::failure("Type handlers were not provided");
   }
 
@@ -166,4 +166,4 @@ void EtwProviderConfig::addEventTypeToHandle(const EtwEventType& value) {
   eventTypes_.push_back(value);
 }
 
-} // namespace osquery
+} // namespace osquery

osquery/filesystem/linux/proc.cpp

@@ -37,6 +37,17 @@ Status procGetNamespaceInode(ino_t& inode,
     return Status(1, "Failed to retrieve the inode for namespace " + path);
   }
 
+  // Ensure the buffer is null-terminated, since readlink() does not do this
+  // for us
+  link_destination[link_dest_length] = '\0';
+
+  // The link destination must be at least long enough to hold the
+  // namespace name, the ":[" separator and a closing "]"
+  if (static_cast<std::size_t>(link_dest_length) <
+      namespace_name.size() + 3) {
+    return Status(1, "Invalid descriptor for namespace " + path);
+  }
+
   // The link destination must be in the following form: namespace:[inode]
   if (std::strncmp(link_destination,
                    namespace_name.data(),
@@ -430,3 +441,4 @@ Expected<std::uint64_t, ProcError> getProcRSS(const std::string& process) {
 }
 
 } // namespace osquery
+

osquery/remote/requests.cpp

@@ -12,6 +12,8 @@
 
 #include <zlib.h>
 
+#include <osquery/logger/logger.h>
+
 namespace osquery {
 
 #define MOD_GZIP_ZLIB_WINDOWSIZE 15
@@ -27,6 +29,8 @@ std::string compressString(const std::string& data) {
                    MOD_GZIP_ZLIB_WINDOWSIZE + 16,
                    MOD_GZIP_ZLIB_CFACTOR,
                    Z_DEFAULT_STRATEGY) != Z_OK) {
+    LOG(ERROR) << "compressString: deflateInit2 failed to initialize zlib "
+                  "stream";
     return std::string();
   }
 
@@ -51,9 +55,13 @@ std::string compressString(const std::string& data) {
 
   deflateEnd(&zs);
   if (ret != Z_STREAM_END) {
+    LOG(ERROR) << "compressString: deflate stream did not end cleanly, "
+                   "zlib return code: "
+                << ret;
     return std::string();
   }
 
   return output;
 }
 }
+

osquery/remote/uri.cpp

@@ -40,7 +40,10 @@ Uri::Uri(const std::string& str) : hasAuthority_(false), port_(0) {
 
   std::smatch match;
   if (!std::regex_match(str, match, uriRegex)) {
-    throw std::invalid_argument("Invalid URL");
+    // Malformed URI (potentially attacker-controlled input); leave this
+    // Uri in a safe, empty default state instead of throwing so that a
+    // single bad remote-supplied URI cannot crash the process.
+    return;
   }
 
   scheme_ = submatch(match, 1);
@@ -66,7 +69,18 @@ Uri::Uri(const std::string& str) : hasAuthority_(false), port_(0) {
                           authority.second,
                           authorityMatch,
                           authorityRegex)) {
-      throw std::invalid_argument("Invalid URI authority");
+      // Malformed authority section (potentially attacker-controlled
+      // input); reset to a safe, empty default state instead of throwing.
+      scheme_.clear();
+      hasAuthority_ = false;
+      username_.clear();
+      password_.clear();
+      host_.clear();
+      path_.clear();
+      port_ = 0;
+      query_.clear();
+      fragment_.clear();
+      return;
     }
 
     std::string port(authorityMatch[4].first, authorityMatch[4].second);

osquery/sql/sql.cpp

@@ -141,10 +141,18 @@ Status SQLPlugin::call(const PluginRequest& request, PluginResponse& response) {
     return Status(1, "SQL plugin must include a request action");
   }
 
-  if (request.at("action") == "query") {
+  const auto& action = request.at("action");
+
+  if (action == "query") {
+    if (request.count("query") == 0) {
+      return Status(1, "SQL plugin query action requires a query");
+    }
     bool use_cache = (request.count("cache") && request.at("cache") == "1");
     return this->query(request.at("query"), response, use_cache);
-  } else if (request.at("action") == "columns") {
+  } else if (action == "columns") {
+    if (request.count("query") == 0) {
+      return Status(1, "SQL plugin columns action requires a query");
+    }
     TableColumns columns;
     auto status = this->getQueryColumns(request.at("query"), columns);
     // Convert columns to response
@@ -155,12 +163,21 @@ Status SQLPlugin::call(const PluginRequest& request, PluginResponse& response) {
            {"o", INTEGER(static_cast<size_t>(std::get<2>(column)))}});
     }
     return status;
-  } else if (request.at("action") == "attach") {
+  } else if (action == "attach") {
+    if (request.count("table") == 0) {
+      return Status(1, "SQL plugin attach action requires a table");
+    }
     // Attach a virtual table name using an optional included definition.
     return this->attach(request.at("table"));
-  } else if (request.at("action") == "detach") {
+  } else if (action == "detach") {
+    if (request.count("table") == 0) {
+      return Status(1, "SQL plugin detach action requires a table");
+    }
     return this->detach(request.at("table"));
-  } else if (request.at("action") == "tables") {
+  } else if (action == "tables") {
+    if (request.count("query") == 0) {
+      return Status(1, "SQL plugin tables action requires a query");
+    }
     std::vector<std::string> tables;
     auto status = this->getQueryTables(request.at("query"), tables);
     if (status.ok()) {

osquery/sql/sqlite_filesystem.cpp

@@ -178,8 +178,13 @@ static void getParentDirectory(sqlite3_context* context,
     sqlite3_result_null(context);
     return;
   }
-  char* result = reinterpret_cast<char*>(malloc(last_slash_pos));
+  char* result = reinterpret_cast<char*>(malloc(last_slash_pos + 1));
+  if (result == nullptr) {
+    sqlite3_result_error_nomem(context);
+    return;
+  }
   memcpy(result, path, last_slash_pos);
+  result[last_slash_pos] = '\0';
   sqlite3_result_text(context, result, last_slash_pos, free);
 }
 
@@ -210,3 +215,4 @@ void registerFilesystemExtensions(sqlite3* db) {
                           nullptr);
 }
 } // namespace osquery
+

osquery/tables/events/linux/hardware_events.cpp

@@ -10,6 +10,9 @@
 #include <string>
 #include <vector>
 
+#include <boost/algorithm/string/classification.hpp>
+#include <boost/algorithm/string/split.hpp>
+
 #include <osquery/core/flags.h>
 #include <osquery/core/tables.h>
 #include <osquery/events/linux/udev.h>
@@ -55,7 +58,17 @@ Status HardwareEventSubscriber::Callback(const ECRef& ec, const SCRef& sc) {
 
   struct udev_device* device = ec->device;
   r["type"] = ec->devtype;
-  if (FLAGS_hardware_disabled_types.find(r.at("type")) != std::string::npos) {
+
+  std::vector<std::string> disabled_types;
+  boost::split(disabled_types,
+               FLAGS_hardware_disabled_types,
+               boost::is_any_of(","));
+  for (auto& disabled_type : disabled_types) {
+    boost::trim(disabled_type);
+  }
+  if (std::find(disabled_types.begin(),
+                disabled_types.end(),
+                r.at("type")) != disabled_types.end()) {
     return Status::success();
   }
 
@@ -74,9 +87,8 @@ Status HardwareEventSubscriber::Callback(const ECRef& ec, const SCRef& sc) {
   r["vendor"] = UdevEventPublisher::getValue(device, "ID_VENDOR_FROM_DATABASE");
   r["vendor_id"] =
       INTEGER(UdevEventPublisher::getValue(device, "ID_VENDOR_ID"));
-  r["serial"] =
-      INTEGER(UdevEventPublisher::getValue(device, "ID_SERIAL_SHORT"));
-  r["revision"] = INTEGER(UdevEventPublisher::getValue(device, "ID_REVISION"));
+  r["serial"] = UdevEventPublisher::getValue(device, "ID_SERIAL_SHORT");
+  r["revision"] = UdevEventPublisher::getValue(device, "ID_REVISION");
   add(r);
   return Status(0);
 }

osquery/tables/events/tests/windows/etw_process_events_tests.cpp

@@ -49,17 +49,17 @@ TEST_F(ETWProcessEventsTests, test_subscriber_exists) {
   ASSERT_TRUE(Registry::get().exists("event_subscriber", ETW_SUBSCRIBER_NAME));
 
   auto plugin = Registry::get().plugin("event_subscriber", ETW_SUBSCRIBER_NAME);
-  auto* subscriber =
-      reinterpret_cast<std::shared_ptr<EtwProcessEventSubscriber>*>(&plugin);
+  auto subscriber =
+      std::dynamic_pointer_cast<EtwProcessEventSubscriber>(plugin);
   EXPECT_NE(subscriber, nullptr);
 }
 
 TEST_F(ETWProcessEventsTests, test_publisher_exists) {
   ASSERT_TRUE(Registry::get().exists("event_publisher", ETW_PUBLISHER_NAME));
 
   auto plugin = Registry::get().plugin("event_publisher", ETW_PUBLISHER_NAME);
-  auto* publisher =
-      reinterpret_cast<std::shared_ptr<EtwPublisherProcesses>*>(&plugin);
+  auto publisher =
+      std::dynamic_pointer_cast<EtwPublisherProcesses>(plugin);
   EXPECT_NE(publisher, nullptr);
 }
 

osquery/tables/events/windows/etw_process_events.cpp

@@ -76,7 +76,7 @@ Status EtwProcessEventSubscriber::eventCallback(
     newRow["token_elevation_status"] = INTEGER(eventPayload->TokenIsElevated);
     newRow["mandatory_label"] = SQL_TEXT(eventPayload->MandatoryLabelSid);
     newRow["process_sequence_number"] =
-        BIGINT(eventPayload->ParentProcessSequenceNumber);
+        BIGINT(eventPayload->ProcessSequenceNumber);
     newRow["parent_process_sequence_number"] =
         BIGINT(eventPayload->ParentProcessSequenceNumber);
 
@@ -118,3 +118,4 @@ Status EtwProcessEventSubscriber::eventCallback(
 }
 
 } // namespace osquery
+

osquery/tables/networking/linux/arp_cache.cpp

@@ -7,6 +7,7 @@
  * SPDX-License-Identifier: (Apache-2.0 OR GPL-2.0-only)
  */
 
+#include <cstdlib>
 #include <fstream>
 
 #include <boost/algorithm/string/split.hpp>
@@ -21,6 +22,10 @@ namespace tables {
 
 const std::string kLinuxArpTable = "/proc/net/arp";
 
+// ARP flag bits as defined by the kernel (see <linux/if_arp.h>).
+static const unsigned long kAtfCom = 0x02;
+static const unsigned long kAtfPerm = 0x04;
+
 QueryData genArpCache(QueryContext& context) {
   QueryData results;
 
@@ -62,8 +67,8 @@ QueryData genArpCache(QueryContext& context) {
     r["interface"] = fields[5];
 
     // Note: it's also possible to detect publish entries (ATF_PUB).
-    if (fields[2] == "0x6") {
-      // The string representation of ATF_COM | ATF_PERM.
+    unsigned long flags = strtoul(fields[2].c_str(), nullptr, 16);
+    if ((flags & (kAtfCom | kAtfPerm)) == (kAtfCom | kAtfPerm)) {
       r["permanent"] = "1";
     } else {
       r["permanent"] = "0";

osquery/tables/system/darwin/processes.cpp

@@ -368,6 +368,9 @@ bool parseProcCmdline(std::string& args, size_t len) {
   start = 0;
   while (nargs-- && nul != std::string::npos) {
     nul = args.find('\0', start);
+    if (nul == std::string::npos) {
+      break;
+    }
     args[nul] = ' ';
     start = nul + 1;
   }
@@ -808,3 +811,4 @@ QueryData genProcessMemoryMap(QueryContext& context) {
 }
 } // namespace tables
 } // namespace osquery
+

osquery/tables/system/darwin/sharing_preferences.cpp

@@ -120,10 +120,10 @@ int getPrinterSharingStatus() {
   int ret = cupsAdminGetServerSettings(cups, &num_settings, &settings);
   if (ret != 0) {
     value = cupsGetOption("_share_printers", num_settings, settings);
-    cupsFreeOptions(num_settings, settings);
   } else {
     VLOG(1) << "Unable to get CUPS server settings: " << cupsLastErrorString();
   }
+  cupsFreeOptions(num_settings, settings);
   httpClose(cups);
 
   if (value != nullptr) {

osquery/tables/system/darwin/usb_devices.cpp

@@ -28,9 +28,12 @@ void genUSBDevice(const io_service_t& device, QueryData& results) {
   Row r;
 
   // Get the device details
-  CFMutableDictionaryRef details;
-  IORegistryEntryCreateCFProperties(
+  CFMutableDictionaryRef details = nullptr;
+  auto ret = IORegistryEntryCreateCFProperties(
       device, &details, kCFAllocatorDefault, kNilOptions);
+  if (ret != KERN_SUCCESS || details == nullptr) {
+    return;
+  }
 
   r["usb_address"] = getIOKitProperty(details, "USB Address");
   r["usb_port"] = getIOKitProperty(details, "PortNum");