From 17836ef7d957cb794c509175df10eb19706cfcc4 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Fri, 17 Jan 2025 16:50:56 +0100 Subject: [PATCH] session/Manager: Emit explicit errors for FindConnection() with proto=65535 We silently broke users constructing conn_id records manually and subsequently using them with lookup_connection() or connection_exists(). This is an attempt to at least report a runtime error about the situation so it doesn't go completely unnoticed. --- src/session/Manager.cc | 12 +++- .../Baseline/bifs.lookup_connection/.stderr | 7 +++ testing/btest/bifs/lookup_connection.zeek | 58 +++++++++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 testing/btest/Baseline/bifs.lookup_connection/.stderr create mode 100644 testing/btest/bifs/lookup_connection.zeek diff --git a/src/session/Manager.cc b/src/session/Manager.cc index 611cdc630e..131805ef85 100644 --- a/src/session/Manager.cc +++ b/src/session/Manager.cc @@ -9,6 +9,8 @@ #include #include "zeek/Conn.h" +#include "zeek/Func.h" +#include "zeek/IP.h" #include "zeek/NetVar.h" #include "zeek/Reporter.h" #include "zeek/RuleMatcher.h" @@ -18,6 +20,7 @@ #include "zeek/packet_analysis/Manager.h" #include "zeek/session/Session.h" #include "zeek/telemetry/Manager.h" +#include "zeek/util.h" zeek::session::Manager* zeek::session_mgr = nullptr; @@ -89,8 +92,15 @@ void Manager::Done() {} Connection* Manager::FindConnection(Val* v) { zeek::detail::ConnKey conn_key(v); - if ( ! conn_key.valid ) + if ( ! conn_key.Valid() ) { + // Produce a loud error for invalid script-layer conn_id records. + const char* extra = ""; + if ( conn_key.transport == UNKNOWN_IP_PROTO ) + extra = ": the proto field has the \"unknown\" 65535 value. Did you forget to set it?"; + + zeek::emit_builtin_error(zeek::util::fmt("invalid connection ID record encountered%s", extra)); return nullptr; + } return FindConnection(conn_key); } diff --git a/testing/btest/Baseline/bifs.lookup_connection/.stderr b/testing/btest/Baseline/bifs.lookup_connection/.stderr new file mode 100644 index 0000000000..0b20847532 --- /dev/null +++ b/testing/btest/Baseline/bifs.lookup_connection/.stderr @@ -0,0 +1,7 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +1362692526.869344 error in <...>/lookup_connection.zeek, line 19: invalid connection ID record encountered: the proto field has the "unknown" 65535 value. Did you forget to set it? (lookup_connection(cid)) +1362692526.869344 error in <...>/lookup_connection.zeek, line 19: connection ID not a known connection (lookup_connection(cid) and [orig_h=141.142.228.5, orig_p=59856/tcp, resp_h=192.150.187.43, resp_p=80/tcp, proto=65535]) +1362692526.869344 error in <...>/lookup_connection.zeek, line 41: invalid connection ID record encountered: the proto field has the "unknown" 65535 value. Did you forget to set it? (connection_exists(my_id)) +1362692526.869344 error in <...>/lookup_connection.zeek, line 51: invalid connection ID record encountered (lookup_connection(my_id)) +1362692526.869344 error in <...>/lookup_connection.zeek, line 51: connection ID not a known connection (lookup_connection(my_id) and [orig_h=, orig_p=, resp_h=, resp_p=, proto=65535]) +1362692526.869344 error in <...>/lookup_connection.zeek, line 57: invalid connection ID record encountered (connection_exists(my_id)) diff --git a/testing/btest/bifs/lookup_connection.zeek b/testing/btest/bifs/lookup_connection.zeek new file mode 100644 index 0000000000..c8c8b72a9f --- /dev/null +++ b/testing/btest/bifs/lookup_connection.zeek @@ -0,0 +1,58 @@ +# @TEST-DOC: Test lookup_connection() and connection_exists() +# +# @TEST-EXEC: zeek -b -r $TRACES/http/get.trace %INPUT +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff .stderr + +event new_connection(c: connection) + { + local c2 = lookup_connection(c$id); + assert c$uid == c2$uid; + + local cid: conn_id; + cid$orig_h = c$id$orig_h; + cid$orig_p = c$id$orig_p; + cid$resp_h = c$id$resp_h; + cid$resp_p = c$id$resp_p; + + # Produces an error on .stderr because cid$proto wasn't + # initialized and then returns a dummy record. + local c3 = lookup_connection(cid); + assert c3$history == ""; + assert c3$id$orig_h == 0.0.0.0; + assert c3$id$orig_p == 0/udp; + + cid$proto = c$id$proto; + local c4 = lookup_connection(cid); + assert c$uid == c4$uid; + } + +event new_connection(c: connection) + { + # This needs to hold. + assert connection_exists(c$id); + + local my_id: conn_id; + my_id$orig_h = c$id$orig_h; + my_id$orig_p = c$id$orig_p; + my_id$resp_h = c$id$resp_h; + my_id$resp_p = c$id$resp_p; + + # Produces an error because cid$proto wasn't initialized. + assert ! connection_exists(my_id); + + my_id$proto = c$id$proto; + assert connection_exists(my_id); + } + +event new_connection(c: connection) + { + # This crashed previously! + local my_id: conn_id; + local c2 = lookup_connection(my_id); + assert c2$history == ""; + assert c2$id$orig_h == 0.0.0.0; + assert c2$id$orig_p == 0/udp; + + # This also crashed! + assert ! connection_exists(my_id); + }