Pull request #4739: appid: sync flow service on protocol based detection

Merge in SNORT/snort3 from ~OSTEPANO/snort3:proto_detection_sync to master

Squashed commit of the following:

commit 727b13d446aa485de0d0f6b5fc4016b065a8fa3c
Author: Oleksandr Stepanov <ostepano@cisco.com>
Date:   Wed Apr 16 06:22:46 2025 -0400

    appid: sync flow service with protocol based detection

diff --git a/src/network_inspectors/appid/app_info_table.cc b/src/network_inspectors/appid/app_info_table.cc
index 8a8cfeaa99371210968bf8801d064c6bc04f64c2..ad8ea25b654a6a9df1f1f3beb0e32c2823c6e201 100644 (file)
--- a/src/network_inspectors/appid/app_info_table.cc
+++ b/src/network_inspectors/appid/app_info_table.cc
@@ -857,8 +857,8 @@ void AppInfoManager::init_appid_info_table(const AppIdConfig& config,
             AppInfoTableEntry* entry = new AppInfoTableEntry(app_id, app_name, service_id,
                 client_id, payload_id, attributes);
 
-             // FIXIT-RC: Sometimes the token is "~". Should we ignore those?
-            if (snort_service_key) 
+            //Ignore "~" service key, which is used to indicate an empty service key
+            if (snort_service_key and (strcmp(snort_service_key, "~") != 0))
                 entry->snort_protocol_id = add_appid_protocol_reference(snort_service_key, sc);
 
             if (!add_entry_to_app_info_name_table(entry->app_name_key, entry))

diff --git a/src/network_inspectors/appid/appid_discovery.cc b/src/network_inspectors/appid/appid_discovery.cc
index e9e147e1e57d48518b8dc0c3ea4e983f9e3f000a..c753c563ccdf8d17bc9f9373f85198dc3ceff8d6 100644 (file)
--- a/src/network_inspectors/appid/appid_discovery.cc
+++ b/src/network_inspectors/appid/appid_discovery.cc
@@ -309,6 +309,7 @@ bool AppIdDiscovery::do_pre_discovery(Packet* p, AppIdSession*& asd, AppIdInspec
             asd->pick_ss_referred_payload_app_id(), change_bits);
         if (PacketTracer::is_daq_activated())
             populate_trace_data(*asd);
+        change_bits.set(APPID_PROTOCOL_ID_BIT);
         asd->publish_appid_event(change_bits, *p);
         asd->set_session_flags(APPID_SESSION_FUTURE_FLOW_IDED);
 
@@ -498,8 +499,9 @@ bool AppIdDiscovery::do_host_port_based_discovery(Packet* p, AppIdSession& asd,
             asd.set_payload_id(hv->appId);
             break;
         default:
+            AppidChangeBits tmp_bits;
             asd.set_service_id(hv->appId, asd.get_odp_ctxt());
-            asd.sync_with_snort_protocol_id(hv->appId, p);
+            asd.sync_with_snort_protocol_id(hv->appId, p, tmp_bits);
             asd.service_disco_state = APPID_DISCO_STATE_FINISHED;
             asd.client_disco_state = APPID_DISCO_STATE_FINISHED;
             asd.set_session_flags(APPID_SESSION_SERVICE_DETECTED);
@@ -722,14 +724,23 @@ bool AppIdDiscovery::do_discovery(Packet* p, AppIdSession& asd, IpProtocol proto
                 const char *app_name = asd.get_odp_ctxt().get_app_info_mgr().get_app_name(ps_id);
                 APPID_LOG(p, TRACE_DEBUG_LEVEL, "Protocol service %s (%d) from protocol\n",
                     app_name ? app_name : "unknown", ps_id);
+
+                asd.sync_with_snort_protocol_id(id, p, change_bits);
             }
             asd.set_session_flags(APPID_SESSION_PORT_SERVICE_DONE);
+            
         }
         else
         {
             service_id = asd.pick_service_app_id();
             misc_id = asd.pick_ss_misc_app_id();
         }
+        if (asd.tpsession)
+            asd.tpsession->set_state(TP_STATE_TERMINATED);
+        asd.set_session_flags(APPID_SESSION_NO_TPI);
+        asd.set_session_flags(APPID_SESSION_SERVICE_DETECTED);
+        asd.client_disco_state = APPID_DISCO_STATE_FINISHED;
+        asd.service_disco_state = APPID_DISCO_STATE_FINISHED;
         return true;
     }
 
