From: Masud Hasan (mashasan) Date: Mon, 20 Sep 2021 19:32:53 +0000 (+0000) Subject: Merge pull request #3061 in SNORT/snort3 from ~MASHASAN/snort3:rna_aep to master X-Git-Tag: 3.1.13.0~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e4b42c44a423afdabc5d9fb511921f94337a4eea;p=thirdparty%2Fsnort3.git Merge pull request #3061 in SNORT/snort3 from ~MASHASAN/snort3:rna_aep to master Squashed commit of the following: commit 5c077c59fdee7b25811399cb54227134cd1c61cd Author: Masud Hasan Date: Tue Sep 14 22:15:39 2021 -0400 host_cache: Avoid data race in cache size access commit eedfb883372e33ff63ffc18b88a4ddca7a6fdefe Author: Masud Hasan Date: Tue Sep 14 22:25:31 2021 -0400 trough: Avoid data race in file count commit f114c5c8711041adf50027f3f8982df1bf267126 Author: Masud Hasan Date: Tue Sep 14 16:16:22 2021 -0400 rna: Avoid data races in vlan and mac address commit 845a8c2c203eea39fea03ef2a437ffbebf9f41c8 Author: Masud Hasan Date: Tue Sep 14 12:48:37 2021 -0400 rna: Avoid infinite loop in ICMPv6 options --- diff --git a/src/hash/lru_cache_shared.h b/src/hash/lru_cache_shared.h index 459b42ec3..2a71ead9b 100644 --- a/src/hash/lru_cache_shared.h +++ b/src/hash/lru_cache_shared.h @@ -109,7 +109,6 @@ public: size_t get_max_size() { - std::lock_guard cache_lock(cache_mutex); return max_size; } @@ -146,8 +145,8 @@ protected: static constexpr size_t mem_chunk = sizeof(Data) + sizeof(Value); - size_t max_size; // Once max_size elements are in the cache, start to - // remove the least-recently-used elements. + std::atomic max_size; // Once max_size elements are in the cache, start to + // remove the least-recently-used elements. std::atomic current_size;// Number of entries currently in the cache. diff --git a/src/host_tracker/host_cache.h b/src/host_tracker/host_cache.h index b5de7e9d3..0cce3e362 100644 --- a/src/host_tracker/host_cache.h +++ b/src/host_tracker/host_cache.h @@ -92,10 +92,8 @@ public: { if ( snort::SnortConfig::log_verbose() ) { - std::lock_guard cache_lock(cache_mutex); - snort::LogLabel("host_cache"); - snort::LogMessage(" memcap: %zu bytes\n", max_size); + snort::LogMessage(" memcap: %zu bytes\n", max_size.load()); } } @@ -107,7 +105,6 @@ public: if ( current_size > new_size ) return true; - std::lock_guard cache_lock(cache_mutex); max_size = new_size; return false; } @@ -132,9 +129,7 @@ public: if ( !list.empty() ) { - // A data race when reload changes max_size while other threads read this may - // delay pruning by one round. Yet, we are avoiding mutex for better performance. - max_size = current_size; + max_size.store(current_size); if ( max_size > new_size ) { LruListIter list_iter = --list.end(); diff --git a/src/host_tracker/host_tracker.cc b/src/host_tracker/host_tracker.cc index a663cb2fa..376246543 100644 --- a/src/host_tracker/host_tracker.cc +++ b/src/host_tracker/host_tracker.cc @@ -214,7 +214,7 @@ bool HostTracker::get_hostmac(const uint8_t* mac, HostMac& hm) return false; } -const uint8_t* HostTracker::get_last_seen_mac() +const uint8_t* HostTracker::get_last_seen_mac(uint8_t* mac_addr) { lock_guard lck(host_tracker_lock); const HostMac_t* max_hm = nullptr; @@ -225,7 +225,10 @@ const uint8_t* HostTracker::get_last_seen_mac() max_hm = &hm; if ( max_hm ) - return max_hm->mac; + { + memcpy(mac_addr, max_hm->mac, MAC_SIZE); + return mac_addr; + } return zero_mac; } @@ -284,30 +287,25 @@ bool HostTracker::make_primary(const uint8_t* mac) return false; } -HostMac* HostTracker::get_max_ttl_hostmac() +bool HostTracker::reset_hops_if_primary() { lock_guard lck(host_tracker_lock); - HostMac_t* max_ttl_hm = nullptr; - uint8_t max_ttl = 0; - for ( auto& hm : macs ) - { if ( hm.primary and hm.visibility ) - return static_cast (&hm); - - if ( hm.ttl > max_ttl and hm.visibility ) { - max_ttl = hm.ttl; - max_ttl_hm = &hm; + if ( !hops ) + return false; + hops = 0; + return true; } - } - return static_cast(max_ttl_hm); + return false; } void HostTracker::update_vlan(uint16_t vth_pri_cfi_vlan, uint16_t vth_proto) { + lock_guard lck(host_tracker_lock); vlan_tag_present = true; vlan_tag.vth_pri_cfi_vlan = vth_pri_cfi_vlan; vlan_tag.vth_proto = vth_proto; @@ -315,16 +313,25 @@ void HostTracker::update_vlan(uint16_t vth_pri_cfi_vlan, uint16_t vth_proto) bool HostTracker::has_vlan() { + lock_guard lck(host_tracker_lock); return vlan_tag_present; } +bool HostTracker::has_same_vlan(uint16_t pvlan) +{ + lock_guard lck(host_tracker_lock); + return vlan_tag_present and ( vlan_tag.vth_pri_cfi_vlan == pvlan ); +} + uint16_t HostTracker::get_vlan() { + lock_guard lck(host_tracker_lock); return vlan_tag.vth_pri_cfi_vlan; } void HostTracker::get_vlan_details(uint8_t& cfi, uint8_t& priority, uint16_t& vid) { + lock_guard lck(host_tracker_lock); cfi = vlan_tag.cfi(); priority = vlan_tag.priority(); vid = vlan_tag.vid(); diff --git a/src/host_tracker/host_tracker.h b/src/host_tracker/host_tracker.h index 02126edbd..9017c3a4e 100644 --- a/src/host_tracker/host_tracker.h +++ b/src/host_tracker/host_tracker.h @@ -286,16 +286,17 @@ public: bool add_payload(HostApplication&, Port, IpProtocol, const AppId payload, const AppId service, size_t max_payloads); - // Returns the hostmac pointer with the highest TTL - HostMac* get_max_ttl_hostmac(); + // Returns true after resetting hops if there is a primary mac + bool reset_hops_if_primary(); // Returns true and copy of the matching HostMac, false if no match... bool get_hostmac(const uint8_t* mac, HostMac& hm); - const uint8_t* get_last_seen_mac(); + const uint8_t* get_last_seen_mac(uint8_t*); void update_vlan(uint16_t vth_pri_cfi_vlan, uint16_t vth_proto); bool has_vlan(); + bool has_same_vlan(uint16_t); uint16_t get_vlan(); void get_vlan_details(uint8_t& cfi, uint8_t& priority, uint16_t& vid); diff --git a/src/network_inspectors/rna/rna_app_discovery.cc b/src/network_inspectors/rna/rna_app_discovery.cc index a44c5e9f6..7203e9138 100644 --- a/src/network_inspectors/rna/rna_app_discovery.cc +++ b/src/network_inspectors/rna/rna_app_discovery.cc @@ -32,6 +32,8 @@ using namespace snort; +static THREAD_LOCAL uint8_t mac_addr[MAC_SIZE]; + RnaTracker RnaAppDiscovery::get_server_rna_tracker(const Packet* p, RNAFlow* rna_flow) { return rna_flow->get_server(p->flow->server_ip); @@ -204,10 +206,10 @@ bool RnaAppDiscovery::discover_service(const Packet* p, DiscoveryFilter& filter, { if ( proto == IpProtocol::TCP ) logger.log(RNA_EVENT_NEW, NEW_TCP_SERVICE, p, &htp, - (const struct in6_addr*) ip.get_ip6_ptr(), htp->get_last_seen_mac(), &ha); + (const struct in6_addr*) ip.get_ip6_ptr(), htp->get_last_seen_mac(mac_addr), &ha); else logger.log(RNA_EVENT_NEW, NEW_UDP_SERVICE, p, &htp, - (const struct in6_addr*) ip.get_ip6_ptr(), htp->get_last_seen_mac(), &ha); + (const struct in6_addr*) ip.get_ip6_ptr(), htp->get_last_seen_mac(mac_addr), &ha); ha.hits = 0; // hit count is reset after logs are written htp->update_service(ha); @@ -250,11 +252,11 @@ void RnaAppDiscovery::discover_payload(const Packet* p, DiscoveryFilter& filter, if ( proto == IpProtocol::TCP ) logger.log(RNA_EVENT_CHANGE, CHANGE_TCP_SERVICE_INFO, p, &srt, (const struct in6_addr*) p->flow->server_ip.get_ip6_ptr(), - srt->get_last_seen_mac(), &local_ha); + srt->get_last_seen_mac(mac_addr), &local_ha); else logger.log(RNA_EVENT_CHANGE, CHANGE_UDP_SERVICE_INFO, p, &srt, (const struct in6_addr*) p->flow->server_ip.get_ip6_ptr(), - srt->get_last_seen_mac(), &local_ha); + srt->get_last_seen_mac(mac_addr), &local_ha); } } @@ -275,7 +277,7 @@ void RnaAppDiscovery::discover_payload(const Packet* p, DiscoveryFilter& filter, if ( new_client_payload ) logger.log(RNA_EVENT_CHANGE, CHANGE_CLIENT_APP_UPDATE, p, &crt, (const struct in6_addr*) p->flow->client_ip.get_ip6_ptr(), - crt->get_last_seen_mac(), &hc); + crt->get_last_seen_mac(mac_addr), &hc); } void RnaAppDiscovery::update_service_info(const Packet* p, DiscoveryFilter& filter, @@ -312,10 +314,10 @@ void RnaAppDiscovery::update_service_info(const Packet* p, DiscoveryFilter& filt if ( proto == IpProtocol::TCP ) logger.log(RNA_EVENT_CHANGE, CHANGE_TCP_SERVICE_INFO, p, &htp, - (const struct in6_addr*) ip.get_ip6_ptr(), htp->get_last_seen_mac(), &ha); + (const struct in6_addr*) ip.get_ip6_ptr(), htp->get_last_seen_mac(mac_addr), &ha); else logger.log(RNA_EVENT_CHANGE, CHANGE_UDP_SERVICE_INFO, p, &htp, - (const struct in6_addr*) ip.get_ip6_ptr(), htp->get_last_seen_mac(), &ha); + (const struct in6_addr*) ip.get_ip6_ptr(), htp->get_last_seen_mac(mac_addr), &ha); ha.hits = 0; htp->update_service(ha); @@ -338,7 +340,8 @@ void RnaAppDiscovery::discover_banner(const Packet* p, DiscoveryFilter& filter, HostApplication ha(p->flow->server_port, proto, service, false); 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(), rt->get_last_seen_mac(), &ha); + (const struct in6_addr*) p->flow->server_ip.get_ip6_ptr(), + rt->get_last_seen_mac(mac_addr), &ha); } void RnaAppDiscovery::discover_client(const Packet* p, DiscoveryFilter& filter, RNAFlow* rna_flow, @@ -365,7 +368,7 @@ void RnaAppDiscovery::discover_client(const Packet* p, DiscoveryFilter& filter, RnaTracker crt = get_client_rna_tracker(p, rna_flow); if ( !crt or !crt->is_visible() ) return; - mac = crt->get_last_seen_mac(); + mac = crt->get_last_seen_mac(mac_addr); } if (conf and conf->max_host_client_apps and @@ -438,7 +441,8 @@ void RnaAppDiscovery::analyze_user_agent_fingerprint(const Packet* p, DiscoveryF device_info, MAX_USER_AGENT_DEVICES) ) { logger.log(RNA_EVENT_NEW, NEW_OS, p, &rt, - (const struct in6_addr*)p->flow->client_ip.get_ip6_ptr(), rt->get_last_seen_mac(), - (FpFingerprint*)uafp, packet_time(), device_info, jail_broken); + (const struct in6_addr*)p->flow->client_ip.get_ip6_ptr(), + rt->get_last_seen_mac(mac_addr), (FpFingerprint*)uafp, packet_time(), + device_info, jail_broken); } } diff --git a/src/network_inspectors/rna/rna_mac_cache.cc b/src/network_inspectors/rna/rna_mac_cache.cc index 3aa8b9e1a..ab1741a5e 100644 --- a/src/network_inspectors/rna/rna_mac_cache.cc +++ b/src/network_inspectors/rna/rna_mac_cache.cc @@ -63,6 +63,12 @@ bool HostTrackerMac::has_vlan() return vlan_tag_present; } +bool HostTrackerMac::has_same_vlan(uint16_t pvlan) +{ + lock_guard lck(host_tracker_mac_lock); + return vlan_tag_present and ( vlan_tag.vth_pri_cfi_vlan == pvlan ); +} + uint16_t HostTrackerMac::get_vlan() { lock_guard lck(host_tracker_mac_lock); diff --git a/src/network_inspectors/rna/rna_mac_cache.h b/src/network_inspectors/rna/rna_mac_cache.h index 391542d29..82e0dfcf2 100644 --- a/src/network_inspectors/rna/rna_mac_cache.h +++ b/src/network_inspectors/rna/rna_mac_cache.h @@ -53,6 +53,7 @@ public: void update_last_seen(uint32_t p_last_seen); void update_vlan(uint16_t vth_pri_cfi_vlan, uint16_t vth_proto); bool has_vlan(); + bool has_same_vlan(uint16_t); void get_vlan_details(uint8_t& cfi, uint8_t& priority, uint16_t& vid); std::vector> get_network_protos() diff --git a/src/network_inspectors/rna/rna_pnd.cc b/src/network_inspectors/rna/rna_pnd.cc index b8f6ab028..e0f1a729f 100644 --- a/src/network_inspectors/rna/rna_pnd.cc +++ b/src/network_inspectors/rna/rna_pnd.cc @@ -228,12 +228,8 @@ void RnaPnd::discover_network(const Packet* p, uint8_t ttl) logger.log(RNA_EVENT_CHANGE, CHANGE_MAC_INFO, p, &ht, src_ip_ptr, src_mac, ht->get_hostmac(src_mac, hm) ? &hm : nullptr, packet_time()); - HostMac* hm_max_ttl = ht->get_max_ttl_hostmac(); - if (hm_max_ttl and hm_max_ttl->primary and ht->get_hops()) - { - ht->update_hops(0); + if ( ht->reset_hops_if_primary() ) logger.log(RNA_EVENT_CHANGE, CHANGE_HOPS, p, &ht, src_ip_ptr, src_mac, packet_time()); - } } if ( p->is_tcp() and ht->get_host_type() == HOST_TYPE_HOST ) @@ -407,7 +403,7 @@ void RnaPnd::generate_change_vlan_update(RnaTracker *rt, const Packet* p, if ( !vh ) return; - if ( isnew or !hm.has_vlan() or (hm.get_vlan() != vh->vth_pri_cfi_vlan) ) + if ( isnew or !hm.has_same_vlan(vh->vth_pri_cfi_vlan) ) { if ( !isnew ) update_vlan(p, hm); @@ -427,7 +423,7 @@ void RnaPnd::generate_change_vlan_update(RnaTracker *rt, const Packet* p, if ( !vh ) return; - if ( isnew or !rt->get()->has_vlan() or (rt->get()->get_vlan() != vh->vth_pri_cfi_vlan) ) + if ( isnew or !rt->get()->has_same_vlan(vh->vth_pri_cfi_vlan) ) { rt->get()->update_vlan(vh->vth_pri_cfi_vlan, vh->vth_proto); logger.log(RNA_EVENT_CHANGE, CHANGE_VLAN_TAG, p, rt, @@ -916,13 +912,13 @@ int RnaPnd::discover_host_types_icmpv6_ndp(RnaTracker& ht, const Packet* p, uint while ( data_len >= 2 ) { - uint8_t opt_type, opt_len; - - opt_type = *data; - opt_len = *(data + 1); + uint8_t opt_type = *data; if ( opt_type == ICMPV6_OPTION_TARGET_LINKLAYER_ADDRESS ) neighbor_src_mac = data + 2; + uint8_t opt_len = *(data + 1); + if ( opt_len == 0 ) + break; data += opt_len * 8; data_len -= opt_len * 8; } @@ -938,13 +934,13 @@ int RnaPnd::discover_host_types_icmpv6_ndp(RnaTracker& ht, const Packet* p, uint while ( data_len >= 2 ) { - uint8_t opt_type, opt_len; - - opt_type = *data; - opt_len = *(data + 1); + uint8_t opt_type = *data; if ( opt_type == ICMPV6_OPTION_SOURCE_LINKLAYER_ADDRESS ) neighbor_src_mac = data + 2; + uint8_t opt_len = *(data + 1); + if ( opt_len == 0 ) + break; data += opt_len * 8; data_len -= opt_len * 8; } diff --git a/src/packet_io/trough.cc b/src/packet_io/trough.cc index db0f02563..df0d91f8e 100644 --- a/src/packet_io/trough.cc +++ b/src/packet_io/trough.cc @@ -42,7 +42,7 @@ std::string Trough::pcap_filter = "*.*cap*"; std::vector::const_iterator Trough::pcap_queue_iter; unsigned Trough::pcap_loop_count = 0; -unsigned Trough::file_count = 0; +std::atomic Trough::file_count{0}; bool Trough::add_pcaps_dir(const std::string& dirname, const std::string& filter) { diff --git a/src/packet_io/trough.h b/src/packet_io/trough.h index 8d2a04879..6e3205238 100644 --- a/src/packet_io/trough.h +++ b/src/packet_io/trough.h @@ -20,6 +20,7 @@ #ifndef TROUGH_H #define TROUGH_H +#include #include #include @@ -80,7 +81,7 @@ private: static std::string pcap_filter; static unsigned pcap_loop_count; - static unsigned file_count; + static std::atomic file_count; }; #endif