From: Oleksandr Stepanov -X (ostepano - SOFTSERVE INC at Cisco) Date: Mon, 16 Jun 2025 10:46:45 +0000 (+0000) Subject: Pull request #4739: appid: sync flow service on protocol based detection X-Git-Tag: 3.9.1.0~10 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=5b460d8abf211ba4cbbbe91af6cf0da606699b46;p=thirdparty%2Fsnort3.git 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 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 8a8cfeaa9..ad8ea25b6 100644 --- 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 e9e147e1e..c753c563c 100644 --- 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 a211a736c..78595f2cc 100644 --- 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 edf21a040..84ba6e67e 100644 --- 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 c1b59a0c8..8017678c3 100644 --- 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 8f123f9aa..43b50c3f5 100644 --- 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 ddae76216..983616eeb 100644 --- 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 e0b2c9454..5c6ba86d7 100644 --- 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 e4730e271..975d24a1a 100644 --- 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 3574c7055..311b32922 100644 --- 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(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 4a166bc91..ab24225b2 100644 --- 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!"); }