@@ -893,5 +904,10 @@ void AppIdDiscovery::do_post_discovery(Packet* p, AppIdSession& asd,
     if (is_discovery_done and asd.get_shadow_traffic_bits() == 0 )
         asd.process_shadow_traffic_appids();
 
+    if (change_bits.test(APPID_SERVICE_BIT))
+    {
+        asd.sync_with_snort_protocol_id(service_id, p, change_bits);
+    }
+
     asd.publish_appid_event(change_bits, *p);
 }

diff --git a/src/network_inspectors/appid/appid_session.cc b/src/network_inspectors/appid/appid_session.cc
index a211a736cc8548a2520d340e187c5ecb713f5d07..78595f2cccdd8fd2c6bfa59a81d66be5d87f15c6 100644 (file)
--- a/src/network_inspectors/appid/appid_session.cc
+++ b/src/network_inspectors/appid/appid_session.cc
@@ -346,7 +346,7 @@ void AppIdSession::reinit_session_data(AppidChangeBits& change_bits,
         APPID_SESSION_HTTP_SESSION | APPID_SESSION_APP_REINSPECT);
 }
 
-void AppIdSession::sync_with_snort_protocol_id(AppId newAppId, Packet* p)
+void AppIdSession::sync_with_snort_protocol_id(AppId newAppId, Packet* p, AppidChangeBits& change_bits)
 {
     if (newAppId  <= APP_ID_NONE or newAppId >= SF_APPID_MAX)
         return;
@@ -386,10 +386,15 @@ void AppIdSession::sync_with_snort_protocol_id(AppId newAppId, Packet* p)
     // A particular APP_ID_xxx may not be assigned a service_snort_key value
     // in the appMapping.data file entry; so ignore the snort_protocol_id ==
     // UNKNOWN_PROTOCOL_ID case.
-    if (tmp_snort_protocol_id != snort_protocol_id)
+    if (tmp_snort_protocol_id)
     {
-        snort_protocol_id = tmp_snort_protocol_id;
-        Stream::set_snort_protocol_id(p->flow, tmp_snort_protocol_id, true);
+        if (tmp_snort_protocol_id != snort_protocol_id)
+        {
+            if (tmp_snort_protocol_id != p->flow->ssn_state.snort_protocol_id)
+                change_bits.set(APPID_PROTOCOL_ID_BIT);
+            snort_protocol_id = tmp_snort_protocol_id;
+            Stream::set_snort_protocol_id(p->flow, tmp_snort_protocol_id, true);
+        }
     }
 }
 
