Pull request #4743: appid: fix tcp dns multiple transaction support

Merge in SNORT/snort3 from ~RDEMPSTE/snort3:dns_logging to master

Squashed commit of the following:

commit ee1088e727a5c83e68e05829bc082cddc9bbf45c
Author: Ron Dempster (rdempste) <rdempste@cisco.com>
Date:   Wed May 14 13:28:31 2025 -0400

    appid: differentiate between request and response DNS host

commit a8454a7feb16cf966ec3d00c30d984caffbe1f5e
Author: Ron Dempster (rdempste) <rdempste@cisco.com>
Date:   Fri May 9 09:27:02 2025 -0400

    appid: fix tcp dns multiple transaction support

diff --git a/src/network_inspectors/appid/appid_dns_session.h b/src/network_inspectors/appid/appid_dns_session.h
index 266431a78064bc2e4fbddc53daa9aaff2be482c1..eeeccc43f8c89b16c0f090fabe3eb8fad3157471 100644 (file)
--- a/src/network_inspectors/appid/appid_dns_session.h
+++ b/src/network_inspectors/appid/appid_dns_session.h
@@ -79,10 +79,19 @@ public:
     const char* get_host() const
     { return host.c_str(); }
 
-    void set_host(char* host, AppidChangeBits& change_bits)
+    void set_host(const char* host, AppidChangeBits& change_bits, bool request)
     {
         this->host = host;
-        change_bits.set(APPID_DNS_HOST_BIT);
+        if (request)
+        {
+            change_bits.reset(APPID_DNS_RESPONSE_HOST_BIT);
+            change_bits.set(APPID_DNS_REQUEST_HOST_BIT);
+        }
+        else
+        {
+            change_bits.reset(APPID_DNS_REQUEST_HOST_BIT);
+            change_bits.set(APPID_DNS_RESPONSE_HOST_BIT);
+        }
     }
 
     uint32_t get_host_len() const

