]> git.ipfire.org Git - thirdparty/snort3.git/commitdiff
Pull request #4742: rna: coverity fixes
authorRaza Shafiq (rshafiq) <rshafiq@cisco.com>
Mon, 19 May 2025 17:29:22 +0000 (17:29 +0000)
committerSteven Baigal (sbaigal) <sbaigal@cisco.com>
Mon, 19 May 2025 17:29:22 +0000 (17:29 +0000)
Merge in SNORT/snort3 from ~RSHAFIQ/snort3:cov_rna to master

Squashed commit of the following:

commit 54f9ee7379d39560e4085b72b5860aa98d4610b4
Author: rshafiq <rshafiq@cisco.com>
Date:   Fri May 2 10:28:27 2025 -0400

    rna: coverity fixes

13 files changed:
src/host_tracker/host_tracker.cc
src/host_tracker/host_tracker_module.cc
src/main/thread.cc
src/managers/module_manager.cc
src/network_inspectors/perf_monitor/perf_module.h
src/network_inspectors/perf_monitor/perf_tracker.cc
src/network_inspectors/rna/rna_app_discovery.cc
src/network_inspectors/rna/rna_logger.cc
src/network_inspectors/rna/rna_logger.h
src/network_inspectors/rna/rna_mac_cache.h
src/network_inspectors/rna/rna_pnd.cc
src/network_inspectors/rna/rna_pnd.h
src/stream/user/user_session.cc

index 8c3d602cad213c87eb2605627940821606f587a5..c2b27fd6a55154d674c03d9918831c63ecae9d31 100644 (file)
@@ -48,6 +48,7 @@ const uint8_t snort::zero_mac[MAC_SIZE] = {0, 0, 0, 0, 0, 0};
 
 HostTracker::HostTracker()
 {
+    //coverity[y2k38_safety]
     last_seen = nat_count_start = (uint32_t) packet_time();
     visibility = host_cache.get_valid_id(0);
 }
@@ -55,6 +56,7 @@ HostTracker::HostTracker()
 void HostTracker::update_last_seen()
 {
     lock_guard<mutex> lck(host_tracker_lock);
+    //coverity[y2k38_safety]
     last_seen = (uint32_t) packet_time();
 }
 
index 1ecb5dd1542edd4c60fda3d4fd6ffda765be254d..a3fb9875654b6c4fa5e20c2dc5a3f80dfc34394f 100644 (file)
@@ -67,8 +67,10 @@ bool HostTrackerModule::set(const char*, Value& v, SnortConfig*)
         v.get_addr(addr);
 
     else if ( v.is("port") )
