From 8192ad581d2425c270b39ebe034a88a5490a5bad Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Fri, 6 Aug 2021 10:32:53 +0100 Subject: [PATCH] Do not lookup ignore_checksums_nets for every packet This could lead to a noticeable (single-percent) performance improvement. Most of the functionality for this is in the packet analyzers that now cache ignore_chesksums_nets. Based on a patch by Arne Welzel (Corelight). --- scripts/base/init-bare.zeek | 7 +++++++ scripts/base/packet-protocols/ip/main.zeek | 10 +++++++++ src/packet_analysis/packet_analysis.bif | 11 ++++++++++ src/packet_analysis/protocol/icmp/ICMP.cc | 2 +- src/packet_analysis/protocol/ip/IP.cc | 3 ++- .../protocol/ip/IPBasedAnalyzer.cc | 15 +++++++++++++ .../protocol/ip/IPBasedAnalyzer.h | 21 +++++++++++++++++++ src/packet_analysis/protocol/tcp/TCP.cc | 3 +-- src/packet_analysis/protocol/tcp/TCP.h | 2 -- src/packet_analysis/protocol/udp/UDP.cc | 2 +- .../weird.log | 11 ++++++++++ testing/btest/Baseline/plugins.hooks/output | 3 +++ .../checksums_ignore_nets_runtime_update.test | 18 ++++++++++++++++ 13 files changed, 101 insertions(+), 7 deletions(-) create mode 100644 testing/btest/Baseline/core.checksums_ignore_nets_runtime_update/weird.log create mode 100644 testing/btest/core/checksums_ignore_nets_runtime_update.test diff --git a/scripts/base/init-bare.zeek b/scripts/base/init-bare.zeek index ea70767f72..8f40fe4551 100644 --- a/scripts/base/init-bare.zeek +++ b/scripts/base/init-bare.zeek @@ -25,6 +25,13 @@ type string_any_table: table[string] of any; ## directly and then remove this alias. type string_set: set[string]; +## A set of subnets. +## +## .. todo:: We need this type definition only for declaring builtin functions +## via ``bifcl``. We should extend ``bifcl`` to understand composite types +## directly and then remove this alias. +type subnet_set: set[subnet]; + ## A set of addresses. ## ## .. todo:: We need this type definition only for declaring builtin functions diff --git a/scripts/base/packet-protocols/ip/main.zeek b/scripts/base/packet-protocols/ip/main.zeek index 0e1c506bcb..145ec639b9 100644 --- a/scripts/base/packet-protocols/ip/main.zeek +++ b/scripts/base/packet-protocols/ip/main.zeek @@ -9,6 +9,14 @@ const IPPROTO_IPIP : count = 4; const IPPROTO_IPV6 : count = 41; const IPPROTO_GRE : count = 47; +function analyzer_option_change_ignore_checksums_nets(ID: string, new_value: set[subnet], location: string) : set[subnet] + { + if ( ID == "ignore_checksums_nets" ) + PacketAnalyzer::__set_ignore_checksums_nets(new_value); + + return new_value; + } + event zeek_init() &priority=20 { PacketAnalyzer::register_packet_analyzer(PacketAnalyzer::ANALYZER_IP, IPPROTO_IPIP, PacketAnalyzer::ANALYZER_IPTUNNEL); @@ -19,4 +27,6 @@ event zeek_init() &priority=20 PacketAnalyzer::register_packet_analyzer(PacketAnalyzer::ANALYZER_IP, IPPROTO_UDP, PacketAnalyzer::ANALYZER_UDP); PacketAnalyzer::register_packet_analyzer(PacketAnalyzer::ANALYZER_IP, IPPROTO_ICMP, PacketAnalyzer::ANALYZER_ICMP); PacketAnalyzer::register_packet_analyzer(PacketAnalyzer::ANALYZER_IP, IPPROTO_ICMP6, PacketAnalyzer::ANALYZER_ICMP); + + Option::set_change_handler("ignore_checksums_nets", analyzer_option_change_ignore_checksums_nets, 5); } diff --git a/src/packet_analysis/packet_analysis.bif b/src/packet_analysis/packet_analysis.bif index cc60808d72..8298b3946f 100644 --- a/src/packet_analysis/packet_analysis.bif +++ b/src/packet_analysis/packet_analysis.bif @@ -4,6 +4,7 @@ module PacketAnalyzer; #include "zeek/packet_analysis/Analyzer.h" #include "zeek/packet_analysis/Manager.h" +#include "zeek/packet_analysis/protocol/ip/IPBasedAnalyzer.h" %%} @@ -47,3 +48,13 @@ function try_register_packet_analyzer_by_name%(parent: string, identifier: count parent_analyzer->RegisterProtocol(identifier, child_analyzer); return zeek::val_mgr->True(); %} + +## Internal function that is used to update the core-mirror of the script-level `ignore_checksums_nets` variable. +function PacketAnalyzer::__set_ignore_checksums_nets%(v: subnet_set%) : bool + %{ + if ( v->GetType()->Tag() != zeek::TYPE_TABLE ) + zeek::emit_builtin_error("update_ignore_checksums_net() requires a table/set argument"); + + zeek::packet_analysis::IP::IPBasedAnalyzer::SetIgnoreChecksumsNets(zeek::IntrusivePtr{zeek::NewRef{}, v->AsTableVal()}); + return zeek::val_mgr->True(); + %} diff --git a/src/packet_analysis/protocol/icmp/ICMP.cc b/src/packet_analysis/protocol/icmp/ICMP.cc index b4ef187cd3..a824d3c1b1 100644 --- a/src/packet_analysis/protocol/icmp/ICMP.cc +++ b/src/packet_analysis/protocol/icmp/ICMP.cc @@ -77,7 +77,7 @@ void ICMPAnalyzer::DeliverPacket(Connection* c, double t, bool is_orig, int rema const std::unique_ptr& ip = pkt->ip_hdr; if ( ! zeek::detail::ignore_checksums && - ! zeek::id::find_val("ignore_checksums_nets")->Contains(ip->IPHeaderSrcAddr()) && + ! GetIgnoreChecksumsNets()->Contains(ip->IPHeaderSrcAddr()) && remaining >= len ) { int chksum = 0; diff --git a/src/packet_analysis/protocol/ip/IP.cc b/src/packet_analysis/protocol/ip/IP.cc index b957f9fce6..e7f84c2675 100644 --- a/src/packet_analysis/protocol/ip/IP.cc +++ b/src/packet_analysis/protocol/ip/IP.cc @@ -1,6 +1,7 @@ // See the file "COPYING" in the main distribution directory for copyright. #include "zeek/packet_analysis/protocol/ip/IP.h" +#include "zeek/packet_analysis/protocol/ip/IPBasedAnalyzer.h" #include "zeek/NetVar.h" #include "zeek/IP.h" #include "zeek/Discard.h" @@ -128,7 +129,7 @@ bool IPAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* packet) return false; if ( ! packet->l2_checksummed && ! detail::ignore_checksums && ip4 && - ! zeek::id::find_val("ignore_checksums_nets")->Contains(packet->ip_hdr->IPHeaderSrcAddr()) && + ! IPBasedAnalyzer::GetIgnoreChecksumsNets()->Contains(packet->ip_hdr->IPHeaderSrcAddr()) && detail::in_cksum(reinterpret_cast(ip4), ip_hdr_len) != 0xffff ) { Weird("bad_IP_checksum", packet); diff --git a/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc b/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc index 287736d6b1..8940751fe5 100644 --- a/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc +++ b/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc @@ -282,3 +282,18 @@ void IPBasedAnalyzer::DumpPortDebug() DBG_LOG(DBG_ANALYZER, " %d/%s: %s", mapping.first, transport_proto_string(transport), s.c_str()); } } + +TableValPtr IPBasedAnalyzer::ignore_checksums_nets_table = nullptr; + +void IPBasedAnalyzer::SetIgnoreChecksumsNets(TableValPtr t) + { + IPBasedAnalyzer::ignore_checksums_nets_table = t; + } + +TableValPtr IPBasedAnalyzer::GetIgnoreChecksumsNets() + { + if ( ! IPBasedAnalyzer::ignore_checksums_nets_table ) + IPBasedAnalyzer::ignore_checksums_nets_table = zeek::id::find_val("ignore_checksums_nets"); + return IPBasedAnalyzer::ignore_checksums_nets_table; + } + diff --git a/src/packet_analysis/protocol/ip/IPBasedAnalyzer.h b/src/packet_analysis/protocol/ip/IPBasedAnalyzer.h index 7ff54a4f09..3f62c5c362 100644 --- a/src/packet_analysis/protocol/ip/IPBasedAnalyzer.h +++ b/src/packet_analysis/protocol/ip/IPBasedAnalyzer.h @@ -7,6 +7,7 @@ #include "zeek/packet_analysis/Analyzer.h" #include "zeek/analyzer/Tag.h" +#include "zeek/ID.h" namespace zeek::analyzer::pia { class PIA; } @@ -61,6 +62,25 @@ public: */ void DumpPortDebug(); + /** + * Updates the internal pointer to the script-level variable `ignore_checksums_nets`. + * This is used to prevent repeated (costly) lookup of the script-level variable + * by IP-based analyzers. + * + * @param t New value of ignore_checksums_nets + */ + static void SetIgnoreChecksumsNets(TableValPtr t); + + + /** + * Gets the interpal pointer to the script-level variable `ignore_checksums_nets`. + * This is used to prevent repeated (costly) lookup of the script-level variable + * by IP-based analyzers. + * + * @return Current value of `ignore_checksums_nets`. + */ + static TableValPtr GetIgnoreChecksumsNets(); + protected: /** @@ -178,6 +198,7 @@ private: TransportProto transport; uint32_t server_port_mask; + static TableValPtr ignore_checksums_nets_table; }; } diff --git a/src/packet_analysis/protocol/tcp/TCP.cc b/src/packet_analysis/protocol/tcp/TCP.cc index 00d4f23a70..3a6de91e41 100644 --- a/src/packet_analysis/protocol/tcp/TCP.cc +++ b/src/packet_analysis/protocol/tcp/TCP.cc @@ -18,7 +18,6 @@ TCPAnalyzer::TCPAnalyzer() : IPBasedAnalyzer("TCP", TRANSPORT_TCP, TCP_PORT_MASK void TCPAnalyzer::Initialize() { - ignored_nets = zeek::id::find_val("ignore_checksums_nets"); } SessionAdapter* TCPAnalyzer::MakeSessionAdapter(Connection* conn) @@ -164,7 +163,7 @@ bool TCPAnalyzer::ValidateChecksum(const IP_Hdr* ip, const struct tcphdr* tp, { if ( ! run_state::current_pkt->l3_checksummed && ! detail::ignore_checksums && - ! ignored_nets->Contains(ip->IPHeaderSrcAddr()) && + ! GetIgnoreChecksumsNets()->Contains(ip->IPHeaderSrcAddr()) && caplen >= len && ! endpoint->ValidChecksum(tp, len, ip->IP4_Hdr()) ) { adapter->Weird("bad_TCP_checksum"); diff --git a/src/packet_analysis/protocol/tcp/TCP.h b/src/packet_analysis/protocol/tcp/TCP.h index 0dd8e88a3c..eb013047b6 100644 --- a/src/packet_analysis/protocol/tcp/TCP.h +++ b/src/packet_analysis/protocol/tcp/TCP.h @@ -86,8 +86,6 @@ private: bool ValidateChecksum(const IP_Hdr* ip, const struct tcphdr* tp, analyzer::tcp::TCP_Endpoint* endpoint, int len, int caplen, TCPSessionAdapter* adapter); - - TableValPtr ignored_nets; }; } diff --git a/src/packet_analysis/protocol/udp/UDP.cc b/src/packet_analysis/protocol/udp/UDP.cc index f798229118..4d1e8cb374 100644 --- a/src/packet_analysis/protocol/udp/UDP.cc +++ b/src/packet_analysis/protocol/udp/UDP.cc @@ -108,7 +108,7 @@ void UDPAnalyzer::DeliverPacket(Connection* c, double t, bool is_orig, int remai auto validate_checksum = ! run_state::current_pkt->l3_checksummed && ! zeek::detail::ignore_checksums && - ! zeek::id::find_val("ignore_checksums_nets")->Contains(ip->IPHeaderSrcAddr()) && + ! GetIgnoreChecksumsNets()->Contains(ip->IPHeaderSrcAddr()) && remaining >=len; constexpr auto vxlan_len = 8; diff --git a/testing/btest/Baseline/core.checksums_ignore_nets_runtime_update/weird.log b/testing/btest/Baseline/core.checksums_ignore_nets_runtime_update/weird.log new file mode 100644 index 0000000000..c5a6161424 --- /dev/null +++ b/testing/btest/Baseline/core.checksums_ignore_nets_runtime_update/weird.log @@ -0,0 +1,11 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +#separator \x09 +#set_separator , +#empty_field (empty) +#unset_field - +#path weird +#open XXXX-XX-XX-XX-XX-XX +#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p name addl notice peer source +#types time string addr port addr port string string bool string string +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.1.28 53246 35.221.46.9 80 bad_TCP_checksum - F zeek TCP +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/plugins.hooks/output b/testing/btest/Baseline/plugins.hooks/output index 12e752da97..e16e7c63b6 100644 --- a/testing/btest/Baseline/plugins.hooks/output +++ b/testing/btest/Baseline/plugins.hooks/output @@ -584,6 +584,7 @@ 0.000000 MetaHookPost CallFunction(Option::set_change_handler, , (default_file_bof_buffer_size, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100)) -> 0.000000 MetaHookPost CallFunction(Option::set_change_handler, , (default_file_timeout_interval, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100)) -> 0.000000 MetaHookPost CallFunction(Option::set_change_handler, , (ignore_checksums_nets, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100)) -> +0.000000 MetaHookPost CallFunction(Option::set_change_handler, , (ignore_checksums_nets, PacketAnalyzer::IP::analyzer_option_change_ignore_checksums_nets{ if (ignore_checksums_nets == PacketAnalyzer::IP::ID) PacketAnalyzer::__set_ignore_checksums_nets(PacketAnalyzer::IP::new_value)return (PacketAnalyzer::IP::new_value)}, 5)) -> 0.000000 MetaHookPost CallFunction(Option::set_change_handler, , (udp_content_delivery_ports_use_resp, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100)) -> 0.000000 MetaHookPost CallFunction(Option::set_change_handler, , (udp_content_ports, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100)) -> 0.000000 MetaHookPost CallFunction(PacketAnalyzer::register_packet_analyzer, , (PacketAnalyzer::ANALYZER_ETHERNET, 2048, PacketAnalyzer::ANALYZER_IP)) -> @@ -1635,6 +1636,7 @@ 0.000000 MetaHookPre CallFunction(Option::set_change_handler, , (default_file_bof_buffer_size, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100)) 0.000000 MetaHookPre CallFunction(Option::set_change_handler, , (default_file_timeout_interval, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100)) 0.000000 MetaHookPre CallFunction(Option::set_change_handler, , (ignore_checksums_nets, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100)) +0.000000 MetaHookPre CallFunction(Option::set_change_handler, , (ignore_checksums_nets, PacketAnalyzer::IP::analyzer_option_change_ignore_checksums_nets{ if (ignore_checksums_nets == PacketAnalyzer::IP::ID) PacketAnalyzer::__set_ignore_checksums_nets(PacketAnalyzer::IP::new_value)return (PacketAnalyzer::IP::new_value)}, 5)) 0.000000 MetaHookPre CallFunction(Option::set_change_handler, , (udp_content_delivery_ports_use_resp, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100)) 0.000000 MetaHookPre CallFunction(Option::set_change_handler, , (udp_content_ports, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100)) 0.000000 MetaHookPre CallFunction(PacketAnalyzer::register_packet_analyzer, , (PacketAnalyzer::ANALYZER_ETHERNET, 2048, PacketAnalyzer::ANALYZER_IP)) @@ -2685,6 +2687,7 @@ 0.000000 | HookCallFunction Option::set_change_handler(default_file_bof_buffer_size, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100) 0.000000 | HookCallFunction Option::set_change_handler(default_file_timeout_interval, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100) 0.000000 | HookCallFunction Option::set_change_handler(ignore_checksums_nets, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100) +0.000000 | HookCallFunction Option::set_change_handler(ignore_checksums_nets, PacketAnalyzer::IP::analyzer_option_change_ignore_checksums_nets{ if (ignore_checksums_nets == PacketAnalyzer::IP::ID) PacketAnalyzer::__set_ignore_checksums_nets(PacketAnalyzer::IP::new_value)return (PacketAnalyzer::IP::new_value)}, 5) 0.000000 | HookCallFunction Option::set_change_handler(udp_content_delivery_ports_use_resp, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100) 0.000000 | HookCallFunction Option::set_change_handler(udp_content_ports, Config::config_option_changed{ Config::log = Config::Info($ts=network_time(), $id=Config::ID, $old_value=Config::format_value(lookup_ID(Config::ID)), $new_value=Config::format_value(Config::new_value))if ( != Config::location) Config::log$location = Config::locationLog::write(Config::LOG, to_any_coerceConfig::log)return (Config::new_value)}, -100) 0.000000 | HookCallFunction PacketAnalyzer::register_packet_analyzer(PacketAnalyzer::ANALYZER_ETHERNET, 2048, PacketAnalyzer::ANALYZER_IP) diff --git a/testing/btest/core/checksums_ignore_nets_runtime_update.test b/testing/btest/core/checksums_ignore_nets_runtime_update.test new file mode 100644 index 0000000000..e3476dfee5 --- /dev/null +++ b/testing/btest/core/checksums_ignore_nets_runtime_update.test @@ -0,0 +1,18 @@ +# @TEST-DOC: Use Config::set_value() to clear ignore_checksums_nets after having received a few packets. Expect a bad_TCP_checksum weird.log entry due to the following packets. +# @TEST-EXEC: zeek -b -r $TRACES/chksums/localhost-bad-chksum.pcap "ignore_checksums_nets += {192.168.0.0/16}" %INPUT +# @TEST-EXEC: btest-diff weird.log + +@load base/frameworks/config +@load base/frameworks/notice + +global packet_counter = 0; + +event new_packet(c: connection, p: pkt_hdr) + { + ++packet_counter; + if ( packet_counter > 3 ) + { + local s: set[subnet] = set(); + Config::set_value("ignore_checksums_nets", s); + } + }