diff --git a/src/network_inspectors/appid/detector_plugins/detector_dns.cc b/src/network_inspectors/appid/detector_plugins/detector_dns.cc
index f5b55f8cb15c1d80da331847403eb5b3be1be1b1..db1d19b39980e25d3f60676f46fbfa1102d949c0 100644 (file)
--- a/src/network_inspectors/appid/detector_plugins/detector_dns.cc
+++ b/src/network_inspectors/appid/detector_plugins/detector_dns.cc
@@ -131,29 +131,30 @@ struct DNSAnswerData
 enum DNSState
 {
     DNS_STATE_QUERY,
-    DNS_STATE_RESPONSE
+    DNS_STATE_RESPONSE,
+    DNS_STATE_MULTI_QUERY
 };
 
 class ServiceDNSData : public AppIdFlowData
 {
 public:
-    ServiceDNSData();
+    ServiceDNSData() = default;
     ~ServiceDNSData() override;
     void save_dns_cache(uint16_t size, const uint8_t* data);
     void free_dns_cache();
 
     DNSState state = DNS_STATE_QUERY;
     uint8_t* cached_data = nullptr;
-    uint16_t cached_len = 0; 
+    uint16_t cached_len = 0;
     uint16_t id = 0;
 };
 
 void ServiceDNSData::save_dns_cache(uint16_t size, const uint8_t* data)
 {
-    if(size > 0) 
+    if(size > 0)
     {
         cached_data = (uint8_t*)snort_calloc(size, sizeof(uint8_t));
-        if(cached_data) 
+        if(cached_data)
         {
             memcpy(cached_data, data, size);
         }
@@ -172,13 +173,6 @@ void ServiceDNSData::free_dns_cache()
     cached_len = 0;
 }
 
-ServiceDNSData::ServiceDNSData()
-{
-    state = DNS_STATE_QUERY;
-    cached_data = nullptr;
-    cached_len = 0;
-}
-
 ServiceDNSData::~ServiceDNSData()
 {
     free_dns_cache();
@@ -253,7 +247,7 @@ APPID_STATUS_CODE DnsValidator::add_dns_query_info(AppIdSession& asd, uint16_t i
             char* new_host = dns_parse_host(host, host_len);
             if (!new_host)
                 return APPID_NOMATCH;
-            dsession->set_host(new_host, change_bits);
+            dsession->set_host(new_host, change_bits, true);
             dsession->set_host_offset(host_offset);
             dsession->set_options_offset(options_offset);
             snort_free(new_host);
@@ -288,7 +282,7 @@ APPID_STATUS_CODE DnsValidator::add_dns_response_info(AppIdSession& asd, uint16_
             char* new_host = dns_parse_host(host, host_len);
             if (!new_host)
                 return APPID_NOMATCH;
-            dsession->set_host(new_host, change_bits);
+            dsession->set_host(new_host, change_bits, false);
             dsession->set_host_offset(host_offset);
             snort_free(new_host);
         }
@@ -724,19 +718,21 @@ int DnsTcpServiceDetector::validate(AppIdDiscoveryArgs& args)
         if (rval != APPID_SUCCESS)
             goto tcp_done;
 
-        if (dd->state == DNS_STATE_QUERY)
+        if (dd->state == DNS_STATE_QUERY || dd->state == DNS_STATE_MULTI_QUERY)
         {
             if (args.dir != APP_ID_FROM_INITIATOR)
                 goto fail;
             dd->id = ((const DNSHeader*)data)->id;
+            DNSState current_state = dd->state;
             dd->state = DNS_STATE_RESPONSE;
-            goto inprocess;
+            if (current_state == DNS_STATE_QUERY)
+                goto inprocess;
+            goto success;
         }
+        else if (args.dir == APP_ID_FROM_RESPONDER && dd->id == ((const DNSHeader*)data)->id)
+            dd->state = DNS_STATE_MULTI_QUERY;
         else
-        {
-            if (args.dir != APP_ID_FROM_RESPONDER || dd->id != ((const DNSHeader*)data)->id)
-                goto fail;
-        }
+            goto fail;
     }
 tcp_done:
     switch (rval)

diff --git a/src/network_inspectors/appid/test/CMakeLists.txt b/src/network_inspectors/appid/test/CMakeLists.txt
index 0eebbf372aff1e424896688265dea09d14a4b2ed..aac3507935bd11cab122dbc64aa8f75def248f38 100644 (file)
--- a/src/network_inspectors/appid/test/CMakeLists.txt
+++ b/src/network_inspectors/appid/test/CMakeLists.txt
@@ -17,6 +17,9 @@ add_cpputest( appid_session_api_test
     SOURCES $<TARGET_OBJECTS:appid_cpputest_deps>
 )
 
+add_cpputest( appid_dns_session_test
+)
+
 add_cpputest( appid_detector_test
     SOURCES $<TARGET_OBJECTS:appid_cpputest_deps>
 )

diff --git a/src/network_inspectors/appid/test/appid_api_test.cc b/src/network_inspectors/appid/test/appid_api_test.cc
index 7b1d892e800c426e703bb8049efc4046c2ccfb24..ddae76216fab7f4b013e97c33b5852432eb98457 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 == 000000000000000000000", test_log);
+    STRCMP_EQUAL("Published change_bits == 0000000000000000000000", 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 == 000000000000100011000", test_log);
+    STRCMP_EQUAL("Published change_bits == 0000000000000100011000", 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 == 000000000000100011000", test_log);
+    STRCMP_EQUAL("Published change_bits == 0000000000000100011000", 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 == 000000000000100011000", test_log);
+    STRCMP_EQUAL("Published change_bits == 0000000000000100011000", 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 == 000000000000000011000", test_log);
+    STRCMP_EQUAL("Published change_bits == 0000000000000000011000", 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 == 000000000000100011000", test_log);
+    STRCMP_EQUAL("Published change_bits == 0000000000000100011000", 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 94a2af541d65689b39a327c2c6214a948decab8a..e0b2c9454ac4274a0dea29ddeed69623e7895e61 100644 (file)
--- a/src/network_inspectors/appid/test/appid_discovery_test.cc
+++ b/src/network_inspectors/appid/test/appid_discovery_test.cc
@@ -422,7 +422,7 @@ TEST(appid_discovery_tests, event_published_when_ignoring_flow)
 
     // Detect changes in service, client, payload, and misc appid
     mock().checkExpectations();
-    STRCMP_EQUAL("Published change_bits == 000000000000001111100", test_log);
+    STRCMP_EQUAL("Published change_bits == 0000000000000001111100", 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 == 000000000000001111100", test_log);
+    STRCMP_EQUAL("Published change_bits == 0000000000000001111100", test_log);
     delete &asd->get_api();
     delete asd;
     delete flow;
@@ -568,11 +568,11 @@ TEST(appid_discovery_tests, change_bits_to_string)
     change_bits.set();
     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, service-info, client-info,"
+        " 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");
 
     // Failure of this test is a reminder that enum is changed, hence translator needs update
-    CHECK_EQUAL(APPID_MAX_BIT, 21);
+    CHECK_EQUAL(APPID_MAX_BIT, 22);
 }
 
 int main(int argc, char** argv)

diff --git a/src/network_inspectors/appid/test/appid_dns_session_test.cc b/src/network_inspectors/appid/test/appid_dns_session_test.cc
new file mode 100644 (file)
index 0000000..0593662
--- /dev/null
+++ b/src/network_inspectors/appid/test/appid_dns_session_test.cc
@@ -0,0 +1,73 @@
+//--------------------------------------------------------------------------
+// Copyright (C) 2016-2025 Cisco and/or its affiliates. All rights reserved.
+//
+// This program is free software; you can redistribute it and/or modify it
+// under the terms of the GNU General Public License Version 2 as published
+// by the Free Software Foundation.  You may not use, modify or distribute
+// this program under any other version of the GNU General Public License.
+//
+// This program is distributed in the hope that it will be useful, but
+// WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+// General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this program; if not, write to the Free Software Foundation, Inc.,
+// 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+//--------------------------------------------------------------------------
+
+// appid_dns_session_test.cc author Ron Dempster <rdempste@cisco.com>
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include <string>
+
+#include "network_inspectors/appid/appid_dns_session.h"
+
+#include <CppUTest/CommandLineTestRunner.h>
+#include <CppUTest/TestHarness.h>
+
+using namespace snort;
+
+TEST_GROUP(appid_dns_session)
+{
+};
+
+TEST(appid_dns_session, set_host)
+{
+    AppIdDnsSession dns_session;
+    AppidChangeBits change_bits;
+
+    char* host1 = "first host";
+    dns_session.set_host(host1, change_bits, true);
+    STRCMP_EQUAL_TEXT(host1, dns_session.get_host(), "DNS session host mismatch");
+    CHECK_TEXT(change_bits.test(APPID_DNS_REQUEST_HOST_BIT), "Change bits should have DNS request bit set");
+    CHECK_TEXT(!change_bits.test(APPID_DNS_RESPONSE_HOST_BIT), "Change bits should not have DNS response bit set");
+    std::string str;
+    change_bits_to_string(change_bits, str);
+    STRCMP_EQUAL_TEXT("dns-host", str.c_str(), "Change bits string mismatch");
+    char* host2 = "second host";
+    dns_session.set_host(host2, change_bits, false);
+    STRCMP_EQUAL_TEXT(host2, dns_session.get_host(), "DNS session host mismatch");
+    CHECK_TEXT(!change_bits.test(APPID_DNS_REQUEST_HOST_BIT), "Change bits should not have DNS request bit set");
+    CHECK_TEXT(change_bits.test(APPID_DNS_RESPONSE_HOST_BIT), "Change bits should have DNS response bit set");
+    str.clear();
+    change_bits_to_string(change_bits, str);
+    STRCMP_EQUAL_TEXT("dns-response-host", str.c_str(), "Change bits string mismatch");
+    dns_session.set_host(host1, change_bits, true);
+    STRCMP_EQUAL_TEXT(host1, dns_session.get_host(), "DNS session host mismatch");
+    CHECK_TEXT(change_bits.test(APPID_DNS_REQUEST_HOST_BIT), "Change bits should have DNS request bit set");
+    CHECK_TEXT(!change_bits.test(APPID_DNS_RESPONSE_HOST_BIT), "Change bits should not have DNS response bit set");
+    str.clear();
+    change_bits_to_string(change_bits, str);
+    STRCMP_EQUAL_TEXT("dns-host", str.c_str(), "Change bits string mismatch");
+}
+
+int main(int argc, char** argv)
+{
+    int rc = CommandLineTestRunner::RunAllTests(argc, argv);
+    return rc;
+}
+

diff --git a/src/pub_sub/appid_events.h b/src/pub_sub/appid_events.h
index 80677becc75201ffffa251dca31478a0fe06eda6..4a166bc913f21e1b2679557ea503219735869449 100644 (file)
--- a/src/pub_sub/appid_events.h
+++ b/src/pub_sub/appid_events.h
@@ -54,7 +54,8 @@ enum AppidChangeBit
     APPID_REFERER_BIT,
 
     // dns
-    APPID_DNS_HOST_BIT,
+    APPID_DNS_REQUEST_HOST_BIT,
+    APPID_DNS_RESPONSE_HOST_BIT,
 
     // other
     APPID_SERVICE_INFO_BIT,
@@ -100,8 +101,10 @@ inline void change_bits_to_string(const AppidChangeBits& change_bits, std::strin
         --n? str.append("response, ") : str.append("response");
     if (change_bits.test(APPID_REFERER_BIT))
         --n? str.append("referrer, ") : str.append("referrer");
-    if (change_bits.test(APPID_DNS_HOST_BIT))
+    if (change_bits.test(APPID_DNS_REQUEST_HOST_BIT))
         --n? str.append("dns-host, ") : str.append("dns-host");
+    if (change_bits.test(APPID_DNS_RESPONSE_HOST_BIT))
+        --n? str.append("dns-response-host, ") : str.append("dns-response-host");
     if (change_bits.test(APPID_SERVICE_INFO_BIT))
         --n? str.append("service-info, ") : str.append("service-info");
     if (change_bits.test(APPID_CLIENT_INFO_BIT))