From: Raza Shafiq (rshafiq) Date: Mon, 19 May 2025 17:29:22 +0000 (+0000) Subject: Pull request #4742: rna: coverity fixes X-Git-Tag: 3.8.1.0~7 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=5810efaf70c89083d46cef60a91f2b666b6ea3d0;p=thirdparty%2Fsnort3.git Pull request #4742: rna: coverity fixes Merge in SNORT/snort3 from ~RSHAFIQ/snort3:cov_rna to master Squashed commit of the following: commit 54f9ee7379d39560e4085b72b5860aa98d4610b4 Author: rshafiq Date: Fri May 2 10:28:27 2025 -0400 rna: coverity fixes --- diff --git a/src/host_tracker/host_tracker.cc b/src/host_tracker/host_tracker.cc index 8c3d602ca..c2b27fd6a 100644 --- a/src/host_tracker/host_tracker.cc +++ b/src/host_tracker/host_tracker.cc @@ -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 lck(host_tracker_lock); + //coverity[y2k38_safety] last_seen = (uint32_t) packet_time(); } diff --git a/src/host_tracker/host_tracker_module.cc b/src/host_tracker/host_tracker_module.cc index 1ecb5dd15..a3fb98756 100644 --- a/src/host_tracker/host_tracker_module.cc +++ b/src/host_tracker/host_tracker_module.cc @@ -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[] = diff --git a/src/main/thread.cc b/src/main/thread.cc index 744f49499..c705feb5d 100644 --- a/src/main/thread.cc +++ b/src/main/thread.cc @@ -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 += '_'; diff --git a/src/managers/module_manager.cc b/src/managers/module_manager.cc index 3c07b32a7..e9cb2654c 100644 --- a/src/managers/module_manager.cc +++ b/src/managers/module_manager.cc @@ -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) ) { diff --git a/src/network_inspectors/perf_monitor/perf_module.h b/src/network_inspectors/perf_monitor/perf_module.h index ce5f0be38..234c5af59 100644 --- a/src/network_inspectors/perf_monitor/perf_module.h +++ b/src/network_inspectors/perf_monitor/perf_module.h @@ -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 pegs; void set_name(const std::string& name); diff --git a/src/network_inspectors/perf_monitor/perf_tracker.cc b/src/network_inspectors/perf_monitor/perf_tracker.cc index cb0088bd9..5554c213e 100644 --- a/src/network_inspectors/perf_monitor/perf_tracker.cc +++ b/src/network_inspectors/perf_monitor/perf_tracker.cc @@ -24,9 +24,10 @@ #include "perf_tracker.h" -#include - #include +#include +#include +#include #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; } diff --git a/src/network_inspectors/rna/rna_app_discovery.cc b/src/network_inspectors/rna/rna_app_discovery.cc index 171ba1d36..e2d9aa3ec 100644 --- a/src/network_inspectors/rna/rna_app_discovery.cc +++ b/src/network_inspectors/rna/rna_app_discovery.cc @@ -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); } } diff --git a/src/network_inspectors/rna/rna_logger.cc b/src/network_inspectors/rna/rna_logger.cc index 7e61dd051..c1a4876f8 100644 --- a/src/network_inspectors/rna/rna_logger.cc +++ b/src/network_inspectors/rna/rna_logger.cc @@ -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* cpeos, uint32_t event_time) + const vector* 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); } diff --git a/src/network_inspectors/rna/rna_logger.h b/src/network_inspectors/rna/rna_logger.h index 634ecca50..57e8390c8 100644 --- a/src/network_inspectors/rna/rna_logger.h +++ b/src/network_inspectors/rna/rna_logger.h @@ -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* cpeos, uint32_t event_time); + const std::vector* 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, diff --git a/src/network_inspectors/rna/rna_mac_cache.h b/src/network_inspectors/rna/rna_mac_cache.h index 737c5606c..edd29fd90 100644 --- a/src/network_inspectors/rna/rna_mac_cache.h +++ b/src/network_inspectors/rna/rna_mac_cache.h @@ -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); diff --git a/src/network_inspectors/rna/rna_pnd.cc b/src/network_inspectors/rna/rna_pnd.cc index 05736df2a..3abb936a2 100644 --- a/src/network_inspectors/rna/rna_pnd.cc +++ b/src/network_inspectors/rna/rna_pnd.cc @@ -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*) ×tamp); - + //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(new HostTracker()); + RnaTracker rt = std::make_shared(); 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); diff --git a/src/network_inspectors/rna/rna_pnd.h b/src/network_inspectors/rna/rna_pnd.h index 64080a729..17c3b3cd9 100644 --- a/src/network_inspectors/rna/rna_pnd.h +++ b/src/network_inspectors/rna/rna_pnd.h @@ -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; diff --git a/src/stream/user/user_session.cc b/src/stream/user/user_session.cc index a8f8a9dbf..a392b3d52 100644 --- a/src/stream/user/user_session.cc +++ b/src/stream/user/user_session.cc @@ -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 )