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 <stechew@cisco.com>
Date:   Wed Apr 29 23:19:24 2020 -0400

    appid: Make unit tests multithread safe.

commit b11c6c8c20b671e2645adbf9c1ac779223927e97
Author: Steve Chew <stechew@cisco.com>
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 <stechew@cisco.com>
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 df54ff8014475cf55eb440053c75959a00bba8e1..64e5391957f419d788b60daf8518fb4ba5b7cb3a 100644 (file)
--- 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 1b90ec30463f8babefab2746855494589bc0cc0c..db3053bbe29176440dbb4e61049ea8eca71dd7e4 100644 (file)
--- 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 f9e23de905c565dbf56a2ee65ab41baff5347f06..d894deab8f2d1c0dc6b5b7ded41a15db0db579c4 100644 (file)
--- 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 7d00314e0eb69985b630f15a29d8d951c0290c78..75fc50f217eaa3140bbb206e42a30c7e4d1e80d9 100644 (file)
--- 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 a60f9fcae0f13b34b214a092d3a51868555930e2..9e656adb382221b35bf3d8117c1fb22d921f76dd 100644 (file)
--- 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 e057d99b53d0df09b1e2a4a735406bc430f40400..9664aa1c1216cd7bc1976fcaad558148c49977d8 100644 (file)
--- 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 79c06544a4bc31f53063690ea23fa3e2ffb9fd9e..3cf16fe3826737f62dc411e9f6e018f663d66fb7 100644 (file)
--- 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 b0ee968d57b9752fcb4ac17907bef41bc5518cf5..c7621d79c90aad9378b4a2e7f124c518ee6d61df 100644 (file)
--- 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 <CppUTest/CommandLineTestRunner.h>
 #include <CppUTest/TestHarness.h>
+#include <CppUTestExt/MockSupport.h>
 
 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 42a3aa44168ee415e1b1310b80b90a98911465ee..5ec1fd726e43415cf96a77b227193a3d9ca3de7f 100644 (file)
--- 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 <CppUTest/CommandLineTestRunner.h>
 #include <CppUTest/TestHarness.h>
+#include <CppUTestExt/MockSupport.h>
 
 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 0981aea9ceec2eec0692a208a9900871ba409a28..78631dd84d6e693479d57ecfb0ac576f1c3513f8 100644 (file)
--- 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