Merge remote-tracking branch 'origin/topic/awelzel/lookup-connection-tweaks'

* origin/topic/awelzel/lookup-connection-tweaks:
  session/Manager: Emit explicit errors for FindConnection() with proto=65535
  IPAddr/ConnKey: Protect from uninitialized conn_id
  IPAddr/ConnKey: Promote transport to uint16_t
  session/Manager: Header cleanup

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)

VERSION

@@ -1 +1 @@
-7.2.0-dev.99
+7.2.0-dev.104

src/IPAddr.cc

@@ -4,14 +4,12 @@
 
 #include <cstdlib>
 #include <string>
-#include <vector>
 
 #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<AddrVal>(orig_h);
     const IPAddr& resp_addr = vl->GetFieldAs<AddrVal>(resp_h);
 
-    auto orig_portv = vl->GetFieldAs<PortVal>(orig_p);
-    auto resp_portv = vl->GetFieldAs<PortVal>(resp_p);
+    const auto& orig_portv = vl->GetFieldAs<PortVal>(orig_p);
+    const auto& resp_portv = vl->GetFieldAs<PortVal>(resp_p);
 
-    auto protov = vl->GetFieldAs<CountVal>(proto);
+    const auto& protov = vl->GetField<CountVal>(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

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

src/session/Manager.cc

@@ -2,27 +2,25 @@
 
 #include "zeek/session/Manager.h"
 
-#include "zeek/zeek-config.h"
-
 #include <arpa/inet.h>
 #include <netinet/in.h>
 #include <pcap.h>
 #include <unistd.h>
 #include <cstdlib>
 
-#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);
 }

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=<uninitialized>, orig_p=<uninitialized>, resp_h=<uninitialized>, resp_p=<uninitialized>, proto=65535])
+1362692526.869344 error in <...>/lookup_connection.zeek, line 57: invalid connection ID record encountered (connection_exists(my_id))

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);
+	}