+    {
+        // coverity[missing_lock] : lock not needed, as this is called at start or reload time
         app.port = v.get_uint16();
-
+    }
     else if ( v.is("proto") )
     {
         const IpProtocol mask[] =
index 744f4949934defb7ccf1840aa3a0c5cf044f9458..c705feb5d971bfdf99554fbcd7c0b6369621942d 100644 (file)
@@ -154,13 +154,13 @@ const char* get_instance_file(std::string& file, const char* name)
     if ( sc->id_subdir )
     {
         file += '/';
-        struct stat s;
 
-        if ( stat(file.c_str(), &s) )
-            // FIXIT-L getting random 0750 or 0700 (umask not thread local)?
-            if ( mkdir(file.c_str(), 0770) == -1 )
-                ParseError("Failed to create directory %s - %s",
-                    file.c_str(),  get_error(errno));
+        // Explicitly set mode to avoid umask issues (fixes random 0750/0700)
+        mode_t old_mask = umask(0);
+        if ( mkdir(file.c_str(), 0770) == -1 and errno != EEXIST )
+            ParseError("Failed to create directory %s - %s",
+                file.c_str(), get_error(errno));
+        umask(old_mask);
     }
     else if ( sep )
         file += '_';
index 3c07b32a751eeecc9f50fafd8fc4d1225f5c024c..e9cb2654c1e9bfb7e8f41f30ebc35c7a8bb6d0d0 100644 (file)
@@ -547,7 +547,9 @@ static bool begin(Module* m, const Parameter* p, const char* s, int idx, int dep
     }
     else
     {
-        assert(p);
+        if (!p)
+            return false;
+
         if ( (!idx and p->type == Parameter::PT_LIST) or
              (idx and p->type != Parameter::PT_LIST) )
         {
index ce5f0be3850bad46df65b46aa07cad9cdca62b9e..234c5af594d7ee43c61d5b017775d6dd52c43f4e 100644 (file)
@@ -60,7 +60,7 @@ struct ModuleConfig
 {
     // state optimized for run time using indices
     // can't be determined until all modules have loaded (PerfMonitor::configure)
-    snort::Module* ptr;
+    snort::Module* ptr = nullptr;
     std::vector<unsigned> pegs;
 
     void set_name(const std::string& name);
index cb0088bd98b88be2a0daa53a51cf522bd7de3c4f..5554c213e6310ae9b7407ab548050e98d498ded8 100644 (file)
 
 #include "perf_tracker.h"
 
-#include <sys/stat.h>
-
 #include <climits>
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <unistd.h>
 
 #include "log/messages.h"
 #include "main/snort_config.h"
@@ -87,36 +88,44 @@ void PerfTracker::close()
         }
     }
 }
-
 bool PerfTracker::open(bool append)
 {
     if (fname.length())
     {
-        // FIXIT-L this should be deleted; was added as 1-time workaround to
-        // get around the borked perms due to a bug that has been fixed
-        struct stat pt;
-        mode_t mode =  S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH;
+        mode_t mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH;
         const char* file_name = fname.c_str();
         bool existed = false;
 
-        // Check file before change permission
-        if (stat(file_name, &pt) == 0)
+        // Open file securely and create if it doesn't exist
+        int flags = O_NOFOLLOW | O_CREAT | (append ? O_APPEND : O_TRUNC) | O_RDWR;
+        mode_t old_umask = umask(022); // Ensure file is world-readable
+        int fd = ::open(file_name, flags, mode);
+        umask(old_umask);
+
+        if (fd < 0)
+        {
+            ErrorMessage("perfmonitor: Cannot open stats file '%s'.\n", file_name);
+            return false;
+        }
+
+        struct stat pt;
+        if (fstat(fd, &pt) == 0)
         {
             existed = true;
 
-            // Only change permission for file owned by root
-            if ((0 == pt.st_uid) || (0 == pt.st_gid))
+            // Only change ownership if file is owned by root
+            if (pt.st_uid == 0 || pt.st_gid == 0)
             {
-                if (chmod(file_name, mode) != 0)
+                const SnortConfig* sc = SnortConfig::get_conf();
+
+                if (fchmod(fd, mode) != 0)
                 {
                     WarningMessage("perfmonitor: Unable to change mode of "
                         "stats file '%s' to mode:%u: %s.\n",
                         file_name, mode, get_error(errno));
                 }
 
-                const SnortConfig* sc = SnortConfig::get_conf();
-
-                if (chown(file_name, sc->get_uid(), sc->get_gid()) != 0)
+                if (fchown(fd, sc->get_uid(), sc->get_gid()) != 0)
                 {
                     WarningMessage("perfmonitor: Unable to change permissions of "
                         "stats file '%s' to user:%d and group:%d: %s.\n",
@@ -125,16 +134,12 @@ bool PerfTracker::open(bool append)
             }
         }
 
-        // This file needs to be readable by everyone
-        mode_t old_umask = umask(022);
-        // Append to the existing file if just starting up, otherwise we've
-        // rotated so start a new one.
-        fh = fopen(file_name, append ? "a" : "w");
-        umask(old_umask);
-
+        // Convert file descriptor to FILE*
+        fh = fdopen(fd, append ? "a" : "w");
         if (!fh)
         {
             ErrorMessage("perfmonitor: Cannot open stats file '%s'.\n", file_name);
+            ::close(fd);
             return false;
         }
 
index 171ba1d36d7c37833c12cb505de582fcb2821dd9..e2d9aa3ec805876d12298af5a3cedae2ad49e8dd 100644 (file)
@@ -202,6 +202,7 @@ bool RnaAppDiscovery::discover_service(const Packet* p, DiscoveryFilter& filter,
     bool is_new = false;
 
     // Work on a local copy instead of reference as we release lock during event generations
+    //coverity[y2k38_safety]
     auto ha = htp->add_service(port, proto, (uint32_t) packet_time(), is_new, service);
     if ( is_new )
     {
@@ -311,6 +312,7 @@ void RnaAppDiscovery::update_service_info(const Packet* p, DiscoveryFilter& filt
         return;
 
     // Work on a local copy for eventing purpose
+    //coverity[y2k38_safety]
     ha.last_seen = (uint32_t) packet_time();
 
     if ( proto == IpProtocol::TCP )
@@ -339,6 +341,7 @@ void RnaAppDiscovery::discover_banner(const Packet* p, DiscoveryFilter& filter,
         return;
 
     HostApplication ha(p->flow->server_port, proto, service, false);
+    //coverity[y2k38_safety]
     ha.last_seen = (uint32_t) packet_time();
     logger.log(RNA_EVENT_CHANGE, CHANGE_BANNER_UPDATE, p, &rt,
         (const struct in6_addr*) p->flow->server_ip.get_ip6_ptr(),
@@ -392,12 +395,13 @@ void RnaAppDiscovery::discover_user(const Packet* p, DiscoveryFilter& filter, RN
     if ( !rt )
         return;
 
-    if ( rt->update_service_user(p->flow->server_port, proto, username,
-        (uint32_t) packet_time(), conf ? conf->max_host_services : 0, login_success) )
+    //coverity[y2k38_safety]
+    if ( rt->update_service_user(p->flow->server_port, proto, username, (uint32_t) packet_time(),
+            conf ? conf->max_host_services : 0, login_success) )
     {
         logger.log(RUA_EVENT, login_success ? CHANGE_USER_LOGIN : FAILED_USER_LOGIN,
             p, &rt, (const struct in6_addr*) p->ptrs.ip_api.get_dst()->get_ip6_ptr(),
-            username, service, (uint32_t) packet_time());
+            username, service, packet_time());
     }
 }
 
@@ -415,7 +419,7 @@ void RnaAppDiscovery::discover_netbios_name(const snort::Packet* p, DiscoveryFil
         const uint8_t* src_mac = layer::get_eth_layer(p)->ether_src;
 
         logger.log(RNA_EVENT_CHANGE, CHANGE_NETBIOS_NAME, src_ip_ptr, src_mac,
-            &rt, p, (uint32_t) packet_time(), 0, nullptr, nullptr, nullptr, nullptr, nullptr,
+            &rt, p, packet_time(), 0, nullptr, nullptr, nullptr, nullptr, nullptr,
             nullptr, APP_ID_NONE, nullptr, false, 0, 0, nullptr, nb_name);
     }
 }
index 7e61dd051b689d79378e4a71828039af85ffc190..c1a4876f80cd175a562cf8cd15caedd4151aaba0 100644 (file)
@@ -159,7 +159,7 @@ void RnaLogger::log(uint16_t type, uint16_t subtype, const Packet* p, RnaTracker
 }
 
 void RnaLogger::log(uint16_t type, uint16_t subtype, const Packet* p, RnaTracker* ht,
-   const struct in6_addr* ip, const char* user, AppId appid, uint32_t event_time)
+   const struct in6_addr* ip, const char* user, AppId appid, time_t event_time)
 {
     log(type, subtype, ip, nullptr, ht, p, event_time, 0,
         nullptr, nullptr, nullptr, nullptr, nullptr, user, appid);
@@ -174,32 +174,32 @@ void RnaLogger::log(uint16_t type, uint16_t subtype, const Packet* p, RnaTracker
 
 void RnaLogger::log(uint16_t type, uint16_t subtype, const Packet* p, RnaTracker* ht,
     const struct in6_addr* src_ip, const uint8_t* src_mac, const FpFingerprint* fp,
-    uint32_t event_time, const char* device_info, bool jail_broken)
+    time_t event_time, const char* device_info, bool jail_broken)
 {
     log(type, subtype, src_ip, src_mac, ht, p, event_time, 0, nullptr, nullptr,
         fp, nullptr, nullptr, nullptr, APP_ID_NONE, device_info, jail_broken);
 }
 
 void RnaLogger::log(uint16_t type, uint16_t subtype, const Packet* p, RnaTracker* ht,
-    const struct in6_addr* src_ip, const uint8_t* src_mac, uint32_t event_time)
+    const struct in6_addr* src_ip, const uint8_t* src_mac, time_t event_time)
 {
     log(type, subtype, src_ip, src_mac, ht, p, event_time);
 }
 
 void RnaLogger::log(uint16_t type, uint16_t subtype, const Packet* p, RnaTracker* ht,
-    const struct in6_addr* src_ip, const uint8_t* src_mac, const HostMac* hm, uint32_t event_time)
+    const struct in6_addr* src_ip, const uint8_t* src_mac, const HostMac* hm, time_t event_time)
 {
     log(type, subtype, src_ip, src_mac, ht, p, event_time, 0, hm);
 }
 
 void RnaLogger::log(uint16_t type, uint16_t subtype, const Packet* p, RnaTracker* ht,
-    uint16_t proto, const uint8_t* src_mac, const struct in6_addr* src_ip, uint32_t event_time)
+    uint16_t proto, const uint8_t* src_mac, const struct in6_addr* src_ip, time_t event_time)
 {
     log(type, subtype, src_ip, src_mac, ht, p, event_time, proto);
 }
 
 void RnaLogger::log(uint16_t type, uint16_t subtype, const Packet* p, const uint8_t* src_mac,
-    const struct in6_addr* src_ip, RnaTracker* ht, uint32_t event_time, void* cond_var)
+    const struct in6_addr* src_ip, RnaTracker* ht, time_t event_time, void* cond_var)
 {
     log(type, subtype, src_ip, src_mac, ht, p, event_time, 0,
         nullptr, nullptr, nullptr, cond_var);
@@ -215,7 +215,7 @@ void RnaLogger::log(uint16_t type, uint16_t subtype, const snort::Packet* p, Rna
 
 void RnaLogger::log(uint16_t type, uint16_t subtype, const snort::Packet* p, RnaTracker* ht,
     const struct in6_addr* src_ip, const uint8_t* src_mac, const FpFingerprint* fp,
-    const vector<const char*>* cpeos, uint32_t event_time)
+    const vector<const char*>* cpeos, time_t event_time)
 {
     log(type, subtype, src_ip, src_mac, ht, p, event_time, 0, nullptr, nullptr, fp,
         nullptr, nullptr, nullptr, APP_ID_NONE, nullptr, false, 0, 0, nullptr,
@@ -223,7 +223,7 @@ void RnaLogger::log(uint16_t type, uint16_t subtype, const snort::Packet* p, Rna
 }
 
 bool RnaLogger::log(uint16_t type, uint16_t subtype, const struct in6_addr* src_ip,
-    const uint8_t* src_mac, RnaTracker* ht, const Packet* p, uint32_t event_time,
+    const uint8_t* src_mac, RnaTracker* ht, const Packet* p, time_t event_time,
     uint16_t proto, const HostMac* hm, const HostApplication* ha,
     const FpFingerprint* fp, void* cond_var, const HostClient* hc,
     const char* user, AppId appid, const char* di, bool jb, uint32_t lease,
@@ -244,7 +244,9 @@ bool RnaLogger::log(uint16_t type, uint16_t subtype, const struct in6_addr* src_
 
     if ( event_time )
     {
-        rle.event_time = event_time;
+        //coverity[y2k38_safety]
+        rle.event_time = (uint32_t)event_time;
+        //coverity[y2k38_safety]
         (*ht)->update_last_event(event_time);
     }
 
index 634ecca50324428540c903c4fb1e5e07c1ed4e4a..57e8390c86b5296282077b4d0cc68e9d7c8ed067 100644 (file)
@@ -46,35 +46,35 @@ public:
 
     // for host user
     void log(uint16_t type, uint16_t subtype, const snort::Packet*, RnaTracker*,
-        const struct in6_addr*, const char* user, AppId appid, uint32_t event_time);
+        const struct in6_addr*, const char* user, AppId appid, time_t event_time);
 
     // for cpe os info event
     void log(uint16_t type, uint16_t subtype, const snort::Packet* p, RnaTracker* ht,
         const struct in6_addr* src_ip, const uint8_t* src_mac, const snort::FpFingerprint* fp,
-        const std::vector<const char*>* cpeos, uint32_t event_time);
+        const std::vector<const char*>* cpeos, time_t event_time);
 
     // for fingerprint
     void log(uint16_t type, uint16_t subtype, const snort::Packet* p, RnaTracker* ht,
         const struct in6_addr* src_ip, const uint8_t* src_mac, const snort::FpFingerprint* fp,
-        uint32_t event_time, const char* device_info = nullptr, bool jail_broken = false);
+        time_t event_time, const char* device_info = nullptr, bool jail_broken = false);
 
     // for event time
     void log(uint16_t type, uint16_t subtype, const snort::Packet* p, RnaTracker* ht,
-        const struct in6_addr* src_ip, const uint8_t* src_mac, uint32_t event_time);
+        const struct in6_addr* src_ip, const uint8_t* src_mac, time_t event_time);
 
     // for mac event
     void log(uint16_t type, uint16_t subtype, const snort::Packet* p, RnaTracker* ht,
         const struct in6_addr* src_ip, const uint8_t* src_mac,
-        const snort::HostMac* hm = nullptr, uint32_t event_time = 0);
+        const snort::HostMac* hm = nullptr, time_t event_time = 0);
 
     // for protocol event
     void log(uint16_t type, uint16_t subtype, const snort::Packet* p, RnaTracker* ht,
         uint16_t proto, const uint8_t* mac, const struct in6_addr* ip = nullptr,
-        uint32_t event_time = 0);
+        time_t event_time = 0);
 
     // for timeout update
     void log(uint16_t type, uint16_t subtype, const snort::Packet* p, const uint8_t* src_mac,
-        const struct in6_addr* src_ip, RnaTracker* ht, uint32_t event_time, void* cond_var);
+        const struct in6_addr* src_ip, RnaTracker* ht, time_t event_time, void* cond_var);
 
     // for dhcp info event
     void log(uint16_t type, uint16_t subtype, const snort::Packet* p, RnaTracker* ht,
@@ -84,7 +84,7 @@ public:
     // for all
     bool log(uint16_t type, uint16_t subtype, const struct in6_addr* src_ip,
         const uint8_t* src_mac, RnaTracker* ht, const snort::Packet* p = nullptr,
-        uint32_t event_time = 0, uint16_t proto = 0, const snort::HostMac* hm = nullptr,
+        time_t event_time = 0, uint16_t proto = 0, const snort::HostMac* hm = nullptr,
         const snort::HostApplication* ha = nullptr, const snort::FpFingerprint* fp = nullptr,
         void* cond_var = nullptr, const snort::HostClient* hc = nullptr,
         const char* user = nullptr, AppId appid = APP_ID_NONE, const char* device_info = nullptr,
index 737c5606c1286796e7f1e644b0f3ee7a015300fb..edd29fd904edc921459fdb2e11449ddc67c5d766 100644 (file)
@@ -47,7 +47,10 @@ class HostTrackerMac
 {
 public:
     HostTrackerMac()
-    { last_seen = (uint32_t) snort::packet_time(); }
+    {
+        //coverity[y2k38_safety]
+        last_seen = (uint32_t) snort::packet_time(); 
+    }
 
     bool add_network_proto(const uint16_t type);
     void update_last_seen(uint32_t p_last_seen);
index 05736df2afedfb6e2160f71c82215415e7d6e4ab..3abb936a2cd3a5e3cedbb26159f2ed8f2a041a6a 100644 (file)
@@ -241,7 +241,6 @@ void RnaPnd::analyze_netflow_host(NetFlowEvent* nfe)
     if ( ht->add_xport_proto(ptype) )
         logger.log(RNA_EVENT_NEW, NEW_XPORT_PROTOCOL, p, &ht, ptype, src_mac, src_ip_ptr,
             packet_time());
-
     if ( !new_host )
         generate_change_host_update(&ht, p, &src_ip, src_mac, packet_time());
 }
@@ -269,6 +268,7 @@ void RnaPnd::analyze_netflow_service(NetFlowEvent* nfe)
     ht->update_last_seen();
 
     bool is_new = false;
+    //coverity[y2k38_safety]
     auto ha = ht->add_service(port, proto, (uint32_t) packet_time(), is_new, service);
 
     ht->update_service_info(ha, nullptr, nullptr, conf->max_host_service_info);
@@ -358,7 +358,6 @@ void RnaPnd::discover_network(const Packet* p, uint8_t ttl)
     if ( new_mac and !new_host )
     {
         HostMac hm;
-
         logger.log(RNA_EVENT_CHANGE, CHANGE_MAC_ADD, p, &ht, src_ip_ptr, src_mac,
             ht->get_hostmac(src_mac, hm) ? &hm : nullptr, packet_time());
     }
@@ -612,7 +611,7 @@ void RnaPnd::generate_change_host_update_eth(HostTrackerMac* mt, const Packet* p
     {
         logger.log(RNA_EVENT_CHANGE, CHANGE_HOST_UPDATE, p, src_mac, nullptr,
             &rt, last_seen, (void*) &timestamp);
-
+        //coverity[y2k38_safety]
         mt->update_last_event(sec);
     }
 
@@ -701,7 +700,7 @@ void RnaPnd::discover_network_ethernet(const Packet* p)
     if ( !p->is_eth() )
         return;
 
-    RnaTracker rt = shared_ptr<snort::HostTracker>(new HostTracker());
+    RnaTracker rt = std::make_shared<snort::HostTracker>();
 
     if ( layer::get_arp_layer(p) )
         retval = discover_network_arp(p, &rt);
@@ -841,7 +840,7 @@ int RnaPnd::discover_network_arp(const Packet* p, RnaTracker* ht_ref)
     return 0;
 }
 
-int RnaPnd::discover_network_bpdu(const Packet* p, const uint8_t* data, RnaTracker ht_ref)
+int RnaPnd::discover_network_bpdu(const Packet* p, const uint8_t* data, RnaTracker& ht_ref)
 {
     const uint8_t* dst_mac = layer::get_eth_layer(p)->ether_dst;
 
@@ -858,7 +857,7 @@ int RnaPnd::discover_network_bpdu(const Packet* p, const uint8_t* data, RnaTrack
     return discover_switch(p, ht_ref);
 }
 
-int RnaPnd::discover_switch(const Packet* p, RnaTracker ht_ref)
+int RnaPnd::discover_switch(const Packet* p, RnaTracker& ht_ref)
 {
     bool new_host_mac = false;
     MacKey mk(layer::get_eth_layer(p)->ether_src);
index 64080a7293b7d21d6f77722de7dba5185d6879bd..17c3b3cd9131ef13b00f25e865274a636981b683 100644 (file)
@@ -173,11 +173,11 @@ private:
     // RNA utilities for non-IP packets
     void discover_network_ethernet(const snort::Packet*);
     int discover_network_arp(const snort::Packet*, RnaTracker*);
-    int discover_network_bpdu(const snort::Packet*, const uint8_t* data, RnaTracker);
+    int discover_network_bpdu(const snort::Packet*, const uint8_t* data, RnaTracker&);
     int discover_network_cdp(const snort::Packet*, const uint8_t* data, uint16_t rlen,
         RnaTracker&);
 
-    int discover_switch(const snort::Packet*, RnaTracker);
+    int discover_switch(const snort::Packet*, RnaTracker&);
 
     RnaLogger logger;
     DiscoveryFilter filter;
index a8f8a9dbfebcfe5b4b4d300a3e620f2855c31a6c..a392b3d525aa998b21427502dda335716980685e 100644 (file)
@@ -493,8 +493,7 @@ int UserSession::process(Packet* p)
     if ( !ut.splitter or p->ptrs.decode_flags & DECODE_SOF )
         start(p, flow);
 
-    //coverity[forward_null]
-    if ( p->data && p->dsize )
+    if ( p->data && p->dsize && ut.splitter )
         ut.add_data(p);
 
     if ( p->ptrs.decode_flags & DECODE_EOF )