@@ -1133,7 +1138,7 @@ AppIdDnsSession* AppIdSession::get_dns_session() const
 
 bool AppIdSession::is_tp_appid_done() const
 {
-    if (get_session_flags(APPID_SESSION_FUTURE_FLOW) or !tp_appid_ctxt)
+    if (get_session_flags(APPID_SESSION_FUTURE_FLOW | APPID_SESSION_NO_TPI) or !tp_appid_ctxt)
         return true;
 
     if (!tpsession)

diff --git a/src/network_inspectors/appid/appid_session.h b/src/network_inspectors/appid/appid_session.h
index edf21a040fa8ca9e1e1cf13ed0b2967174aaaa49..84ba6e67ea1bda3d5f450e8fee0a5ac47cef6b21 100644 (file)
--- a/src/network_inspectors/appid/appid_session.h
+++ b/src/network_inspectors/appid/appid_session.h
@@ -393,7 +393,7 @@ public:
     void check_tunnel_detection_restart();
     void update_encrypted_app_id(AppId);
     void examine_rtmp_metadata(AppidChangeBits& change_bits);
-    void sync_with_snort_protocol_id(AppId, snort::Packet*);
+    void sync_with_snort_protocol_id(AppId, snort::Packet*, AppidChangeBits&);
     void stop_service_inspection(snort::Packet*,  AppidSessionDirection);
 
     void clear_http_flags();

diff --git a/src/network_inspectors/appid/client_plugins/client_discovery.cc b/src/network_inspectors/appid/client_plugins/client_discovery.cc
index c1b59a0c88e48a5fc954c1261d396ad3ea216daa..8017678c365c5412b86fe7c6d9491b79fe0227f0 100644 (file)
--- a/src/network_inspectors/appid/client_plugins/client_discovery.cc
+++ b/src/network_inspectors/appid/client_plugins/client_discovery.cc
@@ -313,7 +313,6 @@ bool ClientDiscovery::do_client_discovery(AppIdSession& asd, Packet* p,
 {
     bool is_discovery_done = false;
     AppInfoTableEntry* entry;
-    bool was_service = asd.is_service_detected();
     AppId tp_app_id = asd.get_tp_app_id();
 
     if (asd.client_disco_state == APPID_DISCO_STATE_NONE and
@@ -380,8 +379,5 @@ bool ClientDiscovery::do_client_discovery(AppIdSession& asd, Packet* p,
         }
     }
 
-    if ( !was_service && asd.is_service_detected() )
-        asd.sync_with_snort_protocol_id(asd.get_service_id(), p);
-
     return is_discovery_done;
 }

diff --git a/src/network_inspectors/appid/service_plugins/service_discovery.cc b/src/network_inspectors/appid/service_plugins/service_discovery.cc
index 8f123f9aa7134df739d3c09e48696d3c365a89ee..43b50c3f54b6597461c8ad61451f168bb298373e 100644 (file)
--- a/src/network_inspectors/appid/service_plugins/service_discovery.cc
+++ b/src/network_inspectors/appid/service_plugins/service_discovery.cc
@@ -771,13 +771,6 @@ bool ServiceDiscovery::do_service_discovery(AppIdSession& asd, Packet* p,
             asd.examine_rtmp_metadata(change_bits);
         else if (asd.get_session_flags(APPID_SESSION_SSL_SESSION) and asd.tsession)
             asd.examine_ssl_metadata(change_bits);
-
-        if (tp_app_id <= APP_ID_NONE and asd.get_session_flags(
-            APPID_SESSION_SERVICE_DETECTED | APPID_SESSION_NOT_A_SERVICE |
-            APPID_SESSION_IGNORE_HOST) == APPID_SESSION_SERVICE_DETECTED)
-        {
-            asd.sync_with_snort_protocol_id(asd.get_service_id(), p);
-        }
     }
 
     return is_discovery_done;

diff --git a/src/network_inspectors/appid/test/appid_api_test.cc b/src/network_inspectors/appid/test/appid_api_test.cc
index ddae76216fab7f4b013e97c33b5852432eb98457..983616eebc259b79711e334eef5cdca755c63346 100644 (file)
--- a/src/network_inspectors/appid/test/appid_api_test.cc
+++ b/src/network_inspectors/appid/test/appid_api_test.cc
@@ -292,7 +292,7 @@ TEST(appid_api, ssl_app_group_id_lookup)
     CHECK_EQUAL(service, APPID_UT_ID);
     CHECK_EQUAL(client, APPID_UT_ID);
     CHECK_EQUAL(payload, APPID_UT_ID);
-    STRCMP_EQUAL("Published change_bits == 0000000000000000000000", test_log);
+    STRCMP_EQUAL("Published change_bits == 00000000000000000000000", test_log);
 
     // Server name based detection
     service = APP_ID_NONE;
@@ -307,7 +307,7 @@ TEST(appid_api, ssl_app_group_id_lookup)
     STRCMP_EQUAL(mock_session->tsession->get_tls_first_alt_name(), APPID_UT_TLS_HOST);
     STRCMP_EQUAL(mock_session->tsession->get_tls_cname(), APPID_UT_TLS_HOST);
     STRCMP_EQUAL(mock_session->tsession->get_tls_sni(),  APPID_UT_TLS_HOST);
-    STRCMP_EQUAL("Published change_bits == 0000000000000100011000", test_log);
+    STRCMP_EQUAL("Published change_bits == 00000000000000100011000", test_log);
 
     // Common name based detection
     mock_session->tsession->set_tls_host("www.cisco.com", 13, change_bits);
@@ -324,7 +324,7 @@ TEST(appid_api, ssl_app_group_id_lookup)
     STRCMP_EQUAL(mock_session->tsession->get_tls_host(), APPID_UT_TLS_HOST);
     STRCMP_EQUAL(mock_session->tsession->get_tls_cname(), APPID_UT_TLS_HOST);
     STRCMP_EQUAL(mock_session->tsession->get_tls_org_unit(), "Cisco");
-    STRCMP_EQUAL("Published change_bits == 0000000000000100011000", test_log);
+    STRCMP_EQUAL("Published change_bits == 00000000000000100011000", test_log);
 
     // First alt name based detection
     change_bits.reset();
@@ -336,7 +336,7 @@ TEST(appid_api, ssl_app_group_id_lookup)
     CHECK_EQUAL(payload, APPID_UT_ID + 1);
     STRCMP_EQUAL(mock_session->tsession->get_tls_host(), APPID_UT_TLS_HOST);
     STRCMP_EQUAL(mock_session->tsession->get_tls_first_alt_name(), APPID_UT_TLS_HOST);
-    STRCMP_EQUAL("Published change_bits == 0000000000000100011000", test_log);
+    STRCMP_EQUAL("Published change_bits == 00000000000000100011000", test_log);
 
     // Org unit based detection
     string host = "";
@@ -348,7 +348,7 @@ TEST(appid_api, ssl_app_group_id_lookup)
     CHECK_EQUAL(client, APPID_UT_ID + 3);
     CHECK_EQUAL(payload, APPID_UT_ID + 3);
     STRCMP_EQUAL(mock_session->tsession->get_tls_org_unit(), APPID_UT_ORG_UNIT);
-    STRCMP_EQUAL("Published change_bits == 0000000000000000011000", test_log);
+    STRCMP_EQUAL("Published change_bits == 00000000000000000011000", test_log);
 
     // Override client id found by SSL pattern matcher with the client id provided by
     // Encrypted Visibility Engine if available
@@ -367,7 +367,7 @@ TEST(appid_api, ssl_app_group_id_lookup)
     STRCMP_EQUAL(mock_session->tsession->get_tls_host(), APPID_UT_TLS_HOST);
     STRCMP_EQUAL(mock_session->tsession->get_tls_first_alt_name(), APPID_UT_TLS_HOST);
     STRCMP_EQUAL(mock_session->tsession->get_tls_cname(), APPID_UT_TLS_HOST);
-    STRCMP_EQUAL("Published change_bits == 0000000000000100011000", test_log);
+    STRCMP_EQUAL("Published change_bits == 00000000000000100011000", test_log);
 
     //check for sni mismatch being stored in sni field
     change_bits.reset();

diff --git a/src/network_inspectors/appid/test/appid_discovery_test.cc b/src/network_inspectors/appid/test/appid_discovery_test.cc
index e0b2c9454ac4274a0dea29ddeed69623e7895e61..5c6ba86d723ada56eb6dd7eebe287e4993310407 100644 (file)
--- a/src/network_inspectors/appid/test/appid_discovery_test.cc
+++ b/src/network_inspectors/appid/test/appid_discovery_test.cc
@@ -224,7 +224,7 @@ uint32_t AppInfoManager::getAttributeBits(AppId)
 }
 
 // Stubs for AppIdSession
-void AppIdSession::sync_with_snort_protocol_id(AppId, Packet*) {}
+void AppIdSession::sync_with_snort_protocol_id(AppId, Packet*,AppidChangeBits&) {}
 void AppIdSession::check_app_detection_restart(AppidChangeBits&, ThirdPartyAppIdContext*) {}
 void AppIdSession::set_client_appid_data(AppId, AppidChangeBits&, char*) {}
 void AppIdSession::examine_rtmp_metadata(AppidChangeBits&) {}
@@ -420,9 +420,9 @@ TEST(appid_discovery_tests, event_published_when_ignoring_flow)
 
     AppIdDiscovery::do_application_discovery(&p, ins, app_ctxt.get_odp_ctxt(), nullptr);
 
-    // Detect changes in service, client, payload, and misc appid
+    // Detect changes in service, client, payload, misc appid and snort protocol id
     mock().checkExpectations();
-    STRCMP_EQUAL("Published change_bits == 0000000000000001111100", test_log);
+    STRCMP_EQUAL("Published change_bits == 10000000000000001111100", test_log);
 
     delete &asd->get_api();
     delete asd;
@@ -461,7 +461,7 @@ TEST(appid_discovery_tests, event_published_when_processing_flow)
 
     // Detect changes in service, client, payload, and misc appid
     mock().checkExpectations();
-    STRCMP_EQUAL("Published change_bits == 0000000000000001111100", test_log);
+    STRCMP_EQUAL("Published change_bits == 00000000000000001111100", test_log);
     delete &asd->get_api();
     delete asd;
     delete flow;
@@ -569,10 +569,10 @@ TEST(appid_discovery_tests, change_bits_to_string)
     change_bits_to_string(change_bits, str);
     STRCMP_EQUAL(str.c_str(), "created, reset, service, client, payload, misc, referred, host,"
         " tls-host, url, user-agent, response, referrer, dns-host, dns-response-host, service-info, client-info,"
-        " user-info, netbios-name, netbios-domain, finished, tls-version");
+        " user-info, netbios-name, netbios-domain, finished, tls-version, protocol-id");
 
     // Failure of this test is a reminder that enum is changed, hence translator needs update
