From be061bfe0449f3dfb3fea76af3a01b5428e24b33 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Fri, 22 Aug 2025 14:05:44 +0200 Subject: [PATCH] IPBasedAnalyzer: Ensure a connection's ConnVal is updated One idea for the issue that a connection record's endpoint fields may be stale when events are raised before the ConnSize analyzer saw the packet and no analyzer calls GetVal() for the connection afterwards. While this looks a bit ad hoc, I'm leaning towards a follow-up, extending GetVal(skip_update_connval=false) API and then updating BinPAC/Spicy to generate code that passes ``false``. That could avoid a number of unnecessary UpateConnVal() invocations when calling GetVal() multiple times for the same connection and instead just do it once when the packet has been processed. Closes #4214 --- .../protocol/ip/IPBasedAnalyzer.cc | 20 +++++++++ .../core.conn-size-endpoint-update/out | 37 ++++++++++++++++ testing/btest/Traces/tcp/synack.pcap | Bin 0 -> 114 bytes .../btest/core/conn-size-endpoint-update.zeek | 41 ++++++++++++++++++ 4 files changed, 98 insertions(+) create mode 100644 testing/btest/Baseline/core.conn-size-endpoint-update/out create mode 100644 testing/btest/Traces/tcp/synack.pcap create mode 100644 testing/btest/core/conn-size-endpoint-update.zeek diff --git a/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc b/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc index 8980f5a7de..4adc27d5cf 100644 --- a/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc +++ b/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc @@ -3,6 +3,7 @@ #include "zeek/packet_analysis/protocol/ip/IPBasedAnalyzer.h" #include "zeek/Conn.h" +#include "zeek/Event.h" #include "zeek/RunState.h" #include "zeek/Val.h" #include "zeek/analyzer/Manager.h" @@ -41,6 +42,7 @@ bool IPBasedAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pkt key->Init(*pkt); + const auto queued_events_before = zeek::event_mgr.Size(); // Used for determining if any events were raised. const std::shared_ptr& ip_hdr = pkt->ip_hdr; auto src_addr = key->SrcAddr(); auto src_port = key->SrcPort(); @@ -108,6 +110,24 @@ bool IPBasedAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pkt run_state::current_timestamp = 0; run_state::current_pkt = nullptr; + // If any events were queued while processing the packet, call conn->GetVal() + // once more here to trigger a recursive UpdateConnVal() call on the analyzer tree. + // This is primarily for the ConnSize analyzer that implements UpdateConnVal(), + // but does usually not raise any events and therefore does not call GetVal(). + // The ConnSize data is exposed on the script-level connection record through the + // orig and resp endpoint fields, however. This call ensures that these fields + // aren't stale events were only raised *before* the ConnSize analyzer. + // + // This is a bit unfortunate, as the GetVal() call will result in an extra + // GetVal() call that might not be needed. Maybe we could extend GetVal() to + // skip UpdateConnVal() when called from BinPac or Spicy going forward so that + // it is just called once after processing a packet. @awelzel hasn't seen + // UpdateConnVal() prominently in flame graphs though, so skipping this for now + // as it's not clear it'd actually be worth it. + const auto queued_events_after = zeek::event_mgr.Size(); + if ( queued_events_after > queued_events_before ) + (void)conn->GetVal(); + // If the packet is reassembled, disable packet dumping because the // pointer math to dump the data wouldn't work. if ( pkt->ip_hdr->Reassembled() ) diff --git a/testing/btest/Baseline/core.conn-size-endpoint-update/out b/testing/btest/Baseline/core.conn-size-endpoint-update/out new file mode 100644 index 0000000000..64cd65346d --- /dev/null +++ b/testing/btest/Baseline/core.conn-size-endpoint-update/out @@ -0,0 +1,37 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +==== zeek_init, syn.pcap +new_connection, CHhAvVGS1DHFjwGM9 + orig, [size=0, state=1, num_pkts=1, num_bytes_ip=64, flow_label=0, l2_addr=c8:bc:c8:96:d2:a0] + resp, [size=0, state=0, num_pkts=0, num_bytes_ip=0, flow_label=0, l2_addr=00:10:db:88:d2:ef] +connection_SYN_packet, CHhAvVGS1DHFjwGM9, orig + orig, [size=0, state=1, num_pkts=1, num_bytes_ip=64, flow_label=0, l2_addr=c8:bc:c8:96:d2:a0] + resp, [size=0, state=0, num_pkts=0, num_bytes_ip=0, flow_label=0, l2_addr=00:10:db:88:d2:ef] +connection_state_remove, CHhAvVGS1DHFjwGM9 + orig, [size=0, state=1, num_pkts=1, num_bytes_ip=64, flow_label=0, l2_addr=c8:bc:c8:96:d2:a0] + resp, [size=0, state=0, num_pkts=0, num_bytes_ip=0, flow_label=0, l2_addr=00:10:db:88:d2:ef] +==== zeek_init, synack.pcap +new_connection, CHhAvVGS1DHFjwGM9 + orig, [size=0, state=2, num_pkts=1, num_bytes_ip=60, flow_label=0, l2_addr=00:10:db:88:d2:ef] + resp, [size=0, state=0, num_pkts=0, num_bytes_ip=0, flow_label=0, l2_addr=c8:bc:c8:96:d2:a0] +connection_SYN_packet, CHhAvVGS1DHFjwGM9, orig + orig, [size=0, state=2, num_pkts=1, num_bytes_ip=60, flow_label=0, l2_addr=00:10:db:88:d2:ef] + resp, [size=0, state=0, num_pkts=0, num_bytes_ip=0, flow_label=0, l2_addr=c8:bc:c8:96:d2:a0] +connection_state_remove, CHhAvVGS1DHFjwGM9 + orig, [size=0, state=2, num_pkts=1, num_bytes_ip=60, flow_label=0, l2_addr=00:10:db:88:d2:ef] + resp, [size=0, state=0, num_pkts=0, num_bytes_ip=0, flow_label=0, l2_addr=c8:bc:c8:96:d2:a0] +==== zeek_init, get.trace +new_connection, CHhAvVGS1DHFjwGM9 + orig, [size=0, state=1, num_pkts=1, num_bytes_ip=64, flow_label=0, l2_addr=c8:bc:c8:96:d2:a0] + resp, [size=0, state=0, num_pkts=0, num_bytes_ip=0, flow_label=0, l2_addr=00:10:db:88:d2:ef] +connection_SYN_packet, CHhAvVGS1DHFjwGM9, orig + orig, [size=0, state=1, num_pkts=1, num_bytes_ip=64, flow_label=0, l2_addr=c8:bc:c8:96:d2:a0] + resp, [size=0, state=0, num_pkts=0, num_bytes_ip=0, flow_label=0, l2_addr=00:10:db:88:d2:ef] +connection_SYN_packet, CHhAvVGS1DHFjwGM9, resp + orig, [size=0, state=4, num_pkts=1, num_bytes_ip=64, flow_label=0, l2_addr=c8:bc:c8:96:d2:a0] + resp, [size=0, state=4, num_pkts=1, num_bytes_ip=60, flow_label=0, l2_addr=00:10:db:88:d2:ef] +connection_established, CHhAvVGS1DHFjwGM9 + orig, [size=0, state=4, num_pkts=1, num_bytes_ip=64, flow_label=0, l2_addr=c8:bc:c8:96:d2:a0] + resp, [size=0, state=4, num_pkts=1, num_bytes_ip=60, flow_label=0, l2_addr=00:10:db:88:d2:ef] +connection_state_remove, CHhAvVGS1DHFjwGM9 + orig, [size=136, state=5, num_pkts=7, num_bytes_ip=512, flow_label=0, l2_addr=c8:bc:c8:96:d2:a0] + resp, [size=5007, state=5, num_pkts=7, num_bytes_ip=5379, flow_label=0, l2_addr=00:10:db:88:d2:ef] diff --git a/testing/btest/Traces/tcp/synack.pcap b/testing/btest/Traces/tcp/synack.pcap new file mode 100644 index 0000000000000000000000000000000000000000..56689cccd23de78acdc0db951a0063714c9884f6 GIT binary patch literal 114 zcmca|c+)~A1{MYw`2U}Qff2}A$7vbp6T-*f1!RNpi9IK#U0T2(aJ%EudkzLy1_m1j z1_uUxwz#wd({^k3_B~-`2zYs6>H2ea{}hd^7YJEQ&> out +# @TEST-EXEC: zeek -b -r $TRACES/tcp/synack.pcap %INPUT >> out +# @TEST-EXEC: zeek -b -r $TRACES/http/get.trace %INPUT >> out +# +# @TEST-EXEC: btest-diff out + +event zeek_init() + { + print "==== zeek_init", split_string(packet_source()$path, /\//)[-1]; + } + +event new_connection(c: connection) + { + print "new_connection", c$uid; + print " orig", c$orig; + print " resp", c$resp; + } + +event connection_SYN_packet(c: connection, pkt: SYN_packet) + { + print "connection_SYN_packet", c$uid, pkt$is_orig ? "orig" : "resp"; + print " orig", c$orig; + print " resp", c$resp; + } + +event connection_established(c: connection) + { + print "connection_established", c$uid; + print " orig", c$orig; + print " resp", c$resp; + } + +event connection_state_remove(c: connection) + { + print "connection_state_remove", c$uid; + print " orig", c$orig; + print " resp", c$resp; + }