From 1fe6a021696a99dd9c50b089830d6992e8503708 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Tue, 13 Aug 2024 17:58:48 +0100 Subject: [PATCH 1/8] Make ssl_history work for SSLv2 handshakes/connections It turns out that the ssl_history field never was populated with C/S for SSLv2 connections, or connections using the SSLv2 handshake. In our testcases, the latter is especially common - with connections up to TLS1 using the old SSLv2 client hello for backwards compatibility. This change resolves this issue. As the history is not by default enabled in a lot of locations, baseline impact is minor. --- scripts/base/protocols/ssl/main.zeek | 10 ++++++++++ .../scripts.base.protocols.ssl.keyexchange/ssl-all.log | 2 +- .../ssl-all.log | 2 +- testing/external/commit-hash.zeek-testing | 2 +- testing/external/commit-hash.zeek-testing-private | 2 +- 5 files changed, 14 insertions(+), 4 deletions(-) diff --git a/scripts/base/protocols/ssl/main.zeek b/scripts/base/protocols/ssl/main.zeek index eadc4e20fb..f3c3f21c73 100644 --- a/scripts/base/protocols/ssl/main.zeek +++ b/scripts/base/protocols/ssl/main.zeek @@ -282,6 +282,11 @@ event ssl_client_hello(c: connection, version: count, record_version: count, pos c$ssl$session_id = bytestring_to_hexstr(session_id); c$ssl$client_ticket_empty_session_seen = F; } + + # add manually for SSLv2, since the handshake_message event is not raised, as there is no handshake protocol. + # We don't really have a direction in that case + if ( version == 2 ) + add_to_history(c, T, "c"); } event ssl_server_hello(c: connection, version: count, record_version: count, possible_ts: time, server_random: string, session_id: string, cipher: count, comp_method: count) &priority=5 @@ -302,6 +307,11 @@ event ssl_server_hello(c: connection, version: count, record_version: count, pos if ( c$ssl?$session_id && c$ssl$session_id == bytestring_to_hexstr(session_id) && c$ssl$version_num/0xFF != 0x7F && c$ssl$version_num != TLSv13 ) c$ssl$resumed = T; + + # add manually for SSLv2, since the handshake_message event is not raised, as there is no handshake protocol. + # We don't really have a direction in that case + if ( version == 2 ) + add_to_history(c, F, "s"); } event ssl_extension_supported_versions(c: connection, is_client: bool, versions: index_vec) diff --git a/testing/btest/Baseline/scripts.base.protocols.ssl.keyexchange/ssl-all.log b/testing/btest/Baseline/scripts.base.protocols.ssl.keyexchange/ssl-all.log index 91d81e7b40..8eef3ece7a 100644 --- a/testing/btest/Baseline/scripts.base.protocols.ssl.keyexchange/ssl-all.log +++ b/testing/btest/Baseline/scripts.base.protocols.ssl.keyexchange/ssl-all.log @@ -27,7 +27,7 @@ XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.18.50 56981 74.125.239.97 443 TLSv12 #open XXXX-XX-XX-XX-XX-XX #fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p version cipher curve server_name resumed last_alert next_protocol established ssl_history cert_chain_fps client_cert_chain_fps sni_matches_cert client_record_version client_random client_cipher_suites server_record_version server_random server_dh_p server_dh_q server_dh_Ys server_ecdh_point server_signature_sig_alg server_signature_hash_alg server_signature server_cert_sha1 client_rsa_pms client_dh_Yc client_ecdh_point #types time string addr port addr port string string string string bool string string bool string vector[string] vector[string] bool string string string string string string string string string count count string string string string string -XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.150.187.164 58868 194.127.84.106 443 TLSv10 TLS_RSA_WITH_RC4_128_MD5 - - F - - T sxnGIi ddd0218a34972ceab3d200b78959bd2b4c95eadf37399df35bfd68a5b658bc78,ba352de8d8faa0ecfdbeee560fa308fe192023d3b18d83a68845933bebf28360 (empty) - unknown-0 e6b8efdf91cf44f7eae43c83398fdcb2 TLS_DHE_RSA_WITH_AES_256_CBC_SHA,TLS_DHE_DSS_WITH_AES_256_CBC_SHA,TLS_RSA_WITH_AES_256_CBC_SHA,TLS_DHE_RSA_WITH_AES_128_CBC_SHA,TLS_DHE_DSS_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_RC4_128_MD5,TLS_RSA_WITH_RC4_128_SHA,TLS_RSA_WITH_AES_128_CBC_SHA,TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA,TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA,SSL_RSA_FIPS_WITH_3DES_EDE_CBC_SHA,TLS_RSA_WITH_3DES_EDE_CBC_SHA,TLS_DHE_RSA_WITH_DES_CBC_SHA,TLS_DHE_DSS_WITH_DES_CBC_SHA,SSL_RSA_FIPS_WITH_DES_CBC_SHA,TLS_RSA_WITH_DES_CBC_SHA,TLS_RSA_EXPORT1024_WITH_RC4_56_SHA,TLS_RSA_EXPORT1024_WITH_DES_CBC_SHA,TLS_RSA_EXPORT_WITH_RC4_40_MD5,TLS_RSA_EXPORT_WITH_RC2_CBC_40_MD5 TLSv10 45c7bb492b658d5183bbaedbf35e8f126ff926b14979cd703d242aea996a5fda - - - - - - - 2c322ae2b7fe91391345e070b63668978bb1c9da 008057aaeea52e6d030e54fa9328781fda6f8de80ed8531946bfa8adc4b51ca7502cbce62bae6949f6b865d7125e256643b5ede4dd4cf42107cfa73c418f10881edf38a75f968b507f08f9c1089ef26bfd322cf44c0b746b8e3dff731f2585dcf26abb048d55e661e1d2868ccc9c338e451c30431239f96a00e4843b6aa00ba51785 - - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.150.187.164 58868 194.127.84.106 443 TLSv10 TLS_RSA_WITH_RC4_128_MD5 - - F - - T CsxnGIi ddd0218a34972ceab3d200b78959bd2b4c95eadf37399df35bfd68a5b658bc78,ba352de8d8faa0ecfdbeee560fa308fe192023d3b18d83a68845933bebf28360 (empty) - unknown-0 e6b8efdf91cf44f7eae43c83398fdcb2 TLS_DHE_RSA_WITH_AES_256_CBC_SHA,TLS_DHE_DSS_WITH_AES_256_CBC_SHA,TLS_RSA_WITH_AES_256_CBC_SHA,TLS_DHE_RSA_WITH_AES_128_CBC_SHA,TLS_DHE_DSS_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_RC4_128_MD5,TLS_RSA_WITH_RC4_128_SHA,TLS_RSA_WITH_AES_128_CBC_SHA,TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA,TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA,SSL_RSA_FIPS_WITH_3DES_EDE_CBC_SHA,TLS_RSA_WITH_3DES_EDE_CBC_SHA,TLS_DHE_RSA_WITH_DES_CBC_SHA,TLS_DHE_DSS_WITH_DES_CBC_SHA,SSL_RSA_FIPS_WITH_DES_CBC_SHA,TLS_RSA_WITH_DES_CBC_SHA,TLS_RSA_EXPORT1024_WITH_RC4_56_SHA,TLS_RSA_EXPORT1024_WITH_DES_CBC_SHA,TLS_RSA_EXPORT_WITH_RC4_40_MD5,TLS_RSA_EXPORT_WITH_RC2_CBC_40_MD5 TLSv10 45c7bb492b658d5183bbaedbf35e8f126ff926b14979cd703d242aea996a5fda - - - - - - - 2c322ae2b7fe91391345e070b63668978bb1c9da 008057aaeea52e6d030e54fa9328781fda6f8de80ed8531946bfa8adc4b51ca7502cbce62bae6949f6b865d7125e256643b5ede4dd4cf42107cfa73c418f10881edf38a75f968b507f08f9c1089ef26bfd322cf44c0b746b8e3dff731f2585dcf26abb048d55e661e1d2868ccc9c338e451c30431239f96a00e4843b6aa00ba51785 - - XXXXXXXXXX.XXXXXX ClEkJM2Vm5giqnMf4h 192.150.187.164 58869 194.127.84.106 443 TLSv10 TLS_RSA_WITH_RC4_128_MD5 - - F - - T CsxnGIi ddd0218a34972ceab3d200b78959bd2b4c95eadf37399df35bfd68a5b658bc78,ba352de8d8faa0ecfdbeee560fa308fe192023d3b18d83a68845933bebf28360 (empty) - TLSv10 a8a2ab739a64abb4e68cfcfc3470ff6269b1a86858501fbbd1327ed8 TLS_DHE_RSA_WITH_AES_256_CBC_SHA,TLS_DHE_DSS_WITH_AES_256_CBC_SHA,TLS_RSA_WITH_AES_256_CBC_SHA,TLS_DHE_RSA_WITH_AES_128_CBC_SHA,TLS_DHE_DSS_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_RC4_128_MD5,TLS_RSA_WITH_RC4_128_SHA,TLS_RSA_WITH_AES_128_CBC_SHA,TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA,TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA,SSL_RSA_FIPS_WITH_3DES_EDE_CBC_SHA,TLS_RSA_WITH_3DES_EDE_CBC_SHA,TLS_DHE_RSA_WITH_DES_CBC_SHA,TLS_DHE_DSS_WITH_DES_CBC_SHA,SSL_RSA_FIPS_WITH_DES_CBC_SHA,TLS_RSA_WITH_DES_CBC_SHA,TLS_RSA_EXPORT1024_WITH_RC4_56_SHA,TLS_RSA_EXPORT1024_WITH_DES_CBC_SHA,TLS_RSA_EXPORT_WITH_RC4_40_MD5,TLS_RSA_EXPORT_WITH_RC2_CBC_40_MD5 TLSv10 45c7bb4c0fac7f7823587c68438c87876533af7b0baa2a8f1078eb8d182247e9 - - - - - - - 2c322ae2b7fe91391345e070b63668978bb1c9da 0080891c1b6b5f0ec9da1b38d5ba6efe9c0380219d1ac4e63a0e8993306cddc6944a57c9292beb5652794181f747d0e868b84dca7dfe9783d1baa2ef3bb68d929b2818c5b58b8f47663220f9781fa469fea7e7d17d410d3979aa15a7be651c9f16fbf1a04f87a95e742c3fe20ca6faf0d2e950708533fd3346e17e410f0f86c01f52 - - XXXXXXXXXX.XXXXXX C4J4Th3PJpwUYZZ6gc 192.150.187.164 58870 194.127.84.106 443 TLSv10 TLS_RSA_WITH_RC4_128_MD5 - - F - - T CsxnGIi ddd0218a34972ceab3d200b78959bd2b4c95eadf37399df35bfd68a5b658bc78,ba352de8d8faa0ecfdbeee560fa308fe192023d3b18d83a68845933bebf28360 (empty) - TLSv10 240604be2f5644c8dfd2e51cc2b3a30171bd58853ed7c6e3fcd18846 TLS_DHE_RSA_WITH_AES_256_CBC_SHA,TLS_DHE_DSS_WITH_AES_256_CBC_SHA,TLS_RSA_WITH_AES_256_CBC_SHA,TLS_DHE_RSA_WITH_AES_128_CBC_SHA,TLS_DHE_DSS_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_RC4_128_MD5,TLS_RSA_WITH_RC4_128_SHA,TLS_RSA_WITH_AES_128_CBC_SHA,TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA,TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA,SSL_RSA_FIPS_WITH_3DES_EDE_CBC_SHA,TLS_RSA_WITH_3DES_EDE_CBC_SHA,TLS_DHE_RSA_WITH_DES_CBC_SHA,TLS_DHE_DSS_WITH_DES_CBC_SHA,SSL_RSA_FIPS_WITH_DES_CBC_SHA,TLS_RSA_WITH_DES_CBC_SHA,TLS_RSA_EXPORT1024_WITH_RC4_56_SHA,TLS_RSA_EXPORT1024_WITH_DES_CBC_SHA,TLS_RSA_EXPORT_WITH_RC4_40_MD5,TLS_RSA_EXPORT_WITH_RC2_CBC_40_MD5 TLSv10 45c7bb4ffd1b8c1308a2caac010fcb76e9bd21987d897cb6c028cdb3176d5904 - - - - - - - 2c322ae2b7fe91391345e070b63668978bb1c9da 008032a6f5fd530f342e4d5b4043765005ba018f488800f897c259b005ad2a544f5800e99812d9a6336e84b07e4595d1b8ae00a582d91804fe715c132d1bdb112e66361db80a57a441fc8ea784ea76ec44b9f3a0f9ddc29be68010ff3bcfffc285a294511991d7952cbbfee88a869818bae31f32f7099b0754d9ce75b8fea887e1b8 - - #close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/scripts.policy.protocols.ssl.ssl-log-ext/ssl-all.log b/testing/btest/Baseline/scripts.policy.protocols.ssl.ssl-log-ext/ssl-all.log index a2cd6f882c..fc8d05ef2b 100644 --- a/testing/btest/Baseline/scripts.policy.protocols.ssl.ssl-log-ext/ssl-all.log +++ b/testing/btest/Baseline/scripts.policy.protocols.ssl.ssl-log-ext/ssl-all.log @@ -27,7 +27,7 @@ XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.18.50 56981 74.125.239.97 443 TLSv12 #open XXXX-XX-XX-XX-XX-XX #fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p version cipher curve server_name resumed last_alert next_protocol established ssl_history cert_chain_fps client_cert_chain_fps sni_matches_cert server_version client_version client_ciphers ssl_client_exts ssl_server_exts ticket_lifetime_hint dh_param_size point_formats client_curves orig_alpn client_supported_versions server_supported_version psk_key_exchange_modes client_key_share_groups server_key_share_group client_comp_methods sigalgs hashalgs #types time string addr port addr port string string string string bool string string bool string vector[string] vector[string] bool count count vector[count] vector[count] vector[count] count count vector[count] vector[count] vector[string] vector[count] count vector[count] vector[count] count vector[count] vector[count] vector[count] -XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.150.187.164 58868 194.127.84.106 443 TLSv10 TLS_RSA_WITH_RC4_128_MD5 - - F - - T sxnGIi ddd0218a34972ceab3d200b78959bd2b4c95eadf37399df35bfd68a5b658bc78,ba352de8d8faa0ecfdbeee560fa308fe192023d3b18d83a68845933bebf28360 (empty) - 769 2 57,56,53,51,50,4,5,47,22,19,65279,10,21,18,65278,9,100,98,3,6 - - - - - - - - - - - - (empty) - - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.150.187.164 58868 194.127.84.106 443 TLSv10 TLS_RSA_WITH_RC4_128_MD5 - - F - - T CsxnGIi ddd0218a34972ceab3d200b78959bd2b4c95eadf37399df35bfd68a5b658bc78,ba352de8d8faa0ecfdbeee560fa308fe192023d3b18d83a68845933bebf28360 (empty) - 769 2 57,56,53,51,50,4,5,47,22,19,65279,10,21,18,65278,9,100,98,3,6 - - - - - - - - - - - - (empty) - - XXXXXXXXXX.XXXXXX ClEkJM2Vm5giqnMf4h 192.150.187.164 58869 194.127.84.106 443 TLSv10 TLS_RSA_WITH_RC4_128_MD5 - - F - - T CsxnGIi ddd0218a34972ceab3d200b78959bd2b4c95eadf37399df35bfd68a5b658bc78,ba352de8d8faa0ecfdbeee560fa308fe192023d3b18d83a68845933bebf28360 (empty) - 769 769 57,56,53,51,50,4,5,47,22,19,65279,10,21,18,65278,9,100,98,3,6 - - - - - - - - - - - - 0 - - XXXXXXXXXX.XXXXXX C4J4Th3PJpwUYZZ6gc 192.150.187.164 58870 194.127.84.106 443 TLSv10 TLS_RSA_WITH_RC4_128_MD5 - - F - - T CsxnGIi ddd0218a34972ceab3d200b78959bd2b4c95eadf37399df35bfd68a5b658bc78,ba352de8d8faa0ecfdbeee560fa308fe192023d3b18d83a68845933bebf28360 (empty) - 769 769 57,56,53,51,50,4,5,47,22,19,65279,10,21,18,65278,9,100,98,3,6 - - - - - - - - - - - - 0 - - #close XXXX-XX-XX-XX-XX-XX diff --git a/testing/external/commit-hash.zeek-testing b/testing/external/commit-hash.zeek-testing index c6c71cea50..0dafd33e69 100644 --- a/testing/external/commit-hash.zeek-testing +++ b/testing/external/commit-hash.zeek-testing @@ -1 +1 @@ -4f808b8e1a4e99e738af85f15a5534c0ee99cdba +e3a2d006118261fe49e10ac44f6668204e21ccd4 diff --git a/testing/external/commit-hash.zeek-testing-private b/testing/external/commit-hash.zeek-testing-private index b41c8fda5c..c0b142fede 100644 --- a/testing/external/commit-hash.zeek-testing-private +++ b/testing/external/commit-hash.zeek-testing-private @@ -1 +1 @@ -3df94cb39ab9c0079e82a7f2cd5edb561c2ec07b +09f0fa3be12ca58ce045f3009e36e7fff0f2ccbf From 547144d07ee7bdf4a4d5b855d8fa78c5e11f69f2 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Fri, 16 Aug 2024 16:55:56 +0200 Subject: [PATCH 2/8] Revert "Remove deprecated port/ports fields for spicy analyzers" This reverts commit 15d404dd191a723960e4efd956eec22739d3f1c2. --- scripts/spicy/zeek_rt.hlt | 2 +- src/spicy/manager.cc | 21 ++++ src/spicy/manager.h | 11 +- src/spicy/runtime-support.cc | 9 +- src/spicy/runtime-support.h | 7 +- src/spicy/spicyz/glue-compiler.cc | 112 ++++++++++++++++++- src/spicy/spicyz/glue-compiler.h | 1 + testing/btest/spicy/event-user-type | 10 +- testing/btest/spicy/port-deprecated.evt | 21 ++++ testing/btest/spicy/port-fail.evt | 24 ++++ testing/btest/spicy/port-range-one-port.zeek | 24 ++++ 11 files changed, 217 insertions(+), 25 deletions(-) create mode 100644 testing/btest/spicy/port-deprecated.evt create mode 100644 testing/btest/spicy/port-fail.evt create mode 100644 testing/btest/spicy/port-range-one-port.zeek diff --git a/scripts/spicy/zeek_rt.hlt b/scripts/spicy/zeek_rt.hlt index 801c49a9b1..3f4dd28adc 100644 --- a/scripts/spicy/zeek_rt.hlt +++ b/scripts/spicy/zeek_rt.hlt @@ -18,7 +18,7 @@ type ZeekTypeTag = enum { } &cxxname="::zeek::spicy::rt::ZeekTypeTag"; declare public void register_spicy_module_begin(string name, string description) &cxxname="zeek::spicy::rt::register_spicy_module_begin"; -declare public void register_protocol_analyzer(string name, hilti::Protocol protocol, string parser_orig, string parser_resp, string replaces, string linker_scope) &cxxname="zeek::spicy::rt::register_protocol_analyzer" &have_prototype; +declare public void register_protocol_analyzer(string name, hilti::Protocol protocol, vector ports, string parser_orig, string parser_resp, string replaces, string linker_scope) &cxxname="zeek::spicy::rt::register_protocol_analyzer" &have_prototype; declare public void register_file_analyzer(string name, vector mime_types, string parser, string replaces, string linker_scope) &cxxname="zeek::spicy::rt::register_file_analyzer" &have_prototype; declare public void register_packet_analyzer(string name, string parser, string replaces, string linker_scope) &cxxname="zeek::spicy::rt::register_packet_analyzer" &have_prototype; declare public void register_type(string ns, string id, BroType t) &cxxname="zeek::spicy::rt::register_type" &have_prototype; diff --git a/src/spicy/manager.cc b/src/spicy/manager.cc index 7919380111..1a9420e22a 100644 --- a/src/spicy/manager.cc +++ b/src/spicy/manager.cc @@ -61,6 +61,7 @@ void Manager::registerSpicyModuleEnd() { } void Manager::registerProtocolAnalyzer(const std::string& name, hilti::rt::Protocol proto, + const hilti::rt::Vector<::zeek::spicy::rt::PortRange>& ports, const std::string& parser_orig, const std::string& parser_resp, const std::string& replaces, const std::string& linker_scope) { SPICY_DEBUG(hilti::rt::fmt("Have Spicy protocol analyzer %s", name)); @@ -73,6 +74,7 @@ void Manager::registerProtocolAnalyzer(const std::string& name, hilti::rt::Proto info.name_zeek = hilti::rt::replace(name, "::", "_"); info.name_zeekygen = hilti::rt::fmt("", name); info.protocol = proto; + info.ports = ports; info.linker_scope = linker_scope; // We may have that analyzer already iff it was previously pre-registered @@ -699,6 +701,25 @@ void Manager::InitPostScript() { if ( ! tag ) reporter->InternalError("cannot get analyzer tag for '%s'", p.name_analyzer.c_str()); + for ( const auto& ports : p.ports ) { + const auto proto = ports.begin.protocol(); + + // Port ranges are closed intervals. + for ( auto port = ports.begin.port(); port <= ports.end.port(); ++port ) { + const auto port_ = hilti::rt::Port(port, proto); + SPICY_DEBUG(hilti::rt::fmt(" Scheduling analyzer for port %s", port_)); + analyzer_mgr->RegisterAnalyzerForPort(tag, transport_protocol(port_), port); + + // Don't double register in case of single-port ranges. + if ( ports.begin.port() == ports.end.port() ) + break; + + // Explicitly prevent overflow. + if ( port == std::numeric_limits::max() ) + break; + } + } + if ( p.parser_resp ) { for ( auto port : p.parser_resp->ports ) { if ( port.direction != ::spicy::rt::Direction::Both && diff --git a/src/spicy/manager.h b/src/spicy/manager.h index 195ae3adf1..118e03b6c3 100644 --- a/src/spicy/manager.h +++ b/src/spicy/manager.h @@ -85,6 +85,7 @@ public: * * @param name name of the analyzer as defined in its EVT file * @param proto analyzer's transport-layer protocol + * @param prts well-known ports for the analyzer; it'll be activated automatically for these * @param parser_orig name of the Spicy parser for the originator side; must match the name that * Spicy registers the unit's parser with * @param parser_resp name of the Spicy parser for the originator side; must match the name that @@ -94,9 +95,10 @@ public: * @param linker_scope scope of current HLTO file, which will restrict visibility of the * registration */ - void registerProtocolAnalyzer(const std::string& name, hilti::rt::Protocol proto, const std::string& parser_orig, - const std::string& parser_resp, const std::string& replaces, - const std::string& linker_scope); + void registerProtocolAnalyzer(const std::string& name, hilti::rt::Protocol proto, + const hilti::rt::Vector<::zeek::spicy::rt::PortRange>& ports, + const std::string& parser_orig, const std::string& parser_resp, + const std::string& replaces, const std::string& linker_scope); /** * Runtime method to register a file analyzer with its Zeek-side @@ -341,6 +343,7 @@ private: std::string name_parser_resp; std::string name_replaces; hilti::rt::Protocol protocol = hilti::rt::Protocol::Undef; + hilti::rt::Vector<::zeek::spicy::rt::PortRange> ports; std::string linker_scope; // Computed and available once the analyzer has been registered. @@ -354,7 +357,7 @@ private: bool operator==(const ProtocolAnalyzerInfo& other) const { return name_analyzer == other.name_analyzer && name_parser_orig == other.name_parser_orig && name_parser_resp == other.name_parser_resp && name_replaces == other.name_replaces && - protocol == other.protocol && linker_scope == other.linker_scope; + protocol == other.protocol && ports == other.ports && linker_scope == other.linker_scope; } bool operator!=(const ProtocolAnalyzerInfo& other) const { return ! (*this == other); } diff --git a/src/spicy/runtime-support.cc b/src/spicy/runtime-support.cc index 8dbf0c39a7..f5afd37461 100644 --- a/src/spicy/runtime-support.cc +++ b/src/spicy/runtime-support.cc @@ -26,11 +26,12 @@ void rt::register_spicy_module_begin(const std::string& name, const std::string& void rt::register_spicy_module_end() { spicy_mgr->registerSpicyModuleEnd(); } -void rt::register_protocol_analyzer(const std::string& name, hilti::rt::Protocol proto, const std::string& parser_orig, - const std::string& parser_resp, const std::string& replaces, - const std::string& linker_scope) { +void rt::register_protocol_analyzer(const std::string& name, hilti::rt::Protocol proto, + const hilti::rt::Vector<::zeek::spicy::rt::PortRange>& ports, + const std::string& parser_orig, const std::string& parser_resp, + const std::string& replaces, const std::string& linker_scope) { auto _ = hilti::rt::profiler::start("zeek/rt/register_protocol_analyzer"); - spicy_mgr->registerProtocolAnalyzer(name, proto, parser_orig, parser_resp, replaces, linker_scope); + spicy_mgr->registerProtocolAnalyzer(name, proto, ports, parser_orig, parser_resp, replaces, linker_scope); } void rt::register_file_analyzer(const std::string& name, const hilti::rt::Vector& mime_types, diff --git a/src/spicy/runtime-support.h b/src/spicy/runtime-support.h index c0bf9f4631..0397dc86cc 100644 --- a/src/spicy/runtime-support.h +++ b/src/spicy/runtime-support.h @@ -106,9 +106,10 @@ void register_spicy_module_begin(const std::string& id, const std::string& descr * Registers a Spicy protocol analyzer with its EVT meta information with the * plugin's runtime. */ -void register_protocol_analyzer(const std::string& id, hilti::rt::Protocol proto, const std::string& parser_orig, - const std::string& parser_resp, const std::string& replaces, - const std::string& linker_scope); +void register_protocol_analyzer(const std::string& id, hilti::rt::Protocol proto, + const hilti::rt::Vector<::zeek::spicy::rt::PortRange>& ports, + const std::string& parser_orig, const std::string& parser_resp, + const std::string& replaces, const std::string& linker_scope); /** * Registers a Spicy file analyzer with its EVT meta information with the diff --git a/src/spicy/spicyz/glue-compiler.cc b/src/spicy/spicyz/glue-compiler.cc index c34d7e1f6b..e9240ed245 100644 --- a/src/spicy/spicyz/glue-compiler.cc +++ b/src/spicy/spicyz/glue-compiler.cc @@ -260,6 +260,79 @@ static std::string extract_expr(const std::string& chunk, size_t* i) { return expr; } +static hilti::rt::Port extract_port(const std::string& chunk, size_t* i) { + eat_spaces(chunk, i); + + std::string s; + size_t j = *i; + + while ( j < chunk.size() && isdigit(chunk[j]) ) + ++j; + + if ( *i == j ) + throw ParseError("cannot parse port specification"); + + hilti::rt::Protocol proto; + uint64_t port = std::numeric_limits::max(); + + s = chunk.substr(*i, j - *i); + hilti::util::atoi_n(s.begin(), s.end(), 10, &port); + + if ( port > 65535 ) + throw ParseError("port outside of valid range"); + + *i = j; + + if ( chunk[*i] != '/' ) + throw ParseError("cannot parse port specification"); + + (*i)++; + + if ( looking_at(chunk, *i, "tcp") ) { + proto = hilti::rt::Protocol::TCP; + eat_token(chunk, i, "tcp"); + } + + else if ( looking_at(chunk, *i, "udp") ) { + proto = hilti::rt::Protocol::UDP; + eat_token(chunk, i, "udp"); + } + + else if ( looking_at(chunk, *i, "icmp") ) { + proto = hilti::rt::Protocol::ICMP; + eat_token(chunk, i, "icmp"); + } + + else + throw ParseError("cannot parse port specification"); + + return {static_cast(port), proto}; +} + +static ::zeek::spicy::rt::PortRange extract_port_range(const std::string& chunk, size_t* i) { + auto start = extract_port(chunk, i); + auto end = std::optional(); + + if ( looking_at(chunk, *i, "-") ) { + eat_token(chunk, i, "-"); + end = extract_port(chunk, i); + } + + if ( end ) { + if ( start.protocol() != end->protocol() ) + throw ParseError("start and end of port range must have same protocol"); + + if ( start.port() > end->port() ) + throw ParseError("start of port range cannot be after its end"); + } + + if ( ! end ) + // EVT port ranges are a closed. + end = hilti::rt::Port(start.port(), start.protocol()); + + return {start, *end}; +} + void GlueCompiler::init(Driver* driver, int zeek_version) { _driver = driver; _zeek_version = zeek_version; @@ -631,11 +704,25 @@ glue::ProtocolAnalyzer GlueCompiler::parseProtocolAnalyzer(const std::string& ch } } - else if ( looking_at(chunk, i, "ports") || looking_at(chunk, i, "port") ) { - throw ParseError(hilti::rt::fmt( - "Analyzer %s is using the removed 'port' or 'ports' keyword to register " - "well-known ports. Use Analyzer::register_for_ports() in the accompanying Zeek script instead.", - a.name)); + else if ( looking_at(chunk, i, "ports") ) { + eat_token(chunk, &i, "ports"); + eat_token(chunk, &i, "{"); + + while ( true ) { + a.ports.push_back(extract_port_range(chunk, &i)); + + if ( looking_at(chunk, i, "}") ) { + eat_token(chunk, &i, "}"); + break; + } + + eat_token(chunk, &i, ","); + } + } + + else if ( looking_at(chunk, i, "port") ) { + eat_token(chunk, &i, "port"); + a.ports.push_back(extract_port_range(chunk, &i)); } else if ( looking_at(chunk, i, "replaces") ) { @@ -652,6 +739,14 @@ glue::ProtocolAnalyzer GlueCompiler::parseProtocolAnalyzer(const std::string& ch eat_token(chunk, &i, ","); } + if ( ! a.ports.empty() ) + hilti::logger().warning( + hilti::rt:: + fmt("Remove in v7.1: Analyzer %s is using the deprecated 'port' or 'ports' keyword to register " + "well-known ports. Use Analyzer::register_for_ports() in the accompanying Zeek script instead.", + a.name), + a.location); + return a; } @@ -939,6 +1034,13 @@ bool GlueCompiler::compile() { preinit_body.addCall("zeek_rt::register_protocol_analyzer", {builder()->stringMutable(a.name.str()), builder()->id(protocol), + builder()->vector( + hilti::util::transform(a.ports, + [this](const auto& p) -> hilti::Expression* { + return builder()->call("zeek_rt::make_port_range", + {builder()->port(p.begin), + builder()->port(p.end)}); + })), builder()->stringMutable(a.unit_name_orig.str()), builder()->stringMutable(a.unit_name_resp.str()), builder()->stringMutable(a.replaces), builder()->scope()}); diff --git a/src/spicy/spicyz/glue-compiler.h b/src/spicy/spicyz/glue-compiler.h index 22ffcdc332..58e42909f3 100644 --- a/src/spicy/spicyz/glue-compiler.h +++ b/src/spicy/spicyz/glue-compiler.h @@ -45,6 +45,7 @@ struct ProtocolAnalyzer { hilti::Location location; /**< Location where the analyzer was defined. */ hilti::ID name; /**< Name of the analyzer. */ hilti::rt::Protocol protocol = hilti::rt::Protocol::Undef; /**< The transport layer the analyzer uses. */ + std::vector<::zeek::spicy::rt::PortRange> ports; /**< The ports associated with the analyzer. */ hilti::ID unit_name_orig; /**< The fully-qualified name of the unit type to parse the originator side. */ hilti::ID unit_name_resp; /**< The fully-qualified name of the unit type to parse the originator diff --git a/testing/btest/spicy/event-user-type b/testing/btest/spicy/event-user-type index 14b0883d5f..75f99b4042 100644 --- a/testing/btest/spicy/event-user-type +++ b/testing/btest/spicy/event-user-type @@ -25,7 +25,8 @@ type Y = unit { # @TEST-START-FILE foo.evt protocol analyzer spicy::foo over UDP: - parse with foo::X; + parse with foo::X, + ports { 12345/udp, 31337/udp }; import foo; @@ -35,13 +36,6 @@ on foo::X -> event foo::X($conn, $is_orig, self.y); # @TEST-END-FILE # @TEST-START-FILE foo.zeek -const foo_ports = { 12345/udp, 31337/udp}; - -event zeek_init() -{ - Analyzer::register_for_ports(Analyzer::ANALYZER_SPICY_FOO, foo_ports); -} - event foo::X(c: connection, is_orig: bool, y: foo::Y) { print fmt("is_orig=%d y=%s", is_orig, y); diff --git a/testing/btest/spicy/port-deprecated.evt b/testing/btest/spicy/port-deprecated.evt new file mode 100644 index 0000000000..220a9d1faf --- /dev/null +++ b/testing/btest/spicy/port-deprecated.evt @@ -0,0 +1,21 @@ +# @TEST-REQUIRES: have-spicy +# +# @TEST-EXEC: spicyz -d -o test.hlto ./udp-test.evt 2>out.stderr +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff out.stderr +# +# @TEST-DOC: Remove with v7.1: Specifying ports is deprecated. + +module Test; + +import zeek; + +public type Message = unit { + data: bytes &eod {} +}; + +# @TEST-START-FILE udp-test.evt +protocol analyzer spicy::TEST over UDP: + parse with Test::Message, + port 11337/udp-11340/udp, + ports {31337/udp-31340/udp}; +# @TEST-END-FILE diff --git a/testing/btest/spicy/port-fail.evt b/testing/btest/spicy/port-fail.evt new file mode 100644 index 0000000000..e51ca0fb79 --- /dev/null +++ b/testing/btest/spicy/port-fail.evt @@ -0,0 +1,24 @@ +# @TEST-REQUIRES: have-spicy +# +# @TEST-EXEC-FAIL: spicyz %INPUT -d -o x.hlto >output 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=diff-canonifier-spicy btest-diff output +# +# @TEST-DOC: Remove with v7.1 + +protocol analyzer spicy::SSH over TCP: + port 123456/udp; + +@TEST-START-NEXT + +protocol analyzer spicy::SSH over TCP: + port -1/udp; + +@TEST-START-NEXT + +protocol analyzer spicy::SSH over TCP: + port 1/udp-2/tcp; + +@TEST-START-NEXT + +protocol analyzer spicy::SSH over TCP: + port 2/udp-1/udp; diff --git a/testing/btest/spicy/port-range-one-port.zeek b/testing/btest/spicy/port-range-one-port.zeek new file mode 100644 index 0000000000..95c32f2b27 --- /dev/null +++ b/testing/btest/spicy/port-range-one-port.zeek @@ -0,0 +1,24 @@ +# @TEST-REQUIRES: have-spicy +# +# @TEST-EXEC: spicyz -o test.hlto udp-test.spicy ./udp-test.evt +# @TEST-EXEC: HILTI_DEBUG=zeek zeek -Cr ${TRACES}/udp-packet.pcap test.hlto %INPUT >out 2>&1 +# @TEST-EXEC: grep -e 'Scheduling analyzer' -e 'error during parsing' < out > out.filtered +# @TEST-EXEC: btest-diff out.filtered + +# @TEST-DOC: Remove with v7.1. Expect a single 'Scheduling analyzer ...' message in the debug output and no parsing errors. There was a bug that 'port 31336/udp' would be wrongly interpreted as a 31336/udp-31337/udp port range. Regression test for #3278. + +# @TEST-START-FILE udp-test.spicy +module UDPTest; + +public type Message = unit { + data: bytes &eod { + assert False: "not reached"; + } +}; +# @TEST-END-FILE + +# @TEST-START-FILE udp-test.evt +protocol analyzer spicy::UDP_TEST over UDP: + parse with UDPTest::Message, + port 31336/udp; +# @TEST-END-FILE From 4b369bad2d1b83beaaf276450538e3081c8547ea Mon Sep 17 00:00:00 2001 From: zeek-bot Date: Wed, 21 Aug 2024 00:14:27 +0000 Subject: [PATCH 3/8] Update doc submodule [nomail] [skip ci] --- doc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc b/doc index 2f14aee921..425ce7933d 160000 --- a/doc +++ b/doc @@ -1 +1 @@ -Subproject commit 2f14aee921e8f7df417423079f9341e1e72c5d7e +Subproject commit 425ce7933df90afdd34c7d5695b17d44a13ae8a7 From 0d3296590d66b02711b6e4d9d4ae394618cebdab Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Fri, 16 Aug 2024 17:51:01 +0200 Subject: [PATCH 4/8] Spicy: Register well-known ports through an event handler. This avoids the earlier problem of not tracking ports correctly in scriptland, while still supporting `port` in EVT files and `%port` in Spicy files. As it turns out we are already following the same approach for file analyzers' MIME types, so I'm applying the same pattern: it's one event per port, without further customization points. That leaves the patch pretty small after all while fixing the original issue. --- doc | 2 +- .../base/frameworks/spicy/init-framework.zeek | 8 ++++- src/spicy/manager.cc | 25 ++++++++++++--- src/spicy/manager.h | 4 +-- src/spicy/port-range.h | 5 +++ src/spicy/spicyz/glue-compiler.cc | 8 ----- .../Baseline/spicy.port-deprecated/out.stderr | 2 -- testing/btest/Baseline/spicy.port-fail/output | 2 +- testing/btest/Baseline/spicy.port/output | 19 +++++++++++ testing/btest/spicy/port-deprecated.evt | 21 ------------ testing/btest/spicy/port-fail.evt | 2 -- testing/btest/spicy/port-range-one-port.zeek | 2 +- testing/btest/spicy/port.zeek | 32 +++++++++++++++++++ 13 files changed, 89 insertions(+), 43 deletions(-) delete mode 100644 testing/btest/Baseline/spicy.port-deprecated/out.stderr create mode 100644 testing/btest/Baseline/spicy.port/output delete mode 100644 testing/btest/spicy/port-deprecated.evt create mode 100644 testing/btest/spicy/port.zeek diff --git a/doc b/doc index 2f14aee921..d2a0223f33 160000 --- a/doc +++ b/doc @@ -1 +1 @@ -Subproject commit 2f14aee921e8f7df417423079f9341e1e72c5d7e +Subproject commit d2a0223f337c0dd9f029910e894b4d3bd5c6a4a1 diff --git a/scripts/base/frameworks/spicy/init-framework.zeek b/scripts/base/frameworks/spicy/init-framework.zeek index ae3a3b8e65..de6b528ee4 100644 --- a/scripts/base/frameworks/spicy/init-framework.zeek +++ b/scripts/base/frameworks/spicy/init-framework.zeek @@ -47,12 +47,18 @@ export { # Marked with &is_used to suppress complaints when there aren't any # Spicy file analyzers loaded, and hence this event can't be generated. -# The attribute is only supported for Zeek 5.0 and higher. event spicy_analyzer_for_mime_type(a: Files::Tag, mt: string) &is_used { Files::register_for_mime_type(a, mt); } +# Marked with &is_used to suppress complaints when there aren't any +# Spicy protocol analyzers loaded, and hence this event can't be generated. +event spicy_analyzer_for_port(a: Analyzer::Tag, p: port) &is_used + { + Analyzer::register_for_port(a, p); + } + function enable_protocol_analyzer(tag: Analyzer::Tag) : bool { return Spicy::__toggle_analyzer(tag, T); diff --git a/src/spicy/manager.cc b/src/spicy/manager.cc index 1a9420e22a..423febf1c9 100644 --- a/src/spicy/manager.cc +++ b/src/spicy/manager.cc @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -32,6 +33,7 @@ #include "zeek/spicy/file-analyzer.h" #include "zeek/spicy/packet-analyzer.h" #include "zeek/spicy/protocol-analyzer.h" +#include "zeek/spicy/runtime-support.h" #include "zeek/zeek-config-paths.h" using namespace zeek; @@ -74,9 +76,13 @@ void Manager::registerProtocolAnalyzer(const std::string& name, hilti::rt::Proto info.name_zeek = hilti::rt::replace(name, "::", "_"); info.name_zeekygen = hilti::rt::fmt("", name); info.protocol = proto; - info.ports = ports; info.linker_scope = linker_scope; + // Store ports in a deterministic order. We can't (easily) sort the + // `hilti::rt::Vector` unfortunately. + std::copy(ports.begin(), ports.end(), std::back_inserter(info.ports)); + std::sort(info.ports.begin(), info.ports.end()); + // We may have that analyzer already iff it was previously pre-registered // without a linker scope. We'll then only set the scope now. if ( auto t = _analyzer_name_to_tag_type.find(info.name_zeek); t != _analyzer_name_to_tag_type.end() ) { @@ -701,14 +707,25 @@ void Manager::InitPostScript() { if ( ! tag ) reporter->InternalError("cannot get analyzer tag for '%s'", p.name_analyzer.c_str()); + auto register_analyzer_for_port = [&](auto tag, const hilti::rt::Port& port_) { + SPICY_DEBUG(hilti::rt::fmt(" Scheduling analyzer for port %s", port_)); + + // Well-known ports are registered in scriptland, so we'll raise an + // event that will do it for us through a predefined handler. + zeek::Args vals = Args(); + vals.emplace_back(tag.AsVal()); + vals.emplace_back(zeek::spicy::rt::to_val(port_, base_type(TYPE_PORT))); + EventHandlerPtr handler = event_registry->Register("spicy_analyzer_for_port"); + event_mgr.Enqueue(handler, vals); + }; + for ( const auto& ports : p.ports ) { const auto proto = ports.begin.protocol(); // Port ranges are closed intervals. for ( auto port = ports.begin.port(); port <= ports.end.port(); ++port ) { const auto port_ = hilti::rt::Port(port, proto); - SPICY_DEBUG(hilti::rt::fmt(" Scheduling analyzer for port %s", port_)); - analyzer_mgr->RegisterAnalyzerForPort(tag, transport_protocol(port_), port); + register_analyzer_for_port(tag, port_); // Don't double register in case of single-port ranges. if ( ports.begin.port() == ports.end.port() ) @@ -727,7 +744,7 @@ void Manager::InitPostScript() { continue; SPICY_DEBUG(hilti::rt::fmt(" Scheduling analyzer for port %s", port.port)); - analyzer_mgr->RegisterAnalyzerForPort(tag, transport_protocol(port.port), port.port.port()); + register_analyzer_for_port(tag, port.port); } } } diff --git a/src/spicy/manager.h b/src/spicy/manager.h index 118e03b6c3..55f47c51fd 100644 --- a/src/spicy/manager.h +++ b/src/spicy/manager.h @@ -85,7 +85,7 @@ public: * * @param name name of the analyzer as defined in its EVT file * @param proto analyzer's transport-layer protocol - * @param prts well-known ports for the analyzer; it'll be activated automatically for these + * @param ports well-known ports for the analyzer; it'll be activated automatically for these * @param parser_orig name of the Spicy parser for the originator side; must match the name that * Spicy registers the unit's parser with * @param parser_resp name of the Spicy parser for the originator side; must match the name that @@ -343,7 +343,7 @@ private: std::string name_parser_resp; std::string name_replaces; hilti::rt::Protocol protocol = hilti::rt::Protocol::Undef; - hilti::rt::Vector<::zeek::spicy::rt::PortRange> ports; + std::vector<::zeek::spicy::rt::PortRange> ports; // we keep this sorted std::string linker_scope; // Computed and available once the analyzer has been registered. diff --git a/src/spicy/port-range.h b/src/spicy/port-range.h index bbe0d58c12..7e71d433f8 100644 --- a/src/spicy/port-range.h +++ b/src/spicy/port-range.h @@ -19,6 +19,11 @@ struct PortRange { hilti::rt::Port begin; /**< first port in the range */ hilti::rt::Port end; /**< last port in the range */ + + bool operator<(const PortRange& other) const { + // Just get us a deterministic order. + return std::tie(begin, end) < std::tie(other.begin, other.end); + } }; inline bool operator==(const PortRange& a, const PortRange& b) { diff --git a/src/spicy/spicyz/glue-compiler.cc b/src/spicy/spicyz/glue-compiler.cc index e9240ed245..2b75a90949 100644 --- a/src/spicy/spicyz/glue-compiler.cc +++ b/src/spicy/spicyz/glue-compiler.cc @@ -739,14 +739,6 @@ glue::ProtocolAnalyzer GlueCompiler::parseProtocolAnalyzer(const std::string& ch eat_token(chunk, &i, ","); } - if ( ! a.ports.empty() ) - hilti::logger().warning( - hilti::rt:: - fmt("Remove in v7.1: Analyzer %s is using the deprecated 'port' or 'ports' keyword to register " - "well-known ports. Use Analyzer::register_for_ports() in the accompanying Zeek script instead.", - a.name), - a.location); - return a; } diff --git a/testing/btest/Baseline/spicy.port-deprecated/out.stderr b/testing/btest/Baseline/spicy.port-deprecated/out.stderr deleted file mode 100644 index a033682601..0000000000 --- a/testing/btest/Baseline/spicy.port-deprecated/out.stderr +++ /dev/null @@ -1,2 +0,0 @@ -### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. -[warning] <...>/udp-test.evt:4: Remove in v7.1: Analyzer spicy::TEST is using the deprecated 'port' or 'ports' keyword to register well-known ports. Use Analyzer::register_for_ports() in the accompanying Zeek script instead. diff --git a/testing/btest/Baseline/spicy.port-fail/output b/testing/btest/Baseline/spicy.port-fail/output index f572d2e79a..24eb09807d 100644 --- a/testing/btest/Baseline/spicy.port-fail/output +++ b/testing/btest/Baseline/spicy.port-fail/output @@ -1,3 +1,3 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. -[error] <...>/port-fail.evt:9: port outside of valid range +[error] <...>/port-fail.evt:7: port outside of valid range [error] error loading EVT file "<...>/port-fail.evt" diff --git a/testing/btest/Baseline/spicy.port/output b/testing/btest/Baseline/spicy.port/output new file mode 100644 index 0000000000..938a6c7b35 --- /dev/null +++ b/testing/btest/Baseline/spicy.port/output @@ -0,0 +1,19 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +Analyzer::ANALYZER_SPICY_TEST, 11337/udp +Analyzer::ANALYZER_SPICY_TEST, 11338/udp +Analyzer::ANALYZER_SPICY_TEST, 11339/udp +Analyzer::ANALYZER_SPICY_TEST, 11340/udp +Analyzer::ANALYZER_SPICY_TEST, 31337/udp +Analyzer::ANALYZER_SPICY_TEST, 31338/udp +Analyzer::ANALYZER_SPICY_TEST, 31339/udp +Analyzer::ANALYZER_SPICY_TEST, 31340/udp +{ +31339/udp, +31337/udp, +31338/udp, +11339/udp, +11338/udp, +11340/udp, +31340/udp, +11337/udp +} diff --git a/testing/btest/spicy/port-deprecated.evt b/testing/btest/spicy/port-deprecated.evt deleted file mode 100644 index 220a9d1faf..0000000000 --- a/testing/btest/spicy/port-deprecated.evt +++ /dev/null @@ -1,21 +0,0 @@ -# @TEST-REQUIRES: have-spicy -# -# @TEST-EXEC: spicyz -d -o test.hlto ./udp-test.evt 2>out.stderr -# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff out.stderr -# -# @TEST-DOC: Remove with v7.1: Specifying ports is deprecated. - -module Test; - -import zeek; - -public type Message = unit { - data: bytes &eod {} -}; - -# @TEST-START-FILE udp-test.evt -protocol analyzer spicy::TEST over UDP: - parse with Test::Message, - port 11337/udp-11340/udp, - ports {31337/udp-31340/udp}; -# @TEST-END-FILE diff --git a/testing/btest/spicy/port-fail.evt b/testing/btest/spicy/port-fail.evt index e51ca0fb79..f00efc6210 100644 --- a/testing/btest/spicy/port-fail.evt +++ b/testing/btest/spicy/port-fail.evt @@ -2,8 +2,6 @@ # # @TEST-EXEC-FAIL: spicyz %INPUT -d -o x.hlto >output 2>&1 # @TEST-EXEC: TEST_DIFF_CANONIFIER=diff-canonifier-spicy btest-diff output -# -# @TEST-DOC: Remove with v7.1 protocol analyzer spicy::SSH over TCP: port 123456/udp; diff --git a/testing/btest/spicy/port-range-one-port.zeek b/testing/btest/spicy/port-range-one-port.zeek index 95c32f2b27..bdc5219791 100644 --- a/testing/btest/spicy/port-range-one-port.zeek +++ b/testing/btest/spicy/port-range-one-port.zeek @@ -5,7 +5,7 @@ # @TEST-EXEC: grep -e 'Scheduling analyzer' -e 'error during parsing' < out > out.filtered # @TEST-EXEC: btest-diff out.filtered -# @TEST-DOC: Remove with v7.1. Expect a single 'Scheduling analyzer ...' message in the debug output and no parsing errors. There was a bug that 'port 31336/udp' would be wrongly interpreted as a 31336/udp-31337/udp port range. Regression test for #3278. +# @TEST-DOC: Expect a single 'Scheduling analyzer ...' message in the debug output and no parsing errors. There was a bug that 'port 31336/udp' would be wrongly interpreted as a 31336/udp-31337/udp port range. Regression test for #3278. # @TEST-START-FILE udp-test.spicy module UDPTest; diff --git a/testing/btest/spicy/port.zeek b/testing/btest/spicy/port.zeek new file mode 100644 index 0000000000..81d3586c68 --- /dev/null +++ b/testing/btest/spicy/port.zeek @@ -0,0 +1,32 @@ +# @TEST-REQUIRES: have-spicy +# +# @TEST-EXEC: spicyz -d -o test.hlto test.spicy test.evt +# @TEST-EXEC: zeek test.hlto %INPUT >output +# @TEST-EXEC: btest-diff output +# +# @TEST-DOC: Check that we raise port events for Spicy analyzers, and that the ports get correctly registered. + +event spicy_analyzer_for_port(a: Analyzer::Tag, p: port){ + print a, p; +} + +event zeek_done() { + print Analyzer::ports[Analyzer::ANALYZER_SPICY_TEST]; +} + +# @TEST-START-FILE test.spicy +module Test; + +import zeek; + +public type Message = unit { + data: bytes &eod {} +}; +# @TEST-END-FILE + +# @TEST-START-FILE test.evt +protocol analyzer spicy::Test over UDP: + parse with Test::Message, + port 11337/udp-11340/udp, + ports {31337/udp-31340/udp}; +# @TEST-END-FILE From 4576741fe48a803ce31b289038698a8ccd71c240 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Thu, 22 Aug 2024 13:05:59 +0200 Subject: [PATCH 5/8] TCP_Reassembler: Fix IsOrig() position in Match() call Found during a debug session with @rsmmr. Undelivered TCP data would only be matched for the responder and eol set to IsOrig(). --- src/analyzer/protocol/tcp/TCP_Reassembler.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/analyzer/protocol/tcp/TCP_Reassembler.cc b/src/analyzer/protocol/tcp/TCP_Reassembler.cc index e9c2a1f2f6..d6c74edcf8 100644 --- a/src/analyzer/protocol/tcp/TCP_Reassembler.cc +++ b/src/analyzer/protocol/tcp/TCP_Reassembler.cc @@ -273,7 +273,10 @@ void TCP_Reassembler::MatchUndelivered(uint64_t up_to_seq, bool use_last_upper) if ( b.upper > last_reassem_seq ) break; - tcp_analyzer->Conn()->Match(zeek::detail::Rule::PAYLOAD, b.block, b.Size(), false, false, IsOrig(), false); + // Note: Even though this passes bol=false, at the point where + // this code runs, the matcher is re-initialized resulting in + // undelivered data implicitly being bol-anchored. + tcp_analyzer->Conn()->Match(zeek::detail::Rule::PAYLOAD, b.block, b.Size(), IsOrig(), false, false, false); } } From a6edbf8bcdfb326033056310369cfe41630a7145 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Thu, 22 Aug 2024 13:14:24 +0100 Subject: [PATCH 6/8] Fix parsing of version field in SSLv2 client hello It turns out that, for probably a long time, we have reported an incorrect version when parsing an SSLv2 client hello. We always reported this as SSLv2, no matter which version the client hello actually contained. This bug probably went unnoticed for a long time, as SSLv2 is essentially unused nowadays, and as this field does not show up in the default logs. This was found due to a baseline difference when writing the Spicy SSL analyzer. --- scripts/base/protocols/ssl/main.zeek | 8 +++++--- src/analyzer/protocol/ssl/ssl-protocol.pac | 12 ++++++------ .../Baseline/scripts.base.protocols.ssl.dpd/.stdout | 2 +- .../ssl-all.log | 2 +- testing/external/commit-hash.zeek-testing | 2 +- testing/external/commit-hash.zeek-testing-private | 2 +- 6 files changed, 15 insertions(+), 13 deletions(-) diff --git a/scripts/base/protocols/ssl/main.zeek b/scripts/base/protocols/ssl/main.zeek index f3c3f21c73..0a0c8d6f4a 100644 --- a/scripts/base/protocols/ssl/main.zeek +++ b/scripts/base/protocols/ssl/main.zeek @@ -283,9 +283,11 @@ event ssl_client_hello(c: connection, version: count, record_version: count, pos c$ssl$client_ticket_empty_session_seen = F; } - # add manually for SSLv2, since the handshake_message event is not raised, as there is no handshake protocol. - # We don't really have a direction in that case - if ( version == 2 ) + # add manually for SSLv2 client hello, since the handshake_message event is not raised, as there is no handshake protocol. + # We don't really have a direction in that case. + # SSLv2 client hello is signified by a record_layer version of 0, as the client-hello itself can indicate + # a higher supported maximum version + if ( record_version == 0 ) add_to_history(c, T, "c"); } diff --git a/src/analyzer/protocol/ssl/ssl-protocol.pac b/src/analyzer/protocol/ssl/ssl-protocol.pac index 5914bcaeda..9e1ea3b95c 100644 --- a/src/analyzer/protocol/ssl/ssl-protocol.pac +++ b/src/analyzer/protocol/ssl/ssl-protocol.pac @@ -8,24 +8,24 @@ type SSLRecord(is_orig: bool) = record { head2 : uint8; head3 : uint8; head4 : uint8; - rec : RecordText(this)[] &length=length, &requires(version,content_type,raw_tls_version); + rec : RecordText(this)[] &length=length, &requires(record_layer_version,content_type,raw_tls_version); } &length = length+5, &byteorder=bigendian, &let { - version : int = + record_layer_version : int = $context.connection.determine_ssl_record_layer(head0, head1, head2, head3, head4, is_orig); # unmodified tls record layer version of this packet. Do not use this if you are parsing SSLv2 - raw_tls_version: uint16 = case version of { + raw_tls_version: uint16 = case record_layer_version of { SSLv20 -> 0; default -> (head1<<8) | head2; } &requires(version); - content_type : int = case version of { + content_type : int = case record_layer_version of { SSLv20 -> head2+300; default -> head0; } &requires(version); - length : int = case version of { + length : int = case record_layer_version of { # fail analyzer if the packet cannot be recognized as TLS. UNKNOWN_VERSION -> 0; SSLv20 -> (((head0 & 0x7f) << 8) | head1) - 3; @@ -77,7 +77,7 @@ type V2ClientHello(rec: SSLRecord) = record { session_id : uint8[session_len]; challenge : bytestring &length = chal_len; } &length = 6 + csuit_len + session_len + chal_len, &let { - client_version : int = rec.version; + client_version : int = (rec.head3 << 8) | rec.head4; }; diff --git a/testing/btest/Baseline/scripts.base.protocols.ssl.dpd/.stdout b/testing/btest/Baseline/scripts.base.protocols.ssl.dpd/.stdout index 2e1e5a2282..76b1b068f3 100644 --- a/testing/btest/Baseline/scripts.base.protocols.ssl.dpd/.stdout +++ b/testing/btest/Baseline/scripts.base.protocols.ssl.dpd/.stdout @@ -2,7 +2,7 @@ Start test run Client hello, 192.168.4.149, 91.227.4.92, 2 Start test run -Client hello, 192.150.187.164, 194.127.84.106, 2 +Client hello, 192.150.187.164, 194.127.84.106, 769 Client hello, 192.150.187.164, 194.127.84.106, 769 Client hello, 192.150.187.164, 194.127.84.106, 769 Start test run diff --git a/testing/btest/Baseline/scripts.policy.protocols.ssl.ssl-log-ext/ssl-all.log b/testing/btest/Baseline/scripts.policy.protocols.ssl.ssl-log-ext/ssl-all.log index fc8d05ef2b..679d59b2fd 100644 --- a/testing/btest/Baseline/scripts.policy.protocols.ssl.ssl-log-ext/ssl-all.log +++ b/testing/btest/Baseline/scripts.policy.protocols.ssl.ssl-log-ext/ssl-all.log @@ -27,7 +27,7 @@ XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.18.50 56981 74.125.239.97 443 TLSv12 #open XXXX-XX-XX-XX-XX-XX #fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p version cipher curve server_name resumed last_alert next_protocol established ssl_history cert_chain_fps client_cert_chain_fps sni_matches_cert server_version client_version client_ciphers ssl_client_exts ssl_server_exts ticket_lifetime_hint dh_param_size point_formats client_curves orig_alpn client_supported_versions server_supported_version psk_key_exchange_modes client_key_share_groups server_key_share_group client_comp_methods sigalgs hashalgs #types time string addr port addr port string string string string bool string string bool string vector[string] vector[string] bool count count vector[count] vector[count] vector[count] count count vector[count] vector[count] vector[string] vector[count] count vector[count] vector[count] count vector[count] vector[count] vector[count] -XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.150.187.164 58868 194.127.84.106 443 TLSv10 TLS_RSA_WITH_RC4_128_MD5 - - F - - T CsxnGIi ddd0218a34972ceab3d200b78959bd2b4c95eadf37399df35bfd68a5b658bc78,ba352de8d8faa0ecfdbeee560fa308fe192023d3b18d83a68845933bebf28360 (empty) - 769 2 57,56,53,51,50,4,5,47,22,19,65279,10,21,18,65278,9,100,98,3,6 - - - - - - - - - - - - (empty) - - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.150.187.164 58868 194.127.84.106 443 TLSv10 TLS_RSA_WITH_RC4_128_MD5 - - F - - T CsxnGIi ddd0218a34972ceab3d200b78959bd2b4c95eadf37399df35bfd68a5b658bc78,ba352de8d8faa0ecfdbeee560fa308fe192023d3b18d83a68845933bebf28360 (empty) - 769 769 57,56,53,51,50,4,5,47,22,19,65279,10,21,18,65278,9,100,98,3,6 - - - - - - - - - - - - (empty) - - XXXXXXXXXX.XXXXXX ClEkJM2Vm5giqnMf4h 192.150.187.164 58869 194.127.84.106 443 TLSv10 TLS_RSA_WITH_RC4_128_MD5 - - F - - T CsxnGIi ddd0218a34972ceab3d200b78959bd2b4c95eadf37399df35bfd68a5b658bc78,ba352de8d8faa0ecfdbeee560fa308fe192023d3b18d83a68845933bebf28360 (empty) - 769 769 57,56,53,51,50,4,5,47,22,19,65279,10,21,18,65278,9,100,98,3,6 - - - - - - - - - - - - 0 - - XXXXXXXXXX.XXXXXX C4J4Th3PJpwUYZZ6gc 192.150.187.164 58870 194.127.84.106 443 TLSv10 TLS_RSA_WITH_RC4_128_MD5 - - F - - T CsxnGIi ddd0218a34972ceab3d200b78959bd2b4c95eadf37399df35bfd68a5b658bc78,ba352de8d8faa0ecfdbeee560fa308fe192023d3b18d83a68845933bebf28360 (empty) - 769 769 57,56,53,51,50,4,5,47,22,19,65279,10,21,18,65278,9,100,98,3,6 - - - - - - - - - - - - 0 - - #close XXXX-XX-XX-XX-XX-XX diff --git a/testing/external/commit-hash.zeek-testing b/testing/external/commit-hash.zeek-testing index c73f410174..e535afe051 100644 --- a/testing/external/commit-hash.zeek-testing +++ b/testing/external/commit-hash.zeek-testing @@ -1 +1 @@ -a1c74b74b8755dc4030dfd6034b2bdce23a07072 +df37cbcef57db5aeb09da8045deed0141d471507 diff --git a/testing/external/commit-hash.zeek-testing-private b/testing/external/commit-hash.zeek-testing-private index 005b970c2e..d7ac371590 100644 --- a/testing/external/commit-hash.zeek-testing-private +++ b/testing/external/commit-hash.zeek-testing-private @@ -1 +1 @@ -2a6b523ed423a550cc897cb2cb6a2d6e0cdaea22 +4e30d6b89edf12d99e1165fd5c5e193d1320e371 From be9f170561e1bf10fae0bad2c0c322e1c61fbbef Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Wed, 21 Aug 2024 23:58:41 +0200 Subject: [PATCH 7/8] Analyzer: Do not add child analyzers when finished Depending on an analyzer's implementation, its Done() method may attempt to access analyzer or connection state when executing. When this happens in the destructor of the parent analyzer during the process of destructing a connection, this state may have been deleted, resulting in use-after-free crashes or worse memory corruption. The following cases have been observed in the wild for when this happens. * PIA matching during Done() for undelivered TCP data enables a Spicy based analyzer which in turn attempts to raise an analyzer violation during Done()->EndOfData(). * Spicy analyzers attaching new analyzers during their Done() processing which in turn attempt to use TCP() (to call FindChild()) during Done() while the analyzer tree / connection is being destructed. The second scenario was previously found to happen in the HTTP analyzer and fixed with 6ef9423f3cff13e6c73f97eb6a3a27d6f64cc320. Plug these scenarios by short-circuiting AddChildAnalyzer() if the analyzer or connection have finished or are being finished. --- src/Conn.h | 3 +++ src/analyzer/Analyzer.cc | 38 +++++++++++++++++++++++++------------- 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/src/Conn.h b/src/Conn.h index 67f41dc68d..c1d16f54bd 100644 --- a/src/Conn.h +++ b/src/Conn.h @@ -195,6 +195,9 @@ public: bool PermitWeird(const char* name, uint64_t threshold, uint64_t rate, double duration); + // Returns true once Done() is called. + bool IsFinished() { return finished; } + private: friend class session::detail::Timer; diff --git a/src/analyzer/Analyzer.cc b/src/analyzer/Analyzer.cc index e8f2b65d28..5db65b44a2 100644 --- a/src/analyzer/Analyzer.cc +++ b/src/analyzer/Analyzer.cc @@ -113,19 +113,7 @@ void Analyzer::CtorInit(const zeek::Tag& arg_tag, Connection* arg_conn) { Analyzer::~Analyzer() { assert(finished); - - // Make sure any late entries into the analyzer tree are handled (e.g. - // from some Done() implementation). - LOOP_OVER_GIVEN_CHILDREN(i, new_children) { - if ( ! (*i)->finished ) - (*i)->Done(); - } - - // Deletion of new_children done in separate loop in case a Done() - // implementation tries to inspect analyzer tree w/ assumption that - // all analyzers are still valid. - LOOP_OVER_GIVEN_CHILDREN(i, new_children) - delete *i; + assert(new_children.empty()); LOOP_OVER_CHILDREN(i) delete *i; @@ -330,6 +318,30 @@ void Analyzer::ForwardEndOfData(bool orig) { bool Analyzer::AddChildAnalyzer(Analyzer* analyzer, bool init) { auto t = analyzer->GetAnalyzerTag(); + // Prevent attaching child analyzers to analyzer subtrees where + // either the parent has finished or is being removed. Further, + // don't attach analyzers when the connection has finished or is + // currently being finished (executing Done()). + // + // Scenarios in which analyzers have been observed that late in + // analyzer / connection lifetime are: + // + // * A DPD signature match on undelivered TCP data that is flushed + // during Connection::Done(). The PIA analyzer activates a new + // analyzer adding it to the TCP analyzer. + // + // * Analyzers flushing buffered state during Done(), resulting + // in new analyzers being created. + // + // Analyzers added during Done() are problematic as calling Done() + // within the parent's destructor isn't safe, so we prevent these + // situations. + if ( Removing() || IsFinished() || Conn()->IsFinished() ) { + analyzer->Done(); + delete analyzer; + return false; + } + if ( HasChildAnalyzer(t) || IsPreventedChildAnalyzer(t) ) { analyzer->Done(); delete analyzer; From 617faa1d33a5f7e67fd12ad42ceec9f5b2dac5c6 Mon Sep 17 00:00:00 2001 From: zeek-bot Date: Sat, 24 Aug 2024 00:20:43 +0000 Subject: [PATCH 8/8] Update doc submodule [nomail] [skip ci] --- doc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc b/doc index 1ca467fe0c..fc15fea160 160000 --- a/doc +++ b/doc @@ -1 +1 @@ -Subproject commit 1ca467fe0cb524ff375d957475d0319ab546915b +Subproject commit fc15fea160a40c88ca9868a21203097b3a2b9b71