diff --git a/CHANGES b/CHANGES index 597a8faddd..0b5a013a83 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,28 @@ +7.2.0-dev.104 | 2025-01-21 16:48:56 -0700 + + * session/Manager: Emit explicit errors for FindConnection() with proto=65535 (Arne Welzel, Corelight) + + 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. + + * IPAddr/ConnKey: Protect from uninitialized conn_id (Arne Welzel, Corelight) + + Check if the non-default fields exist using HasField() + and use GetField() for proto such that it'll initialize + the default value which GetFieldAs<> doesn't do. + default + + * IPAddr/ConnKey: Promote transport to uint16_t (Arne Welzel, Corelight) + + Instead of a separate bool field which is also stored in the session + table, promote the transport field to uint16_t and encode an invalid + ConnKey as transport 2**16-2 + + * session/Manager: Header cleanup (Arne Welzel, Corelight) + 7.2.0-dev.99 | 2025-01-20 10:27:32 +0100 * fixes for -O gen-standalone-C++ generation of lambdas (Vern Paxson, Corelight) diff --git a/VERSION b/VERSION index b72def6ac0..426c129ef3 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -7.2.0-dev.99 +7.2.0-dev.104 diff --git a/src/IPAddr.cc b/src/IPAddr.cc index ea32a2f755..1df6e26a4e 100644 --- a/src/IPAddr.cc +++ b/src/IPAddr.cc @@ -4,14 +4,12 @@ #include #include -#include #include "zeek/3rdparty/zeek_inet_ntop.h" #include "zeek/Conn.h" #include "zeek/Hash.h" #include "zeek/Reporter.h" #include "zeek/ZeekString.h" -#include "zeek/analyzer/Manager.h" namespace zeek { @@ -20,7 +18,7 @@ const IPAddr IPAddr::v6_unspecified = IPAddr(); namespace detail { -ConnKey::ConnKey(const IPAddr& src, const IPAddr& dst, uint16_t src_port, uint16_t dst_port, uint8_t proto, +ConnKey::ConnKey(const IPAddr& src, const IPAddr& dst, uint16_t src_port, uint16_t dst_port, uint16_t proto, bool one_way) { Init(src, dst, src_port, dst_port, proto, one_way); } @@ -43,7 +41,6 @@ ConnKey& ConnKey::operator=(const ConnKey& rhs) { port1 = rhs.port1; port2 = rhs.port2; transport = rhs.transport; - valid = rhs.valid; return *this; } @@ -51,7 +48,7 @@ ConnKey& ConnKey::operator=(const ConnKey& rhs) { ConnKey::ConnKey(Val* v) { const auto& vt = v->GetType(); if ( ! IsRecord(vt->Tag()) ) { - valid = false; + transport = INVALID_CONN_KEY_IP_PROTO; return; } @@ -78,7 +75,7 @@ ConnKey::ConnKey(Val* v) { proto = vr->FieldOffset("proto"); if ( orig_h < 0 || resp_h < 0 || orig_p < 0 || resp_p < 0 || proto < 0 ) { - valid = false; + transport = INVALID_CONN_KEY_IP_PROTO; return; } @@ -86,19 +83,24 @@ ConnKey::ConnKey(Val* v) { // types, too. } + if ( ! vl->HasField(orig_h) || ! vl->HasField(resp_h) || ! vl->HasField(orig_p) || ! vl->HasField(resp_p) ) { + transport = INVALID_CONN_KEY_IP_PROTO; + return; + } + const IPAddr& orig_addr = vl->GetFieldAs(orig_h); const IPAddr& resp_addr = vl->GetFieldAs(resp_h); - auto orig_portv = vl->GetFieldAs(orig_p); - auto resp_portv = vl->GetFieldAs(resp_p); + const auto& orig_portv = vl->GetFieldAs(orig_p); + const auto& resp_portv = vl->GetFieldAs(resp_p); - auto protov = vl->GetFieldAs(proto); + const auto& protov = vl->GetField(proto); Init(orig_addr, resp_addr, htons((unsigned short)orig_portv->Port()), htons((unsigned short)resp_portv->Port()), - protov, false); + protov->AsCount(), false); } -void ConnKey::Init(const IPAddr& src, const IPAddr& dst, uint16_t src_port, uint16_t dst_port, uint8_t proto, +void ConnKey::Init(const IPAddr& src, const IPAddr& dst, uint16_t src_port, uint16_t dst_port, uint16_t proto, bool one_way) { // Because of padding in the object, this needs to memset to clear out // the extra memory used by padding. Otherwise, the session key stuff @@ -122,7 +124,6 @@ void ConnKey::Init(const IPAddr& src, const IPAddr& dst, uint16_t src_port, uint } transport = proto; - valid = true; } } // namespace detail diff --git a/src/IPAddr.h b/src/IPAddr.h index b8323fb7a3..64e489ac62 100644 --- a/src/IPAddr.h +++ b/src/IPAddr.h @@ -20,6 +20,9 @@ class Val; namespace detail { +// UNKNOWN_IP_PROTO is 65535 +constexpr uint16_t INVALID_CONN_KEY_IP_PROTO = 65534; + class HashKey; class ConnKey { @@ -28,10 +31,9 @@ public: in6_addr ip2; uint16_t port1 = 0; uint16_t port2 = 0; - uint8_t transport; - bool valid = true; + uint16_t transport = INVALID_CONN_KEY_IP_PROTO; - ConnKey(const IPAddr& src, const IPAddr& dst, uint16_t src_port, uint16_t dst_port, uint8_t proto, bool one_way); + ConnKey(const IPAddr& src, const IPAddr& dst, uint16_t src_port, uint16_t dst_port, uint16_t proto, bool one_way); ConnKey(const ConnTuple& conn); ConnKey(const ConnKey& rhs) { *this = rhs; } ConnKey(Val* v); @@ -45,8 +47,10 @@ public: ConnKey& operator=(const ConnKey& rhs); + bool Valid() const { return transport <= 0xFF; }; + private: - void Init(const IPAddr& src, const IPAddr& dst, uint16_t src_port, uint16_t dst_port, uint8_t proto, bool one_way); + void Init(const IPAddr& src, const IPAddr& dst, uint16_t src_port, uint16_t dst_port, uint16_t proto, bool one_way); }; } // namespace detail diff --git a/src/session/Manager.cc b/src/session/Manager.cc index ef5ce4a76d..131805ef85 100644 --- a/src/session/Manager.cc +++ b/src/session/Manager.cc @@ -2,27 +2,25 @@ #include "zeek/session/Manager.h" -#include "zeek/zeek-config.h" - #include #include #include #include #include -#include "zeek/Desc.h" -#include "zeek/Event.h" +#include "zeek/Conn.h" +#include "zeek/Func.h" +#include "zeek/IP.h" #include "zeek/NetVar.h" #include "zeek/Reporter.h" #include "zeek/RuleMatcher.h" #include "zeek/RunState.h" #include "zeek/Timer.h" #include "zeek/TunnelEncapsulation.h" -#include "zeek/analyzer/Manager.h" -#include "zeek/iosource/IOSource.h" #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; @@ -94,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); + }