-    CHECK_EQUAL(APPID_MAX_BIT, 22);
+    CHECK_EQUAL(APPID_MAX_BIT, 23);
 }
 
 int main(int argc, char** argv)

diff --git a/src/network_inspectors/appid/tp_appid_utils.cc b/src/network_inspectors/appid/tp_appid_utils.cc
index e4730e271da791c757a2f9701a2d2614e1a9c628..975d24a1ae667bed2e30dc2971c4ffc00cf95363 100644 (file)
--- a/src/network_inspectors/appid/tp_appid_utils.cc
+++ b/src/network_inspectors/appid/tp_appid_utils.cc
@@ -650,11 +650,8 @@ bool do_tp_discovery(ThirdPartyAppIdContext& tp_appid_ctxt, AppIdSession& asd, I
 
     if ( tp_app_id > APP_ID_NONE )
     {
-        AppId snort_app_id = APP_ID_NONE;
-
         if ( hsession )
         {
-            snort_app_id = APP_ID_HTTP;
             //data should never be APP_ID_HTTP
             if (tp_app_id != APP_ID_HTTP)
                 asd.set_tp_payload_app_id(*p, direction, tp_app_id, change_bits);
@@ -728,18 +725,11 @@ bool do_tp_discovery(ThirdPartyAppIdContext& tp_appid_ctxt, AppIdSession& asd, I
                 const char *tp_app_name = asd.get_odp_ctxt().get_app_info_mgr().get_app_name(tp_app_id);
                 APPID_LOG(p, TRACE_DEBUG_LEVEL, "SSL is %s (%d)\n", tp_app_name ? tp_app_name : "unknown", tp_app_id);
             }
-            snort_app_id = APP_ID_SSL;
         }
         else if (asd.get_service_id() == APP_ID_QUIC)
             asd.set_tp_payload_app_id(*p, direction, tp_app_id, change_bits);
-        else
-        {
-            //for non-http protocols, tp id is treated like serviceId
-            snort_app_id = tp_app_id;
-        }
 
         asd.set_tp_app_id(*p, direction, tp_app_id, change_bits);
-        asd.sync_with_snort_protocol_id(snort_app_id, p);
     }
 
     if (direction == APP_ID_FROM_INITIATOR)

diff --git a/src/network_inspectors/binder/binder.cc b/src/network_inspectors/binder/binder.cc
index 3574c7055eb3ae9c9cdbd5749ef25c3f8de6f8e2..311b32922fd3d6d8cd4cfdcb2d55c3c4010d9918 100644 (file)
--- a/src/network_inspectors/binder/binder.cc
+++ b/src/network_inspectors/binder/binder.cc
@@ -775,15 +775,12 @@ void Binder::handle_appid_service_change(DataEvent& event, Flow& flow)
 {
     AppidEvent& appid_event = static_cast<AppidEvent&>(event);
 
-    if(appid_event.get_change_bitset().test(APPID_SERVICE_BIT))
+    if(appid_event.get_change_bitset().test(APPID_PROTOCOL_ID_BIT))
     {
-        if ((appid_event.get_appid_session_api().get_service_app_id() <= 0)
-            or (flow.ssn_state.snort_protocol_id == 0))
-            return;
-
         Stuff stuff;
         const SnortConfig* sc = SnortConfig::get_conf();
         const char* service = sc->proto_ref->get_name(flow.ssn_state.snort_protocol_id);
+        
         get_bindings(flow, stuff, service);
 
         if(stuff.action == BindUse::BA_ALLOW)

diff --git a/src/pub_sub/appid_events.h b/src/pub_sub/appid_events.h
index 4a166bc913f21e1b2679557ea503219735869449..ab24225b281d3e7d2c70b6dc3acdb18fcbe34856 100644 (file)
--- a/src/pub_sub/appid_events.h
+++ b/src/pub_sub/appid_events.h
@@ -65,6 +65,7 @@ enum AppidChangeBit
     APPID_NETBIOS_DOMAIN_BIT,
     APPID_DISCOVERY_FINISHED_BIT,
     APPID_TLS_VERSION_BIT,
+    APPID_PROTOCOL_ID_BIT,
 
     APPID_MAX_BIT
 };
@@ -119,6 +120,8 @@ inline void change_bits_to_string(const AppidChangeBits& change_bits, std::strin
         --n? str.append("finished, ") : str.append("finished");
     if (change_bits.test(APPID_TLS_VERSION_BIT))
         --n? str.append("tls-version, ") : str.append("tls-version");
+    if (change_bits.test(APPID_PROTOCOL_ID_BIT))
+        --n? str.append("protocol-id, ") : str.append("protocol-id");
     if (n != 0) // make sure all bits from AppidChangeBit enum get translated
         str.append("change_bits_to_string error!");
 }