From a3ff0181ac7d03fafb426c67e55ca85ab02d39fd Mon Sep 17 00:00:00 2001 From: "Oleksandr Stepanov -X (ostepano - SOFTSERVE INC at Cisco)" Date: Wed, 29 May 2024 19:42:09 +0000 Subject: [PATCH] Pull request #4319: appid: Store and retrieve only SNI in AppIdSession Merge in SNORT/snort3 from ~OSTEPANO/snort3:tls_sni_event to master Squashed commit of the following: commit 6a591a048bc22e8d5fa99d6876613443dabf8352 Author: Oleksandr Stepanov Date: Fri May 3 11:01:52 2024 -0400 appid: Store and retrieve only SNI in AppIdSession --- src/network_inspectors/appid/appid_api.cc | 5 +++- src/network_inspectors/appid/appid_session.h | 25 +++++++++++++++++++ .../appid/appid_session_api.h | 14 +++++++++++ .../appid/service_plugins/service_ssl.cc | 8 +----- .../appid/test/appid_api_test.cc | 15 ++++++++++- 5 files changed, 58 insertions(+), 9 deletions(-) diff --git a/src/network_inspectors/appid/appid_api.cc b/src/network_inspectors/appid/appid_api.cc index f7ccfec66..623a99c56 100644 --- a/src/network_inspectors/appid/appid_api.cc +++ b/src/network_inspectors/appid/appid_api.cc @@ -141,7 +141,10 @@ bool AppIdApi::ssl_app_group_id_lookup(Flow* flow, const char* server_name, if (!asd->tsession) asd->tsession = new TlsSession(); else if (sni_mismatch) - asd->tsession->set_tls_host(nullptr, 0, change_bits); + { + asd->tsession->process_sni_mismatch(); + } + if (sni_mismatch) asd->scan_flags |= SCAN_SPOOFED_SNI_FLAG; diff --git a/src/network_inspectors/appid/appid_session.h b/src/network_inspectors/appid/appid_session.h index 715d3f20d..fe6e2fd11 100644 --- a/src/network_inspectors/appid/appid_session.h +++ b/src/network_inspectors/appid/appid_session.h @@ -118,6 +118,8 @@ public: snort_free(tls_cname); if (tls_org_unit) snort_free(tls_org_unit); + if (tls_host_mismatch) + snort_free(tls_host_mismatch); } const char* get_tls_host() const @@ -141,6 +143,22 @@ public: return nullptr; } + const char* get_tls_sni() const + { + return tls_host_mismatch ? tls_host_mismatch : tls_host; + } + + void process_sni_mismatch() + { + if(tls_host) + { + if(tls_host_mismatch) + snort_free(tls_host_mismatch); + tls_host_mismatch = tls_host; + tls_host = nullptr; + } + } + const char* get_tls_first_alt_name() const { return tls_first_alt_name; } const char* get_tls_cname() const { return tls_cname; } @@ -228,6 +246,7 @@ public: private: char* tls_host = nullptr; + char* tls_host_mismatch = nullptr; char* tls_first_alt_name = nullptr; char* tls_cname = nullptr; char* tls_org_unit = nullptr; @@ -601,7 +620,10 @@ public: void set_tls_host(const AppidChangeBits& change_bits) { if (tsession and change_bits[APPID_TLSHOST_BIT]) + { api.set_tls_host(tsession->get_tls_host()); + api.set_tls_sni(tsession->get_tls_sni()); + } } void set_tls_host(const char* tls_host) @@ -612,7 +634,10 @@ public: void set_tls_host() { if (tsession and tsession->is_tls_host_unpublished()) + { api.set_tls_host(tsession->get_tls_host()); + api.set_tls_sni(tsession->get_tls_sni()); + } } void set_netbios_name(AppidChangeBits& change_bits, const char *name) diff --git a/src/network_inspectors/appid/appid_session_api.h b/src/network_inspectors/appid/appid_session_api.h index fd6c4e12f..e2e0669a0 100644 --- a/src/network_inspectors/appid/appid_session_api.h +++ b/src/network_inspectors/appid/appid_session_api.h @@ -157,6 +157,8 @@ public: void clear_user_logged_in() { flags.user_logged_in = false; } + const char* get_tls_sni() const { return tls_sni; } + protected: AppIdSessionApi(const AppIdSession* asd, const SfIp& ip); @@ -176,6 +178,7 @@ private: snort::SfIp initiator_ip; ServiceAppDescriptor service; char* tls_host = nullptr; + char* tls_sni = nullptr; char* netbios_name = nullptr; char* netbios_domain = nullptr; std::string session_id; @@ -203,6 +206,7 @@ private: snort_free(tls_host); snort_free(netbios_name); snort_free(netbios_domain); + snort_free(tls_sni); delete dsession; } @@ -223,6 +227,16 @@ private: } } + void set_tls_sni(const char* sni) + { + if (sni and sni != tls_sni) + { + if (tls_sni) + snort_free(tls_sni); + tls_sni = snort_strdup(sni); + } + } + friend AppIdSession; }; diff --git a/src/network_inspectors/appid/service_plugins/service_ssl.cc b/src/network_inspectors/appid/service_plugins/service_ssl.cc index 1c6f9190b..0ad0ce70c 100644 --- a/src/network_inspectors/appid/service_plugins/service_ssl.cc +++ b/src/network_inspectors/appid/service_plugins/service_ssl.cc @@ -525,19 +525,13 @@ success: args.asd.tsession->set_tls_host(ss->client_hello.host_name, 0, args.change_bits); args.asd.scan_flags |= SCAN_SSL_HOST_FLAG; } - else if (ss->server_cert.common_name) - { - /* Use common name (from server) if we didn't get host name (from client). */ - args.asd.tsession->set_tls_host(ss->server_cert.common_name, ss->server_cert.common_name_strlen, - args.change_bits); - args.asd.scan_flags |= SCAN_SSL_HOST_FLAG; - } /* TLS Common Name */ if (ss->server_cert.common_name) { args.asd.tsession->set_tls_cname(ss->server_cert.common_name, 0, args.change_bits); args.asd.scan_flags |= SCAN_SSL_CERTIFICATE_FLAG; + args.asd.scan_flags |= SCAN_SSL_HOST_FLAG; } /* TLS Org Unit */ if (ss->server_cert.org_unit) diff --git a/src/network_inspectors/appid/test/appid_api_test.cc b/src/network_inspectors/appid/test/appid_api_test.cc index ec552b19c..eec184f3f 100644 --- a/src/network_inspectors/appid/test/appid_api_test.cc +++ b/src/network_inspectors/appid/test/appid_api_test.cc @@ -257,7 +257,7 @@ TEST(appid_api, get_application_id) TEST(appid_api, ssl_app_group_id_lookup) { - mock().expectNCalls(6, "publish"); + mock().expectNCalls(7, "publish"); AppId service, client, payload = APP_ID_NONE; bool val = false; @@ -284,6 +284,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(mock_session->tsession->get_tls_sni(), APPID_UT_TLS_HOST); STRCMP_EQUAL("Published change_bits == 00000000000100011000", test_log); // Common name based detection @@ -346,6 +347,18 @@ TEST(appid_api, ssl_app_group_id_lookup) STRCMP_EQUAL(mock_session->tsession->get_tls_cname(), APPID_UT_TLS_HOST); STRCMP_EQUAL("Published change_bits == 00000000000100011000", test_log); + //check for sni mismatch being stored in sni field + change_bits.reset(); + mock_session->tsession->set_tls_host("mismatchedsni.com", 17, change_bits); + service = APP_ID_NONE; + client = APP_ID_NONE; + payload = APP_ID_NONE; + val = appid_api.ssl_app_group_id_lookup(flow, (const char*)APPID_UT_TLS_HOST, (const char*)APPID_UT_TLS_HOST, + nullptr, nullptr, true, service, client, payload); + CHECK_TRUE(val); + STRCMP_EQUAL(APPID_UT_TLS_HOST, mock_session->tsession->get_tls_host()); + STRCMP_EQUAL("mismatchedsni.com", mock_session->tsession->get_tls_sni()); + mock().checkExpectations(); // When appid session is not existing -- 2.47.3