From: Masud Hasan (mashasan) Date: Tue, 10 Nov 2020 20:10:29 +0000 (+0000) Subject: Merge pull request #2602 in SNORT/snort3 from ~MMATIRKO/snort3:delete_pld to master X-Git-Tag: 3.0.3-5~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7bb3796c131005f849e64f2f68b715f844d3e2bc;p=thirdparty%2Fsnort3.git Merge pull request #2602 in SNORT/snort3 from ~MMATIRKO/snort3:delete_pld to master Squashed commit of the following: commit 9ce30c2e4c67083106e3d5b3ccacc1c58cf6c3a6 Author: Michael Matirko Date: Wed Nov 4 12:28:30 2020 -0500 rna: delete payloads when clients, services are deleted; add unit tests --- diff --git a/src/host_tracker/host_tracker.cc b/src/host_tracker/host_tracker.cc index 97341db79..7d5f1662f 100644 --- a/src/host_tracker/host_tracker.cc +++ b/src/host_tracker/host_tracker.cc @@ -56,7 +56,7 @@ bool HostTracker::add_network_proto(const uint16_t type) { if ( proto.first == type ) { - if (proto.second == true) + if ( proto.second ) return false; else { @@ -78,7 +78,7 @@ bool HostTracker::add_xport_proto(const uint8_t type) { if ( proto.first == type ) { - if (proto.second == true) + if ( proto.second ) return false; else { @@ -114,11 +114,11 @@ bool HostTracker::add_mac(const uint8_t* mac, uint8_t ttl, uint8_t primary) return true; } - if (!invisible_swap_candidate and !hm_t.visibility) + if ( !invisible_swap_candidate and !hm_t.visibility ) invisible_swap_candidate = &hm_t; } - if (invisible_swap_candidate) + if ( invisible_swap_candidate ) { memcpy(invisible_swap_candidate->mac, mac, MAC_SIZE); invisible_swap_candidate->ttl = ttl; @@ -136,14 +136,42 @@ bool HostTracker::add_mac(const uint8_t* mac, uint8_t ttl, uint8_t primary) bool HostTracker::add_payload_no_lock(const AppId pld, HostApplication* ha, size_t max_payloads) { - if ( max_payloads and ha->payloads.size() >= max_payloads ) + Payload_t* invisible_swap_candidate = nullptr; + + for ( auto& p : ha->payloads ) + { + if ( p.first == pld ) + { + if ( p.second ) + { + return false; + } + else + { + p.second = true; + ha->num_visible_payloads++; + return true; + } + } + + if ( !invisible_swap_candidate and !p.second ) + invisible_swap_candidate = &p; + } + + if ( invisible_swap_candidate ) + { + invisible_swap_candidate->first = pld; + invisible_swap_candidate->second = true; + ha->num_visible_payloads++; + return true; + } + + if ( ha->payloads.size() >= max_payloads ) return false; - for ( const auto& app : ha->payloads ) - if ( app == pld ) - return false; + ha->payloads.emplace_back(pld, true); + ha->num_visible_payloads++; - ha->payloads.emplace_back(pld); return true; } @@ -347,23 +375,43 @@ void HostTracker::clear_service(HostApplication& ha) bool HostTracker::add_client_payload(HostClient& hc, AppId payload, size_t max_payloads) { + Payload_t* invisible_swap_candidate = nullptr; std::lock_guard lck(host_tracker_lock); for ( auto& client : clients ) if ( client.id == hc.id and client.service == hc.service ) { - if ( max_payloads and client.payloads.size() >= max_payloads ) - return false; + for ( auto& pld : client.payloads ) + { + if ( pld.first == payload ) + { + if ( pld.second ) + return false; - for (const auto& pld : client.payloads) + pld.second = true; + client.num_visible_payloads++; + return true; + } + if ( !invisible_swap_candidate and !pld.second ) + invisible_swap_candidate = &pld; + } + + if ( invisible_swap_candidate ) { - if ( pld == payload ) - return false; + invisible_swap_candidate->second = true; + invisible_swap_candidate->first = payload; + client.num_visible_payloads++; + hc.payloads = client.payloads; + return true; } - client.payloads.emplace_back(payload); + if ( client.payloads.size() >= max_payloads ) + return false; + + client.payloads.emplace_back(payload, true); hc.payloads = client.payloads; strncpy(hc.version, client.version, INFO_SIZE); + client.num_visible_payloads++; return true; } @@ -387,7 +435,7 @@ bool HostTracker::add_service(const HostApplication& app, bool* added) *added = true; } - if (s.visibility == false) + if ( s.visibility == false ) { if (added) *added = true; @@ -401,7 +449,7 @@ bool HostTracker::add_service(const HostApplication& app, bool* added) services.emplace_back(app.port, app.proto, app.appid, app.inferred_appid); num_visible_services++; - if (added) + if ( added ) *added = true; return true; @@ -454,7 +502,7 @@ bool HostTracker::add_payload(HostApplication& local_ha, Port port, IpProtocol p auto ha = find_service_no_lock(port, proto, service); - if (ha) + if ( ha ) { bool success = add_payload_no_lock(payload, ha, max_payloads); local_ha = *ha; @@ -566,13 +614,13 @@ void HostTracker::update_service_proto(HostApplication& app, IpProtocol proto) void HostTracker::update_ha_no_lock(HostApplication& dst, HostApplication& src) { - if (dst.appid == APP_ID_NONE) + if ( dst.appid == APP_ID_NONE ) dst.appid = src.appid; else src.appid = dst.appid; - for (auto& i: src.info) - if (i.visibility == true) + for ( auto& i: src.info ) + if ( i.visibility == true ) dst.info.emplace_back(i.version, i.vendor); dst.hits = src.hits; @@ -597,12 +645,12 @@ bool HostTracker::update_service_info(HostApplication& ha, const char* vendor, HostApplicationInfo* available = nullptr; for ( auto& i : s.info ) { - if (((!version and i.version[0] == '\0') or + if ( ((!version and i.version[0] == '\0') or (version and !strncmp(version, i.version, INFO_SIZE))) and ((!vendor and i.vendor[0] == '\0') or - (vendor and !strncmp(vendor, i.vendor, INFO_SIZE)))) + (vendor and !strncmp(vendor, i.vendor, INFO_SIZE))) ) { - if (i.visibility == false) + if ( i.visibility == false ) { i.visibility = true; // rediscover it update_ha_no_lock(ha, s); @@ -708,7 +756,7 @@ bool HostTracker::set_visibility(bool v) visibility = v; - if (visibility == false) + if ( visibility == false ) { for (auto& proto : network_protos) proto.second = false; @@ -727,11 +775,15 @@ bool HostTracker::set_visibility(bool v) for (auto& info : s.info) info.visibility = false; s.user[0] = '\0'; + set_payload_visibility_no_lock(s.payloads, v, s.num_visible_payloads); } num_visible_services = 0; for ( auto& c : clients ) + { c.visibility = false; + set_payload_visibility_no_lock(c.payloads, v, c.num_visible_payloads); + } num_visible_clients = 0; tcp_fpids.clear(); @@ -744,9 +796,9 @@ bool HostTracker::set_visibility(bool v) bool HostTracker::set_network_proto_visibility(uint16_t proto, bool v) { std::lock_guard lck(host_tracker_lock); - for (auto& pp : network_protos) + for ( auto& pp : network_protos ) { - if (pp.first == proto) + if ( pp.first == proto ) { pp.second = v; return true; @@ -758,9 +810,9 @@ bool HostTracker::set_network_proto_visibility(uint16_t proto, bool v) bool HostTracker::set_xproto_visibility(uint8_t proto, bool v) { std::lock_guard lck(host_tracker_lock); - for (auto& pp : xport_protos) + for ( auto& pp : xport_protos ) { - if (pp.first == proto) + if ( pp.first == proto ) { pp.second = v; return true; @@ -769,6 +821,21 @@ bool HostTracker::set_xproto_visibility(uint8_t proto, bool v) return false; } +void HostTracker::set_payload_visibility_no_lock(PayloadVector& pv, bool v, size_t& num_vis) +{ + for ( auto& p : pv ) + { + if ( p.second != v ) + { + p.second = v; + if ( v ) + num_vis++; + else + num_vis--; + } + } +} + bool HostTracker::set_service_visibility(Port port, IpProtocol proto, bool v) { std::lock_guard lck(host_tracker_lock); @@ -792,6 +859,8 @@ bool HostTracker::set_service_visibility(Port port, IpProtocol proto, bool v) s.user[0] = '\0'; s.banner_updated = false; } + + set_payload_visibility_no_lock(s.payloads, v, s.num_visible_payloads); return true; } } @@ -811,10 +880,11 @@ bool HostTracker::set_client_visibility(const HostClient& hc, bool v) assert(num_visible_clients > 0 ); num_visible_clients--; } - else if (c.visibility == false and v == true) + else if ( c.visibility == false and v == true ) num_visible_clients++; c.visibility = v; + set_payload_visibility_no_lock(c.payloads, v, c.num_visible_payloads); deleted = true; } } @@ -862,7 +932,7 @@ size_t HostTracker::get_client_count() HostClient::HostClient(AppId clientid, const char *ver, AppId ser) : id(clientid), service(ser) { - if (ver) + if ( ver ) { strncpy(version, ver, INFO_SIZE); version[INFO_SIZE-1] = '\0'; @@ -889,7 +959,7 @@ HostClient HostTracker::find_or_add_client(AppId id, const char* version, AppId return c; } - else if ( !available and !c.visibility) + else if ( !available and !c.visibility ) available = &c; } @@ -914,12 +984,12 @@ HostClient HostTracker::find_or_add_client(AppId id, const char* version, AppId HostApplicationInfo::HostApplicationInfo(const char *ver, const char *ven) { - if (ver) + if ( ver ) { strncpy(version, ver, INFO_SIZE); version[INFO_SIZE-1] = '\0'; } - if (ven) + if ( ven ) { strncpy(vendor, ven, INFO_SIZE); vendor[INFO_SIZE-1] = '\0'; @@ -1002,14 +1072,15 @@ void HostTracker::stringify(string& str) str += ", version: " + string(i.version); } - auto total_payloads = s.payloads.size(); - if ( total_payloads > 0 ) + auto vis_payloads = s.num_visible_payloads; + if ( vis_payloads > 0 ) { str += ", payload"; - str += (total_payloads > 1) ? "s: " : ": "; + str += (vis_payloads > 1) ? "s: " : ": "; for ( const auto& pld : s.payloads ) { - str += to_string(pld) + (--total_payloads ? ", " : ""); + if ( pld.second ) + str += to_string(pld.first) + (--vis_payloads ? ", " : ""); } } if ( *s.user ) @@ -1022,7 +1093,7 @@ void HostTracker::stringify(string& str) str += "\nclients size: " + to_string(num_visible_clients); for ( const auto& c : clients ) { - if (c.visibility == false) + if ( c.visibility == false ) continue; str += "\n id: " + to_string(c.id) @@ -1030,13 +1101,16 @@ void HostTracker::stringify(string& str) if ( c.version[0] != '\0' ) str += ", version: " + string(c.version); - auto total_payloads = c.payloads.size(); - if ( total_payloads ) + auto vis_payloads = c.num_visible_payloads; + if ( vis_payloads ) { str += ", payload"; - str += (total_payloads > 1) ? "s: " : ": "; + str += (vis_payloads > 1) ? "s: " : ": "; for ( const auto& pld : c.payloads ) - str += to_string(pld) + (--total_payloads ? ", " : ""); + { + if ( pld.second ) + str += to_string(pld.first) + (--vis_payloads ? ", " : ""); + } } } } diff --git a/src/host_tracker/host_tracker.h b/src/host_tracker/host_tracker.h index 77a0ffb9b..deb4fe243 100644 --- a/src/host_tracker/host_tracker.h +++ b/src/host_tracker/host_tracker.h @@ -81,7 +81,8 @@ struct HostApplicationInfo }; typedef HostCacheAllocIp HostAppInfoAllocator; -typedef AppId Payload_t; +typedef std::pair Payload_t; +typedef std::vector> PayloadVector; struct HostApplication { @@ -90,8 +91,10 @@ struct HostApplication bool banner = false) : port(pt), proto(pr), appid(ap), inferred_appid(in), hits(ht), last_seen(ls), banner_updated(banner) { } HostApplication(const HostApplication& ha): port(ha.port), proto(ha.proto), appid(ha.appid), - inferred_appid(ha.inferred_appid), hits(ha.hits), last_seen(ha.last_seen), info(ha.info), - payloads(ha.payloads) { } + inferred_appid(ha.inferred_appid), hits(ha.hits), last_seen(ha.last_seen), + num_visible_payloads(ha.num_visible_payloads), info(ha.info), payloads(ha.payloads), + visibility(ha.visibility) { } + HostApplication& operator=(const HostApplication& ha) { port = ha.port; @@ -104,6 +107,7 @@ struct HostApplication payloads = ha.payloads; visibility = ha.visibility; banner_updated = ha.banner_updated; + num_visible_payloads = ha.num_visible_payloads; return *this; } @@ -115,14 +119,19 @@ struct HostApplication uint32_t last_seen = 0; char user[INFO_SIZE] = { '\0' }; bool banner_updated = false; + size_t num_visible_payloads = 0; std::vector info; - std::vector> payloads; + PayloadVector payloads; friend class HostTracker; +// visibility is public in UT only, to avoid extra lock/unlock funcs used only by UT +#ifndef UNIT_TEST private: +#endif bool visibility = true; + }; struct HostClient @@ -132,7 +141,8 @@ struct HostClient AppId id; char version[INFO_SIZE] = { '\0' }; AppId service; - std::vector> payloads; + PayloadVector payloads; + size_t num_visible_payloads = 0; bool operator==(const HostClient& c) const { @@ -385,6 +395,22 @@ public: bool set_service_visibility(Port, IpProtocol, bool v = true); bool set_client_visibility(const HostClient&, bool v = true); +#ifdef UNIT_TEST + // Caller is responsible for checking visibility + std::vector get_services() + { + std::lock_guard lck(host_tracker_lock); + return services; + } + + // Caller is responsible for checking visibility + std::vector get_clients() + { + std::lock_guard lck(host_tracker_lock); + return clients; + } +#endif + private: mutable std::mutex host_tracker_lock; // ensure that updates to a shared object are safe uint8_t hops; // hops from the snort inspector, e.g., zero for ARP @@ -423,6 +449,9 @@ private: HostApplication* find_and_add_service_no_lock(Port, IpProtocol, uint32_t lseen, bool& is_new, AppId, uint16_t max_services = 0); + // Sets all payloads visible or invisible + void set_payload_visibility_no_lock(PayloadVector& pv, bool v, size_t& num_vis); + // Hide / delete the constructor from the outside world. We don't want to // have zombie host trackers, i.e. host tracker objects that live outside // the host cache. diff --git a/src/host_tracker/test/host_tracker_test.cc b/src/host_tracker/test/host_tracker_test.cc index 9e108b646..cacc64faa 100644 --- a/src/host_tracker/test/host_tracker_test.cc +++ b/src/host_tracker/test/host_tracker_test.cc @@ -76,6 +76,218 @@ TEST(host_tracker, add_find_service_test) CHECK(APP_ID_NONE == ht.get_appid(8080, IpProtocol::UDP)); } +// Test HostTracker add and rediscover service payload +// (reuse/rediscover a deleted payload) +TEST(host_tracker, add_rediscover_service_payload_test) +{ + HostTracker ht; + HostApplication local_ha; + + ht.add_service(80, IpProtocol::TCP, 676, true); + ht.add_service(443, IpProtocol::TCP, 1122); + + // Test adding payload + // Add a payload for service http (appid 676), payload == appid 261 + // With 5 max payloads + ht.add_payload(local_ha, 80, IpProtocol::TCP, 261, 676, 5); + ht.add_payload(local_ha, 80, IpProtocol::TCP, 100, 676, 5); + + auto services = ht.get_services(); + + // Verify we added the services, payload visibility == true + for (auto& srv : services) + { + CHECK(srv.visibility); + for (auto& pld : srv.payloads) + CHECK(pld.second); + } + + // Delete the service + ht.set_service_visibility(80, IpProtocol::TCP, false); + + // Verify that the service (and payloads) are deleted + services = ht.get_services(); + for (auto& srv : services) + { + if (srv.port == 80) + CHECK(srv.visibility == false); + for ( auto& pld : srv.payloads ) + CHECK(pld.second == false); + } + + // Test rediscovery + // One payload is rediscovered as itself (existing payload with visibility = false) + // The other payload uses an existing slot that was freed when payload 100 was deleted + ht.add_service(80, IpProtocol::TCP, 676, true); + ht.add_payload(local_ha, 80, IpProtocol::TCP, 261, 676, 5); + ht.add_payload(local_ha, 80, IpProtocol::TCP, 101, 676, 5); + + services = ht.get_services(); + for (auto& srv : services) + { + if (srv.port == 80) + CHECK(srv.visibility == true); + for ( auto& pld : srv.payloads ) + CHECK(pld.second); + } + + CHECK(services.front().payloads.size() == 2); +} + +// Test HostTracker with max payloads and payload reuse +// (reuse a deleted payload for a new payload) +TEST(host_tracker, max_payloads_test) +{ + HostTracker ht; + HostApplication local_ha; + + ht.add_service(80, IpProtocol::TCP, 676, true); + + // These payloads aren't valid payloads, but host tracker doesn't care + ht.add_payload(local_ha, 80, IpProtocol::TCP, 111, 676, 5); + ht.add_payload(local_ha, 80, IpProtocol::TCP, 222, 676, 5); + ht.add_payload(local_ha, 80, IpProtocol::TCP, 333, 676, 5); + ht.add_payload(local_ha, 80, IpProtocol::TCP, 444, 676, 5); + ht.add_payload(local_ha, 80, IpProtocol::TCP, 555, 676, 5); + + auto services = ht.get_services(); + + // Verify we added the services, payload visibility == true + CHECK(services.front().payloads.size() == 5); + + for (auto& pld : services.front().payloads) + CHECK(pld.second); + + // Delete the service + ht.set_service_visibility(80, IpProtocol::TCP, false); + + // Check all payloads are invisible after service visibility is false + services = ht.get_services(); + for (auto& pld : services.front().payloads) + CHECK(pld.second == false); + + // Add a payload; we are already at max payloads, so re-use an existing slot + ht.add_service(80, IpProtocol::TCP, 676, true); + ht.add_payload(local_ha, 80, IpProtocol::TCP, 999, 676, 5); + bool found_new = false; + services = ht.get_services(); + for (auto& pld : services.front().payloads) + { + if (pld.first == 999) + { + CHECK(pld.second); + found_new = true; + } + else + { + CHECK(pld.second == false); + } + } + + // Check we still have only 5 payloads, and the new payload was added + CHECK(services.front().payloads.size() == 5 and found_new); + CHECK(services.front().num_visible_payloads == 1); +} + +// Test HostTracker with simple client payload rediscovery +// (reuse/rediscover a deleted payload) +TEST(host_tracker, client_payload_rediscovery_test) +{ + HostTracker ht; + HostClient hc; + bool new_client = false; + + // Create a new client, HTTP + hc = ht.find_or_add_client(2, "one", 676, new_client); + CHECK(new_client); + + // Add payloads 123 and 456 + ht.add_client_payload(hc, 123, 5); + ht.add_client_payload(hc, 456, 5); + auto clients = ht.get_clients(); + CHECK(clients.front().payloads.size() == 2); + + // Delete client, ensure payloads are also deleted + ht.set_client_visibility(hc, false); + clients = ht.get_clients(); + for (auto& pld : clients.front().payloads) + CHECK(pld.second == false); + + // Rediscover client, make sure payloads are still deleted + hc = ht.find_or_add_client(2, "one", 676, new_client); + clients = ht.get_clients(); + for (auto& pld : clients.front().payloads) + CHECK(pld.second == false); + + // Re-add payloads, ensure they're actually visible now + ht.add_client_payload(hc, 123, 5); + ht.add_client_payload(hc, 456, 5); + clients = ht.get_clients(); + for (auto& pld : clients.front().payloads) + CHECK(pld.second == true); + + CHECK(clients.front().payloads.size() == 2); + + // Make sure we didn't just add an extra client and two new payloads, rather than reusing + CHECK(clients.size() == 1); +} + +// Test HostTracker with max payloads and payload reuse +// (reuse an old payload for a new payload) +TEST(host_tracker, client_payload_max_payloads_test) +{ + HostTracker ht; + HostClient hc; + bool new_client = false; + + // Create a new client, HTTP + hc = ht.find_or_add_client(2, "one", 676, new_client); + + // Add five (max payloads) payloads + ht.add_client_payload(hc, 111, 5); + ht.add_client_payload(hc, 222, 5); + ht.add_client_payload(hc, 333, 5); + ht.add_client_payload(hc, 444, 5); + ht.add_client_payload(hc, 555, 5); + auto clients = ht.get_clients(); + CHECK_FALSE(ht.add_client_payload(hc, 666, 5)); + CHECK(clients.front().payloads.size() == 5); + + // Delete client, ensure payloads are also deleted + ht.set_client_visibility(hc, false); + clients = ht.get_clients(); + for (auto& pld : clients.front().payloads) + CHECK(pld.second == false); + + // Rediscover client, make sure payloads are still deleted + hc = ht.find_or_add_client(2, "one", 676, new_client); + clients = ht.get_clients(); + for (auto& pld : clients.front().payloads) + CHECK(pld.second == false); + + CHECK(clients.front().num_visible_payloads == 0); + + //Re-add payloads, ensure they're actually visible now + ht.add_client_payload(hc, 666, 5); + ht.add_client_payload(hc, 777, 5); + clients = ht.get_clients(); + for (auto& pld : clients.front().payloads) + { + if (pld.first == 666 or pld.first == 777) + { + CHECK(pld.second); + } + else + { + CHECK(pld.second == false); + } + } + + CHECK(clients.front().payloads.size() == 5); + CHECK(clients.front().num_visible_payloads == 2); + CHECK(clients.size() == 1); +} + // Test copying data and deleting copied list TEST(host_tracker, copy_data_test) {