From b6b0ebbbea4c08708302899f69212a4da957b371 Mon Sep 17 00:00:00 2001 From: "Ron Dempster (rdempste)" Date: Sat, 31 May 2025 15:40:34 +0000 Subject: [PATCH] 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) Date: Wed May 14 13:28:31 2025 -0400 appid: differentiate between request and response DNS host commit a8454a7feb16cf966ec3d00c30d984caffbe1f5e Author: Ron Dempster (rdempste) Date: Fri May 9 09:27:02 2025 -0400 appid: fix tcp dns multiple transaction support --- .../appid/appid_dns_session.h | 13 +++- .../appid/detector_plugins/detector_dns.cc | 36 ++++----- .../appid/test/CMakeLists.txt | 3 + .../appid/test/appid_api_test.cc | 12 +-- .../appid/test/appid_discovery_test.cc | 8 +- .../appid/test/appid_dns_session_test.cc | 73 +++++++++++++++++++ src/pub_sub/appid_events.h | 7 +- 7 files changed, 118 insertions(+), 34 deletions(-) create mode 100644 src/network_inspectors/appid/test/appid_dns_session_test.cc diff --git a/src/network_inspectors/appid/appid_dns_session.h b/src/network_inspectors/appid/appid_dns_session.h index 266431a78..eeeccc43f 100644 --- 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 f5b55f8cb..db1d19b39 100644 --- 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 0eebbf372..aac350793 100644 --- 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 $ ) +add_cpputest( appid_dns_session_test +) + add_cpputest( appid_detector_test SOURCES $ ) diff --git a/src/network_inspectors/appid/test/appid_api_test.cc b/src/network_inspectors/appid/test/appid_api_test.cc index 7b1d892e8..ddae76216 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 == 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 94a2af541..e0b2c9454 100644 --- 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 index 000000000..059366201 --- /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 + +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include + +#include "network_inspectors/appid/appid_dns_session.h" + +#include +#include + +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 80677becc..4a166bc91 100644 --- 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)) -- 2.47.3