Merge pull request #3061 in SNORT/snort3 from ~MASHASAN/snort3:rna_aep to master

Squashed commit of the following:

commit 5c077c59fdee7b25811399cb54227134cd1c61cd
Author: Masud Hasan <mashasan@cisco.com>
Date:   Tue Sep 14 22:15:39 2021 -0400

    host_cache: Avoid data race in cache size access

commit eedfb883372e33ff63ffc18b88a4ddca7a6fdefe
Author: Masud Hasan <mashasan@cisco.com>
Date:   Tue Sep 14 22:25:31 2021 -0400

    trough: Avoid data race in file count

commit f114c5c8711041adf50027f3f8982df1bf267126
Author: Masud Hasan <mashasan@cisco.com>
Date:   Tue Sep 14 16:16:22 2021 -0400

    rna: Avoid data races in vlan and mac address

commit 845a8c2c203eea39fea03ef2a437ffbebf9f41c8
Author: Masud Hasan <mashasan@cisco.com>
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 459b42ec39a1e9b8f7422842cb1e3341742f39fb..2a71ead9b8f5a6aea887e66994b40e787d5fa168 100644 (file)
--- 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<std::mutex> 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<size_t> max_size; // Once max_size elements are in the cache, start to
+                                  // remove the least-recently-used elements.
 
     std::atomic<size_t> 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 b5de7e9d3c55d0c48477920025805326487289b7..0cce3e3625de217792c125a52e835c7976ed0ede 100644 (file)
--- 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<std::mutex> 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<std::mutex> 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 a663cb2fa6775b240d56ecad6ea441369bda1f37..376246543dd3edf4b70de2c6758dea13fc0a860a 100644 (file)
--- 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<mutex> 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<mutex> 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<HostMac*> (&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<HostMac*>(max_ttl_hm);
+    return false;
 }
 
 void HostTracker::update_vlan(uint16_t vth_pri_cfi_vlan, uint16_t vth_proto)
 {
+    lock_guard<mutex> 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<mutex> lck(host_tracker_lock);
     return vlan_tag_present;
 }
 
+bool HostTracker::has_same_vlan(uint16_t pvlan)
+{
+    lock_guard<mutex> lck(host_tracker_lock);
+    return vlan_tag_present and ( vlan_tag.vth_pri_cfi_vlan == pvlan );
+}
+
 uint16_t HostTracker::get_vlan()
 {
+    lock_guard<mutex> 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<mutex> 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 02126edbdecc198d36e51ec4785f02b47d0476a9..9017c3a4ea7c7e0cbe211280349113bf1fc1e0f9 100644 (file)
--- 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 a44c5e9f68f9ce3e70a49a920479359ff250159d..7203e9138c2f0abe6066caaf9d0f12a2bb7193ff 100644 (file)
--- 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 3aa8b9e1a8377fa7c7e6c0f402a289ddea14e694..ab1741a5eb5df0dc13dc5b360a3ba586aee33460 100644 (file)
--- 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<mutex> lck(host_tracker_mac_lock);
+    return vlan_tag_present and ( vlan_tag.vth_pri_cfi_vlan == pvlan );
+}
+
 uint16_t HostTrackerMac::get_vlan()
 {
     lock_guard<mutex> 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 391542d2954366d44c378e5c0e6261f2721dbcf6..82e0dfcf2a28c57762df2a200630f3b0959fee1a 100644 (file)
--- 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<uint16_t, HostCacheAllocMac<uint16_t>> get_network_protos()

diff --git a/src/network_inspectors/rna/rna_pnd.cc b/src/network_inspectors/rna/rna_pnd.cc
index b8f6ab028b9c979fa530d481a485d6ba8b84c2d2..e0f1a729f546f6b0938ab656b46a165505068e15 100644 (file)
--- 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 db0f02563cb92a4590c38d1406079fe7d05cb503..df0d91f8e95e43a946fd2f84b39f67b63e733787 100644 (file)
--- a/src/packet_io/trough.cc
+++ b/src/packet_io/trough.cc
@@ -42,7 +42,7 @@ std::string Trough::pcap_filter = "*.*cap*";
 std::vector<std::string>::const_iterator Trough::pcap_queue_iter;
 
 unsigned Trough::pcap_loop_count = 0;
-unsigned Trough::file_count = 0;
+std::atomic<unsigned> 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 8d2a04879e21a372defc72276f3eeae9337b6255..6e3205238820c5c892cb1d94028fbb7b39c382af 100644 (file)
--- a/src/packet_io/trough.h
+++ b/src/packet_io/trough.h
@@ -20,6 +20,7 @@
 #ifndef TROUGH_H
 #define TROUGH_H
 
+#include <atomic>
 #include <string>
 #include <vector>
 
@@ -80,7 +81,7 @@ private:
     static std::string pcap_filter;
 
     static unsigned pcap_loop_count;
-    static unsigned file_count;
+    static std::atomic<unsigned> file_count;
 };
 
 #endif