From: Steve Chew (stechew) Date: Fri, 1 May 2020 13:05:34 +0000 (+0000) Subject: Merge pull request #2182 in SNORT/snort3 from ~STECHEW/snort3:retry_in_appid to master X-Git-Tag: 3.0.1-3~7 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=53c9dff5aa3e86259988179b05fa4a1de0f9cdce;p=thirdparty%2Fsnort3.git Merge pull request #2182 in SNORT/snort3 from ~STECHEW/snort3:retry_in_appid to master Squashed commit of the following: commit 2051a456ec98881eb3f9c4bf72c8d208700e804e Author: Steve Chew Date: Wed Apr 29 23:19:24 2020 -0400 appid: Make unit tests multithread safe. commit b11c6c8c20b671e2645adbf9c1ac779223927e97 Author: Steve Chew Date: Fri Apr 24 18:35:12 2020 -0400 appid: On API call store new values and publish an event for them immediately. commit 5d5fdc18224e6e0b927080ebdf9c8761139b9e20 Author: Steve Chew Date: Wed Apr 22 17:10:15 2020 -0400 appid: Do not process retry packets but continue processing future packets in AppId. --- diff --git a/src/network_inspectors/appid/appid_api.cc b/src/network_inspectors/appid/appid_api.cc index df54ff801..64e539195 100644 --- a/src/network_inspectors/appid/appid_api.cc +++ b/src/network_inspectors/appid/appid_api.cc @@ -233,10 +233,21 @@ bool AppIdApi::ssl_app_group_id_lookup(Flow* flow, const char* server_name, cons } service_id = asd->get_application_ids_service(); + AppId misc_id = asd->get_application_ids_misc(); + if (client_id == APP_ID_NONE) client_id = asd->get_application_ids_client(); + else + asd->client.set_id(client_id); + if (payload_id == APP_ID_NONE) payload_id = asd->get_application_ids_payload(); + else + asd->payload.set_id(payload_id); + + asd->set_application_ids(service_id, client_id, payload_id, misc_id, change_bits); + + asd->publish_appid_event(change_bits, flow); } else { diff --git a/src/network_inspectors/appid/appid_discovery.cc b/src/network_inspectors/appid/appid_discovery.cc index 1b90ec304..db3053bbe 100644 --- a/src/network_inspectors/appid/appid_discovery.cc +++ b/src/network_inspectors/appid/appid_discovery.cc @@ -137,22 +137,6 @@ void AppIdDiscovery::do_application_discovery(Packet* p, AppIdInspector& inspect misc_id, change_bits); } -void AppIdDiscovery::publish_appid_event(AppidChangeBits& change_bits, Flow* flow) -{ - if (change_bits.none()) - return; - - AppidEvent app_event(change_bits); - DataBus::publish(APPID_EVENT_ANY_CHANGE, app_event, flow); - if (appidDebug->is_active()) - { - std::string str; - change_bits_to_string(change_bits, str); - LogMessage("AppIdDbg %s Published event for changes: %s\n", - appidDebug->get_debug_session(), str.c_str()); - } -} - static inline unsigned get_ipfuncs_flags(const Packet* p, bool dst) { const SfIp* sf_ip; @@ -524,7 +508,7 @@ bool AppIdDiscovery::do_pre_discovery(Packet* p, AppIdSession** p_asd, AppIdInsp AppidChangeBits change_bits; asd->set_application_ids(asd->pick_service_app_id(), asd->pick_client_app_id(), asd->pick_payload_app_id(), asd->pick_misc_app_id(), change_bits); - publish_appid_event(change_bits, p->flow); + asd->publish_appid_event(change_bits, p->flow); asd->set_session_flags(APPID_SESSION_IGNORE_FLOW_IDED); } if (appidDebug->is_active() && @@ -542,6 +526,12 @@ bool AppIdDiscovery::do_pre_discovery(Packet* p, AppIdSession** p_asd, AppIdInsp return false; } + // The packet_flags will not be set on a retry packet so we have to skip + // processing it, but can continue processing the rest of the flow since + // AppId should have seen this packet already. + if (p->is_retry()) + return false; + if (p->ptrs.tcph and !asd->get_session_flags(APPID_SESSION_OOO)) { if ((p->packet_flags & PKT_STREAM_ORDER_BAD) || @@ -977,5 +967,5 @@ void AppIdDiscovery::do_post_discovery(Packet* p, AppIdSession& asd, } asd.set_application_ids(service_id, client_id, payload_id, misc_id, change_bits); - publish_appid_event(change_bits, p->flow); + asd.publish_appid_event(change_bits, p->flow); } diff --git a/src/network_inspectors/appid/appid_discovery.h b/src/network_inspectors/appid/appid_discovery.h index f9e23de90..d894deab8 100644 --- a/src/network_inspectors/appid/appid_discovery.h +++ b/src/network_inspectors/appid/appid_discovery.h @@ -114,7 +114,6 @@ public: static void do_application_discovery(snort::Packet* p, AppIdInspector&, ThirdPartyAppIdContext*); - static void publish_appid_event(AppidChangeBits&, snort::Flow*); AppIdDetectors* get_tcp_detectors() { diff --git a/src/network_inspectors/appid/appid_http_event_handler.cc b/src/network_inspectors/appid/appid_http_event_handler.cc index 7d00314e0..75fc50f21 100644 --- a/src/network_inspectors/appid/appid_http_event_handler.cc +++ b/src/network_inspectors/appid/appid_http_event_handler.cc @@ -151,6 +151,6 @@ void HttpEventHandler::handle(DataEvent& event, Flow* flow) asd->pick_payload_app_id(), asd->pick_misc_app_id(), change_bits); } - AppIdDiscovery::publish_appid_event(change_bits, flow); + asd->publish_appid_event(change_bits, flow); } diff --git a/src/network_inspectors/appid/appid_session.cc b/src/network_inspectors/appid/appid_session.cc index a60f9fcae..9e656adb3 100644 --- a/src/network_inspectors/appid/appid_session.cc +++ b/src/network_inspectors/appid/appid_session.cc @@ -1008,3 +1008,20 @@ void AppIdSession::set_tp_payload_app_id(Packet& p, AppidSessionDirection dir, A } } } + +void AppIdSession::publish_appid_event(AppidChangeBits& change_bits, Flow* flow) +{ + if (change_bits.none()) + return; + + AppidEvent app_event(change_bits); + DataBus::publish(APPID_EVENT_ANY_CHANGE, app_event, flow); + if (appidDebug->is_active()) + { + std::string str; + change_bits_to_string(change_bits, str); + LogMessage("AppIdDbg %s Published event for changes: %s\n", + appidDebug->get_debug_session(), str.c_str()); + } +} + diff --git a/src/network_inspectors/appid/appid_session.h b/src/network_inspectors/appid/appid_session.h index e057d99b5..9664aa1c1 100644 --- a/src/network_inspectors/appid/appid_session.h +++ b/src/network_inspectors/appid/appid_session.h @@ -364,6 +364,7 @@ public: AppidChangeBits& change_bits); void set_tp_payload_app_id(snort::Packet& p, AppidSessionDirection dir, AppId app_id, AppidChangeBits& change_bits); + void publish_appid_event(AppidChangeBits&, snort::Flow*); inline void set_tp_app_id(AppId app_id) { if (tp_app_id != app_id) diff --git a/src/network_inspectors/appid/detector_plugins/detector_sip.cc b/src/network_inspectors/appid/detector_plugins/detector_sip.cc index 79c06544a..3cf16fe38 100644 --- a/src/network_inspectors/appid/detector_plugins/detector_sip.cc +++ b/src/network_inspectors/appid/detector_plugins/detector_sip.cc @@ -337,7 +337,7 @@ void SipEventHandler::handle(DataEvent& event, Flow* flow) AppidChangeBits change_bits; client_handler(sip_event, *asd, change_bits); service_handler(sip_event, *asd, change_bits); - AppIdDiscovery::publish_appid_event(change_bits, flow); + asd->publish_appid_event(change_bits, flow); } void SipEventHandler::client_handler(SipEvent& sip_event, AppIdSession& asd, diff --git a/src/network_inspectors/appid/test/appid_api_test.cc b/src/network_inspectors/appid/test/appid_api_test.cc index b0ee968d5..c7621d79c 100644 --- a/src/network_inspectors/appid/test/appid_api_test.cc +++ b/src/network_inspectors/appid/test/appid_api_test.cc @@ -42,6 +42,7 @@ #include #include +#include using namespace snort; @@ -52,6 +53,21 @@ class Inspector* InspectorManager::get_inspector(char const*, bool, SnortConfig* { return nullptr; } } +void DataBus::publish(const char*, DataEvent& event, Flow*) +{ + AppidEvent* appid_event = (AppidEvent*)&event; + char* test_log = (char*)mock().getData("test_log").getObjectPointer(); + snprintf(test_log, 256, "Published change_bits == %s", + appid_event->get_change_bitset().to_string().c_str()); + mock().actualCall("publish"); +} + +void AppIdSession::publish_appid_event(AppidChangeBits& change_bits, Flow* flow) +{ + AppidEvent app_event(change_bits); + DataBus::publish(APPID_EVENT_ANY_CHANGE, app_event, flow); +} + bool SslPatternMatchers::scan_hostname(unsigned char const*, unsigned long, AppId& client_id, AppId& payload_id) { client_id = APPID_UT_ID + 1; @@ -90,16 +106,19 @@ AppIdSession* mock_session = nullptr; TEST_GROUP(appid_api) { + char test_log[256]; void setup() override { MemoryLeakWarningPlugin::turnOffNewDeleteOverloads(); flow = new Flow; flow->set_flow_data(mock_session); + mock().setDataObject("test_log", "char", test_log); } void teardown() override { delete flow; + mock().clear(); MemoryLeakWarningPlugin::turnOnNewDeleteOverloads(); } }; @@ -129,6 +148,11 @@ TEST(appid_api, produce_ha_state) memset((void*)&cmp_buf, 0, sizeof(cmp_buf)); mock_session->common.flow_type = APPID_FLOW_TYPE_IGNORE; mock_session->common.flags |= APPID_SESSION_SERVICE_DETECTED | APPID_SESSION_HTTP_SESSION; + + // Reset IDs that may be updated by ssl_app_group_id_lookup test. + mock_session->payload.set_id(APPID_UT_ID + 4); + mock_session->client.set_id(APPID_UT_ID + 6); + uint32_t val = appid_api.produce_ha_state(*flow, (uint8_t*)&appHA); CHECK_TRUE(val == sizeof(appHA)); CHECK_TRUE(memcmp(&appHA, &cmp_buf, val) == 0); @@ -190,6 +214,7 @@ TEST(appid_api, produce_ha_state) TEST(appid_api, ssl_app_group_id_lookup) { + mock().expectNCalls(4, "publish"); AppId service, client, payload = APP_ID_NONE; bool val = false; mock_session->common.flow_type = APPID_FLOW_TYPE_IGNORE; @@ -198,12 +223,15 @@ TEST(appid_api, ssl_app_group_id_lookup) CHECK_EQUAL(service, APP_ID_NONE); CHECK_EQUAL(client, APP_ID_NONE); CHECK_EQUAL(payload, APP_ID_NONE); + mock_session->common.flow_type = APPID_FLOW_TYPE_NORMAL; val = appid_api.ssl_app_group_id_lookup(flow, nullptr, nullptr, service, client, payload); CHECK_TRUE(val); CHECK_EQUAL(service, APPID_UT_ID); CHECK_EQUAL(client, APPID_UT_ID); CHECK_EQUAL(payload, APPID_UT_ID); + STRCMP_EQUAL("Published change_bits == 000000001111", test_log); + service = APP_ID_NONE; client = APP_ID_NONE; payload = APP_ID_NONE; @@ -211,6 +239,8 @@ TEST(appid_api, ssl_app_group_id_lookup) CHECK_TRUE(val); CHECK_EQUAL(client, APPID_UT_ID + 1); CHECK_EQUAL(payload, APPID_UT_ID + 1); + STRCMP_EQUAL("Published change_bits == 000001000110", test_log); + AppidChangeBits change_bits; mock_session->tsession->set_tls_host("www.cisco.com", 13, change_bits); mock_session->tsession->set_tls_cname("www.cisco.com", 13); @@ -226,6 +256,8 @@ 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 == 000001000110", test_log); + string host = ""; val = appid_api.ssl_app_group_id_lookup(flow, (const char*)(host.c_str()), (const char*)APPID_UT_TLS_HOST, service, client, payload, (const char*)("Google")); @@ -235,6 +267,8 @@ TEST(appid_api, ssl_app_group_id_lookup) STRCMP_EQUAL(mock_session->tsession->get_tls_host(), nullptr); STRCMP_EQUAL(mock_session->tsession->get_tls_cname(), APPID_UT_TLS_HOST); STRCMP_EQUAL(mock_session->tsession->get_tls_org_unit(), "Google"); + STRCMP_EQUAL("Published change_bits == 000000000110", test_log); + mock().checkExpectations(); } TEST(appid_api, create_appid_session_api) diff --git a/src/network_inspectors/appid/test/appid_discovery_test.cc b/src/network_inspectors/appid/test/appid_discovery_test.cc index 42a3aa441..5ec1fd726 100644 --- a/src/network_inspectors/appid/test/appid_discovery_test.cc +++ b/src/network_inspectors/appid/test/appid_discovery_test.cc @@ -35,6 +35,7 @@ #include #include +#include namespace snort { @@ -105,14 +106,13 @@ void IpApi::set(const SfIp& sip, const SfIp& dip) } // namespace snort // Stubs for publish -static bool databus_publish_called = false; -static char test_log[256]; void DataBus::publish(const char*, DataEvent& event, Flow*) { - databus_publish_called = true; AppidEvent* appid_event = (AppidEvent*)&event; + char* test_log = (char*)mock().getData("test_log").getObjectPointer(); snprintf(test_log, 256, "Published change_bits == %s", appid_event->get_change_bitset().to_string().c_str()); + mock().actualCall("publish"); } // Stubs for matchers @@ -199,6 +199,13 @@ AppIdSession* AppIdSession::allocate_session(const Packet*, IpProtocol, { return nullptr; } + +void AppIdSession::publish_appid_event(AppidChangeBits& change_bits, Flow* flow) +{ + AppidEvent app_event(change_bits); + DataBus::publish(APPID_EVENT_ANY_CHANGE, app_event, flow); +} + void AppIdHttpSession::set_tun_dest(){} // Stubs for ServiceDiscovery @@ -276,6 +283,7 @@ bool AppIdReloadTuner::tune_resources(unsigned int) TEST_GROUP(appid_discovery_tests) { + char test_log[256]; void setup() override { appidDebug = new AppIdDebug(); @@ -283,6 +291,7 @@ TEST_GROUP(appid_discovery_tests) s_app_module = new AppIdModule; s_ins = new AppIdInspector(*s_app_module); AppIdPegCounts::init_pegs(); + mock().setDataObject("test_log", "char", test_log); } void teardown() override @@ -303,12 +312,13 @@ TEST_GROUP(appid_discovery_tests) delete s_app_module; AppIdPegCounts::cleanup_pegs(); AppIdPegCounts::cleanup_peg_info(); + mock().clear(); } }; TEST(appid_discovery_tests, event_published_when_ignoring_flow) { // Testing event from do_pre_discovery() path - databus_publish_called = false; + mock().expectOneCall("publish"); test_log[0] = '\0'; Packet p; p.packet_flags = 0; @@ -329,7 +339,7 @@ TEST(appid_discovery_tests, event_published_when_ignoring_flow) AppIdDiscovery::do_application_discovery(&p, ins, nullptr); // Detect changes in service, client, payload, and misc appid - CHECK_EQUAL(databus_publish_called, true); + mock().checkExpectations(); STRCMP_EQUAL(test_log, "Published change_bits == 000000001111"); delete asd; delete flow; @@ -338,7 +348,7 @@ TEST(appid_discovery_tests, event_published_when_ignoring_flow) TEST(appid_discovery_tests, event_published_when_processing_flow) { // Testing event from do_discovery() path - databus_publish_called = false; + mock().expectOneCall("publish"); test_log[0] = '\0'; Packet p; p.packet_flags = 0; @@ -359,7 +369,7 @@ TEST(appid_discovery_tests, event_published_when_processing_flow) AppIdDiscovery::do_application_discovery(&p, ins, nullptr); // Detect changes in service, client, payload, and misc appid - CHECK_EQUAL(databus_publish_called, true); + mock().checkExpectations(); STRCMP_EQUAL(test_log, "Published change_bits == 000000001111"); delete asd; delete flow; @@ -395,7 +405,7 @@ TEST(appid_discovery_tests, change_bits_for_tls_host) TEST(appid_discovery_tests, change_bits_for_non_http_appid) { // Testing FTP appid - databus_publish_called = false; + mock().expectNCalls(2, "publish"); Packet p; p.packet_flags = 0; DAQ_PktHdr_t pkth; @@ -419,12 +429,10 @@ TEST(appid_discovery_tests, change_bits_for_non_http_appid) AppIdDiscovery::do_application_discovery(&p, ins, nullptr); // Detect event for FTP service and CURL client - CHECK_EQUAL(databus_publish_called, true); CHECK_EQUAL(asd->client.get_id(), APP_ID_CURL); CHECK_EQUAL(asd->service.get_id(), APP_ID_FTP); // Testing DNS appid - databus_publish_called = false; asd->misc_app_id = APP_ID_NONE; asd->payload.set_id(APP_ID_NONE); asd->client.set_id(APP_ID_NONE); @@ -432,7 +440,7 @@ TEST(appid_discovery_tests, change_bits_for_non_http_appid) AppIdDiscovery::do_application_discovery(&p, ins, nullptr); // Detect event for DNS service - CHECK_EQUAL(databus_publish_called, true); + mock().checkExpectations(); CHECK_EQUAL(asd->service.get_id(), APP_ID_DNS); delete asd; diff --git a/src/network_inspectors/appid/test/appid_http_event_test.cc b/src/network_inspectors/appid/test/appid_http_event_test.cc index 0981aea9c..78631dd84 100644 --- a/src/network_inspectors/appid/test/appid_http_event_test.cc +++ b/src/network_inspectors/appid/test/appid_http_event_test.cc @@ -69,8 +69,6 @@ class FakeHttpMsgHeader }; FakeHttpMsgHeader* fake_msg_header = nullptr; -void AppIdDiscovery::publish_appid_event(AppidChangeBits&, Flow*) {} - void AppIdHttpSession::set_http_change_bits(AppidChangeBits&, HttpFieldIds) {} const uint8_t* HttpEvent::get_content_type(int32_t& length) @@ -192,6 +190,8 @@ AppIdSession* AppIdApi::get_appid_session(const Flow&) return mock_session; } +void AppIdSession::publish_appid_event(AppidChangeBits&, Flow*) { } + TEST_GROUP(appid_http_event) { void setup() override