From d6e6fda32725c207385df6ae65c3d9ec21dbe106 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Tue, 13 May 2025 11:31:56 +0200 Subject: [PATCH] More fixes --- src/Conn.cc | 3 +- src/Conn.h | 3 +- src/analyzer/Analyzer.cc | 8 +- src/analyzer/protocol/smtp/BDAT.cc | 4 +- src/conntuple/Builder.cc | 78 +++++++++++++++- src/conntuple/Builder.h | 7 ++ src/conntuple/vlan/Builder.cc | 43 +++++++-- src/conntuple/vlan/Builder.h | 1 + src/packet_analysis/protocol/gtpv1/GTPv1.cc | 8 +- .../protocol/gtpv1/functions.bif | 4 +- .../protocol/ip/IPBasedAnalyzer.cc | 91 ++++--------------- .../protocol/ip/IPBasedAnalyzer.h | 75 ++++++++------- src/packet_analysis/protocol/teredo/Teredo.cc | 11 +-- src/packet_analysis/protocol/teredo/Teredo.h | 2 +- .../protocol/teredo/functions.bif | 4 +- src/session/Key.h | 33 +++---- src/session/Manager.cc | 22 ++--- src/session/Manager.h | 2 +- 18 files changed, 222 insertions(+), 177 deletions(-) diff --git a/src/Conn.cc b/src/Conn.cc index 1222d5c806..0953646ee7 100644 --- a/src/Conn.cc +++ b/src/Conn.cc @@ -29,7 +29,8 @@ uint64_t Connection::total_connections = 0; uint64_t Connection::current_connections = 0; Connection::Connection(IPBasedConnKeyPtr k, zeek::ConnTuple& ct, double t, uint32_t flow, const Packet* pkt) - : Session(t, connection_timeout, connection_status_update, detail::connection_status_update_interval), key(k) { + : Session(t, connection_timeout, connection_status_update, detail::connection_status_update_interval), + key(std::move(k)) { orig_addr = ct.src_addr; resp_addr = ct.dst_addr; orig_port = ct.src_port; diff --git a/src/Conn.h b/src/Conn.h index 62361240eb..1c8d7862c6 100644 --- a/src/Conn.h +++ b/src/Conn.h @@ -54,7 +54,7 @@ enum ConnEventToFlag { }; class IPBasedConnKey; -using IPBasedConnKeyPtr = zeek::IntrusivePtr; +using IPBasedConnKeyPtr = std::unique_ptr; static inline int addr_port_canon_lt(const IPAddr& addr1, uint32_t p1, const IPAddr& addr2, uint32_t p2) { return addr1 < addr2 || (addr1 == addr2 && p1 < p2); @@ -64,7 +64,6 @@ static inline int addr_port_canon_lt(const IPAddr& addr1, uint32_t p1, const IPA class Connection final : public session::Session { public: Connection(zeek::IPBasedConnKeyPtr k, zeek::ConnTuple& ct, double t, uint32_t flow, const Packet* pkt); - // Connection(const detail::ConnKey& k, double t, const ConnTuple* id, uint32_t flow, const Packet* pkt); ~Connection() override; /** diff --git a/src/analyzer/Analyzer.cc b/src/analyzer/Analyzer.cc index 5c08749037..2d5b3d6976 100644 --- a/src/analyzer/Analyzer.cc +++ b/src/analyzer/Analyzer.cc @@ -814,9 +814,9 @@ TEST_SUITE("Analyzer management") { zeek::Packet p; zeek::ConnTuple ct; - zeek::IPBasedConnKeyPtr kp = zeek::make_intrusive(); + zeek::IPBasedConnKeyPtr kp = std::make_unique(); // auto conn = std::make_unique(zeek::detail::OLD_ConnKey(t), 0, &t, 0, &p); - auto conn = std::make_unique(kp, ct, 0, 0, &p); + auto conn = std::make_unique(std::move(kp), ct, 0, 0, &p); auto* tcp = new zeek::packet_analysis::TCP::TCPSessionAdapter(conn.get()); conn->SetSessionAdapter(tcp, nullptr); @@ -848,8 +848,8 @@ TEST_SUITE("Analyzer management") { zeek::Packet p; zeek::ConnTuple ct; - zeek::IPBasedConnKeyPtr kp = zeek::make_intrusive(); - auto conn = std::make_unique(kp, ct, 0, 0, &p); + zeek::IPBasedConnKeyPtr kp = std::make_unique(); + auto conn = std::make_unique(std::move(kp), ct, 0, 0, &p); auto ssh = zeek::analyzer_mgr->InstantiateAnalyzer("SSH", conn.get()); REQUIRE(ssh); diff --git a/src/analyzer/protocol/smtp/BDAT.cc b/src/analyzer/protocol/smtp/BDAT.cc index 8a12c4c066..210ed1de1c 100644 --- a/src/analyzer/protocol/smtp/BDAT.cc +++ b/src/analyzer/protocol/smtp/BDAT.cc @@ -329,8 +329,8 @@ private: TEST_CASE("line forward testing") { zeek::Packet p; zeek::ConnTuple ct; - zeek::IPBasedConnKeyPtr kp = zeek::make_intrusive(); - auto conn = std::make_unique(kp, ct, 0, 0, &p); + zeek::IPBasedConnKeyPtr kp = std::make_unique(); + auto conn = std::make_unique(std::move(kp), ct, 0, 0, &p); auto smtp_analyzer = std::unique_ptr(zeek::analyzer_mgr->InstantiateAnalyzer("SMTP", conn.get())); auto mail = std::make_unique(smtp_analyzer.get()); diff --git a/src/conntuple/Builder.cc b/src/conntuple/Builder.cc index 3175bd91de..91847e1bf4 100644 --- a/src/conntuple/Builder.cc +++ b/src/conntuple/Builder.cc @@ -2,6 +2,7 @@ #include "zeek/conntuple/Builder.h" +#include "zeek/Val.h" #include "zeek/packet_analysis/protocol/ip/IPBasedAnalyzer.h" namespace zeek::conntuple { @@ -9,6 +10,81 @@ namespace zeek::conntuple { Builder::Builder() {} Builder::~Builder() {} -zeek::ConnKeyPtr Builder::NewConnKey() { return zeek::make_intrusive(); } +bool fill_from_val(const IPBasedConnKey* ck, const zeek::ValPtr& v) { + auto& t = ck->RawTuple(); + + const auto& vt = v->GetType(); + if ( ! IsRecord(vt->Tag()) ) { + t.transport = detail::INVALID_CONN_KEY_IP_PROTO; + assert(ck->Error().has_value()); + return false; + } + + RecordType* vr = vt->AsRecordType(); + auto vl = v->As(); + + int orig_h, orig_p; // indices into record's value list + int resp_h, resp_p; + int proto; + + if ( vr == id::conn_id ) { + orig_h = 0; + orig_p = 1; + resp_h = 2; + resp_p = 3; + proto = 4; + } + else { + // While it's not a conn_id, it may have equivalent fields. + orig_h = vr->FieldOffset("orig_h"); + resp_h = vr->FieldOffset("resp_h"); + orig_p = vr->FieldOffset("orig_p"); + resp_p = vr->FieldOffset("resp_p"); + proto = vr->FieldOffset("proto"); + + if ( orig_h < 0 || resp_h < 0 || orig_p < 0 || resp_p < 0 || proto < 0 ) { + t.transport = detail::INVALID_CONN_KEY_IP_PROTO; + assert(ck->Error().has_value()); + return false; + } + + // TODO we ought to check that the fields have the right + // types, too. + } + + if ( ! vl->HasField(orig_h) || ! vl->HasField(resp_h) || ! vl->HasField(orig_p) || ! vl->HasField(resp_p) ) { + t.transport = detail::INVALID_CONN_KEY_IP_PROTO; + assert(ck->Error().has_value()); + return false; + } + + const IPAddr& orig_addr = vl->GetFieldAs(orig_h); + const IPAddr& resp_addr = vl->GetFieldAs(resp_h); + + const auto& orig_portv = vl->GetFieldAs(orig_p); + const auto& resp_portv = vl->GetFieldAs(resp_p); + + const auto& protov = vl->GetField(proto); + + auto ct = ConnTuple{orig_addr, resp_addr, ntohs(orig_portv->Port()), ntohs(resp_portv->Port()), + static_cast(protov->AsCount())}; + + detail::init_raw_tuple(t, ct); + assert(! ck->Error().has_value()); + return true; +} + +zeek::ConnKeyPtr Builder::NewConnKey() { return std::make_unique(); } + +// Creating a ConnKey instance from a ValPtr, assuming conn_id. +zeek::ConnKeyPtr Builder::FromVal(const zeek::ValPtr& v) { + auto ck = NewConnKey(); + auto* k = static_cast(ck.get()); + if ( ! fill_from_val(k, v) ) { + assert(ck->Error().has_value()); + } + + return ck; +} } // namespace zeek::conntuple diff --git a/src/conntuple/Builder.h b/src/conntuple/Builder.h index 40a7a2bf4b..c99237071d 100644 --- a/src/conntuple/Builder.h +++ b/src/conntuple/Builder.h @@ -14,12 +14,19 @@ namespace conntuple { class Builder; using BuilderPtr = std::unique_ptr; +/** + * Fill an IPBasedConnKey from a Zeek script value. + */ +bool fill_from_val(const IPBasedConnKey* k, const zeek::ValPtr& v); + class Builder { public: Builder(); virtual ~Builder(); + // TODO: Should these better be abstract? virtual zeek::ConnKeyPtr NewConnKey(); + virtual zeek::ConnKeyPtr FromVal(const zeek::ValPtr& v); static zeek::conntuple::BuilderPtr Instantiate() { return std::make_unique(); } }; diff --git a/src/conntuple/vlan/Builder.cc b/src/conntuple/vlan/Builder.cc index c058fc6b8d..c58ed5ee35 100644 --- a/src/conntuple/vlan/Builder.cc +++ b/src/conntuple/vlan/Builder.cc @@ -2,7 +2,10 @@ #include "zeek/conntuple/vlan/Builder.h" +#include + #include "zeek/ID.h" +#include "zeek/conntuple/Builder.h" #include "zeek/packet_analysis/protocol/ip/IPBasedAnalyzer.h" #include "zeek/session/Session.h" @@ -20,18 +23,11 @@ public: key.inner_vlan = pkt.inner_vlan; } - bool FromConnIdVal(const zeek::RecordValPtr& rv) override { - if ( ! zeek::IPBasedConnKey::FromConnIdVal(rv) ) - return false; - - // TODO: Also load vlan and inner_vlan from the record! - } - zeek::Span Key() const override { return {reinterpret_cast(&key), reinterpret_cast(&key) + sizeof(key)}; } - detail::RawConnTuple& RawTuple() override { return key.tuple; } + detail::RawConnTuple& RawTuple() const override { return key.tuple; } virtual void FillConnIdVal(RecordValPtr& conn_id) override { if ( conn_id->NumFields() <= 5 ) @@ -47,14 +43,41 @@ public: }; private: + friend class Builder; + // Key bytes. struct { - struct detail::RawConnTuple tuple; + // mutable for non-const RawTuple() return value. + mutable struct detail::RawConnTuple tuple; // Add 802.1Q vlan tags to connection tuples. The tag representation here is as // in the Packet class, since that's where we learn the tag values from. uint32_t vlan; uint32_t inner_vlan; - } key; + } __attribute__((packed, aligned)) key; }; +zeek::ConnKeyPtr Builder::NewConnKey() { return std::make_unique(); } + +zeek::ConnKeyPtr Builder::FromVal(const zeek::ValPtr& v) { + auto ck = NewConnKey(); + auto* k = static_cast(ck.get()); + if ( ! zeek::conntuple::fill_from_val(k, v) ) { + assert(ck->Error().has_value()); + return ck; + } + + auto rt = v->GetType()->AsRecordType(); + auto vl = v->As(); + + // XXX: Use static field offsets + if ( rt->GetFieldType(5)->Tag() == TYPE_INT && vl->HasField(5) ) + k->key.vlan = vl->GetFieldAs(5); + + if ( rt->GetFieldType(6)->Tag() == TYPE_INT && vl->HasField(6) ) + k->key.inner_vlan = vl->GetFieldAs(6); + + + return ck; +} + } // namespace zeek::plugin::Zeek_Conntuple_VLAN diff --git a/src/conntuple/vlan/Builder.h b/src/conntuple/vlan/Builder.h index 1e13de508e..7e5b3fb627 100644 --- a/src/conntuple/vlan/Builder.h +++ b/src/conntuple/vlan/Builder.h @@ -8,6 +8,7 @@ namespace zeek::plugin::Zeek_Conntuple_VLAN { class Builder : public conntuple::Builder { public: virtual zeek::ConnKeyPtr NewConnKey() override; + virtual zeek::ConnKeyPtr FromVal(const zeek::ValPtr& v) override; }; } // namespace zeek::plugin::Zeek_Conntuple_VLAN diff --git a/src/packet_analysis/protocol/gtpv1/GTPv1.cc b/src/packet_analysis/protocol/gtpv1/GTPv1.cc index 1c2fa44901..37f62a328a 100644 --- a/src/packet_analysis/protocol/gtpv1/GTPv1.cc +++ b/src/packet_analysis/protocol/gtpv1/GTPv1.cc @@ -22,12 +22,12 @@ bool GTPv1_Analyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pack const auto& key = conn->Key(); auto sk = key.SessionKey(); - auto cm_it = conn_map.find(sk); if ( cm_it == conn_map.end() ) { - sk.CopyData(); - ConnMap::value_type p{std::move(sk), std::make_unique(this)}; - // cm_it = conn_map.insert(cm_it, std::move(p)); + sk.CopyData(); // Copy key data to store in map. + auto [it, inserted] = conn_map.emplace(std::move(sk), std::make_unique(this)); + assert(inserted); + cm_it = it; // Let script land know about the state we created, so it will // register a conn removal hook for cleanup. diff --git a/src/packet_analysis/protocol/gtpv1/functions.bif b/src/packet_analysis/protocol/gtpv1/functions.bif index 6880dc3a3c..99cbbcec0d 100644 --- a/src/packet_analysis/protocol/gtpv1/functions.bif +++ b/src/packet_analysis/protocol/gtpv1/functions.bif @@ -13,8 +13,8 @@ function remove_gtpv1_connection%(cid: conn_id%) : bool zeek::packet_analysis::AnalyzerPtr gtpv1 = zeek::packet_mgr->GetAnalyzer("GTPv1"); if ( gtpv1 ) { - auto ck = zeek::conntuple_mgr->GetBuilder().NewConnKey(); - // ck->LoadConnIdVal(v); + // XXX: Should be IP specific builder! + auto ck = zeek::conntuple_mgr->GetBuilder().FromVal({zeek::NewRef{}, cid}); auto sk = ck->SessionKey(); static_cast(gtpv1.get())->RemoveConnection(sk); } diff --git a/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc b/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc index dc8c098158..f5638e8880 100644 --- a/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc +++ b/src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc @@ -2,6 +2,8 @@ #include "zeek/packet_analysis/protocol/ip/IPBasedAnalyzer.h" +#include + #include "zeek/Conn.h" #include "zeek/RunState.h" #include "zeek/Val.h" @@ -14,65 +16,6 @@ using namespace zeek; using namespace zeek::packet_analysis::IP; -// Populate a ConnKey from a conn_id record instance. -bool IPBasedConnKey::FromConnIdVal(const zeek::RecordValPtr& rv) { - auto& t = RawTuple(); - const auto& vt = rv->GetType(); - if ( ! IsRecord(vt->Tag()) ) { - t.transport = detail::INVALID_CONN_KEY_IP_PROTO; - return false; - } - - RecordType* vr = vt->AsRecordType(); - auto vl = rv->As(); - - int orig_h, orig_p; // indices into record's value list - int resp_h, resp_p; - int proto; - - if ( vr == id::conn_id ) { - orig_h = 0; - orig_p = 1; - resp_h = 2; - resp_p = 3; - proto = 4; - } - else { - // While it's not a conn_id, it may have equivalent fields. - orig_h = vr->FieldOffset("orig_h"); - resp_h = vr->FieldOffset("resp_h"); - orig_p = vr->FieldOffset("orig_p"); - resp_p = vr->FieldOffset("resp_p"); - proto = vr->FieldOffset("proto"); - - if ( orig_h < 0 || resp_h < 0 || orig_p < 0 || resp_p < 0 || proto < 0 ) { - t.transport = detail::INVALID_CONN_KEY_IP_PROTO; - return false; - } - - // TODO we ought to check that the fields have the right - // types, too. - } - - if ( ! vl->HasField(orig_h) || ! vl->HasField(resp_h) || ! vl->HasField(orig_p) || ! vl->HasField(resp_p) ) { - t.transport = detail::INVALID_CONN_KEY_IP_PROTO; - return false; - } - - const IPAddr& orig_addr = vl->GetFieldAs(orig_h); - const IPAddr& resp_addr = vl->GetFieldAs(resp_h); - - const auto& orig_portv = vl->GetFieldAs(orig_p); - const auto& resp_portv = vl->GetFieldAs(resp_p); - - const auto& protov = vl->GetField(proto); - - auto ct = ConnTuple{orig_addr, resp_addr, orig_portv->Port(), resp_portv->Port(), - static_cast(protov->AsCount())}; - InitRawConnTuple(ct); - return true; -} - IPBasedAnalyzer::IPBasedAnalyzer(const char* name, TransportProto proto, uint32_t mask, bool report_unknown_protocols) : zeek::packet_analysis::Analyzer(name, report_unknown_protocols), transport(proto), server_port_mask(mask) {} @@ -87,17 +30,21 @@ bool IPBasedAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pkt if ( ! BuildConnTuple(len, data, pkt, tuple) ) return false; - // The IPBasedAnalyzer requires a builder that produces IPConnKey instances. - // We could check with dynamic_cast, but that's probably slow, so assume - // plugin providers know what they're doing here and anyhow, we don't really - // have analyzers that instantiate non-IP connections today and definitely - // not here! - auto key = cast_intrusive(zeek::conntuple_mgr->GetBuilder().NewConnKey()); + static IPBasedConnKeyPtr key; // Watch: This is static for reuse! + if ( ! key ) { + ConnKeyPtr ck = conntuple_mgr->GetBuilder().NewConnKey(); + + // The IPBasedAnalyzer requires a builder that produces IPBasedConnKey instances. + // We could check with dynamic_cast, but that's probably slow, so assume plugin + // providers know what they're doing here and anyhow, we don't really have analyzers + // that instantiate non-IP connections today and definitely not here! + key = std::unique_ptr(static_cast(ck.release())); + } // Initialize the key with the IP conn tuple and the packet as additional context. // // Custom IPConnKey implementations can fiddle with the Key through - // the DoInit(const Packet& pkt) hook at this point. + // the DoInit(const Packet& pkt) hook called at this point. key->Init(tuple, *pkt); const std::shared_ptr& ip_hdr = pkt->ip_hdr; @@ -108,6 +55,8 @@ bool IPBasedAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pkt conn = NewConn(std::move(key), tuple, pkt); if ( conn ) session_mgr->Insert(conn, false); + + assert(! key); } else { if ( conn->IsReuse(run_state::processing_start_time, ip_hdr->Payload()) ) { @@ -117,16 +66,14 @@ bool IPBasedAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pkt conn = NewConn(std::move(key), tuple, pkt); if ( conn ) session_mgr->Insert(conn, false); + + assert(! key); } else { conn->CheckEncapsulation(pkt->encap); - // We could give back the ConnKey for re-use to avoid the malloc/memset() - // overhead if we already knew about the session and don't had a use for - // the key other than facilitating the lookup of the session. Not sure - // that's worth it. - // - // GetBuilder().ReturnKey(std::move(key)); + // The next time we get here, the key instance + assert(key); } } diff --git a/src/packet_analysis/protocol/ip/IPBasedAnalyzer.h b/src/packet_analysis/protocol/ip/IPBasedAnalyzer.h index 22cede572e..d828cef160 100644 --- a/src/packet_analysis/protocol/ip/IPBasedAnalyzer.h +++ b/src/packet_analysis/protocol/ip/IPBasedAnalyzer.h @@ -4,6 +4,7 @@ #include #include +#include #include #include "zeek/Conn.h" @@ -30,13 +31,36 @@ namespace detail { // UNKNOWN_IP_PROTO is 65535 constexpr uint16_t INVALID_CONN_KEY_IP_PROTO = 65534; +/** + * Struct for embedding into a IPBasedConnKey. + */ struct RawConnTuple { in6_addr ip1; in6_addr ip2; uint16_t port1 = 0; uint16_t port2 = 0; uint16_t transport = detail::INVALID_CONN_KEY_IP_PROTO; -}; +} __attribute__((packed, aligned)); + +/** + * Initialize a raw conn tuple from a conn tuple in canonicalized form. + */ +inline void init_raw_tuple(RawConnTuple& t, const ConnTuple& ct) { + if ( ct.is_one_way || addr_port_canon_lt(ct.src_addr, ct.src_port, ct.dst_addr, ct.dst_port) ) { + ct.src_addr.CopyIPv6(&t.ip1); + ct.dst_addr.CopyIPv6(&t.ip2); + t.port1 = ct.src_port; + t.port2 = ct.dst_port; + } + else { + ct.dst_addr.CopyIPv6(&t.ip1); + ct.src_addr.CopyIPv6(&t.ip2); + t.port1 = ct.dst_port; + t.port2 = ct.src_port; + } + + t.transport = ct.proto; +} } // namespace detail @@ -56,44 +80,31 @@ public: * subclasses to hook into the initialization of the key. */ void Init(const ConnTuple& ct, const Packet& pkt) { - InitRawConnTuple(ct); + init_raw_tuple(RawTuple(), ct); ConnKey::Init(pkt); } - bool FromConnIdVal(const zeek::RecordValPtr& conn_id) override; + std::optional Error() const override { + auto& rt = RawTuple(); + if ( rt.transport == detail::INVALID_CONN_KEY_IP_PROTO ) + return "invalid connection ID record"; + if ( rt.transport == UNKNOWN_IP_PROTO ) + return "invalid connection ID record: the proto field has the \"unknown\" 65535 value. Did you forget to " + "set it?"; + + return std::nullopt; + } /** - * Return a modifiable version of the raw Conn Tuple. + * Return a modifiable version of the embedded RawConnTuple. * * This is virtual such that subclasses can control where - * their own ConnTuple instance to allow. + * they'd like to place the RawConnTuple within the key. */ - virtual detail::RawConnTuple& RawTuple() = 0; - -protected: - /** - * Helper for subclasses that override Init() to initialize this key's tuple in normalized form. - */ - void InitRawConnTuple(const ConnTuple& ct) { - auto& t = RawTuple(); - if ( ct.is_one_way || addr_port_canon_lt(ct.src_addr, ct.src_port, ct.dst_addr, ct.dst_port) ) { - ct.src_addr.CopyIPv6(&t.ip1); - ct.dst_addr.CopyIPv6(&t.ip2); - t.port1 = ct.src_port; - t.port2 = ct.dst_port; - } - else { - ct.dst_addr.CopyIPv6(&t.ip1); - ct.src_addr.CopyIPv6(&t.ip2); - t.port1 = ct.dst_port; - t.port2 = ct.src_port; - } - - t.transport = ct.proto; - } + virtual detail::RawConnTuple& RawTuple() const = 0; }; -using IPBasedConnKeyPtr = zeek::IntrusivePtr; +using IPBasedConnKeyPtr = std::unique_ptr; /** @@ -103,7 +114,6 @@ class IPConnKey : public IPBasedConnKey { public: IPConnKey() { // Fill holes as we use the full tuple as a Key! - // Could we h memset(static_cast(&key), '\0', sizeof(key)); } @@ -111,11 +121,12 @@ public: return {reinterpret_cast(&key), reinterpret_cast(&key) + sizeof(key)}; } - detail::RawConnTuple& RawTuple() override { return key.tuple; } + detail::RawConnTuple& RawTuple() const override { return key.tuple; } private: struct { - struct detail::RawConnTuple tuple; + // mutable for non-const RawTuple() return value. + mutable struct detail::RawConnTuple tuple; } key; }; diff --git a/src/packet_analysis/protocol/teredo/Teredo.cc b/src/packet_analysis/protocol/teredo/Teredo.cc index af75bbc58c..f19e3b37c3 100644 --- a/src/packet_analysis/protocol/teredo/Teredo.cc +++ b/src/packet_analysis/protocol/teredo/Teredo.cc @@ -185,10 +185,8 @@ bool TeredoAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pack return false; } - auto& k = conn->Key(); + const auto& k = conn->Key(); auto sk = k.SessionKey(); - - // zeek::detail::OLD_ConnKey conn_key = conn->Key(); OrigRespMap::iterator or_it = orig_resp_map.find(sk); // The first time a teredo packet is parsed successfully, insert @@ -196,9 +194,10 @@ bool TeredoAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pack // see valid Teredo packets. Further, raise an event so that script // layer can install a connection removal hooks to cleanup later. if ( or_it == orig_resp_map.end() ) { - sk.CopyData(); - // OrigRespMap::value_type p{std::move(sk), OrigResp{}}; - auto [it, inserted] = orig_resp_map.insert_or_assign(std::move(sk), OrigResp{}); + sk.CopyData(); // Copy key data to store in map. + auto [it, inserted] = orig_resp_map.emplace(std::move(sk), OrigResp{}); + assert(inserted); + or_it = it; packet->session->EnqueueEvent(new_teredo_state, nullptr, packet->session->GetVal()); } diff --git a/src/packet_analysis/protocol/teredo/Teredo.h b/src/packet_analysis/protocol/teredo/Teredo.h index 70be32f600..00f998707c 100644 --- a/src/packet_analysis/protocol/teredo/Teredo.h +++ b/src/packet_analysis/protocol/teredo/Teredo.h @@ -54,7 +54,7 @@ protected: bool valid_resp = false; bool confirmed = false; }; - using OrigRespMap = std::unordered_map; + using OrigRespMap = std::map; OrigRespMap orig_resp_map; std::unique_ptr pattern_re; diff --git a/src/packet_analysis/protocol/teredo/functions.bif b/src/packet_analysis/protocol/teredo/functions.bif index 947e88c9fa..1ec560e244 100644 --- a/src/packet_analysis/protocol/teredo/functions.bif +++ b/src/packet_analysis/protocol/teredo/functions.bif @@ -13,8 +13,8 @@ function remove_teredo_connection%(cid: conn_id%) : bool zeek::packet_analysis::AnalyzerPtr teredo = zeek::packet_mgr->GetAnalyzer("Teredo"); if ( teredo ) { - auto ck = zeek::conntuple_mgr->GetBuilder().NewConnKey(); - // ck->LoadConnIdVal(v); + // XXX: Should be IP specific builder! + auto ck = zeek::conntuple_mgr->GetBuilder().FromVal({zeek::NewRef{}, cid}); auto sk = ck->SessionKey(); static_cast(teredo.get())->RemoveConnection(sk); } diff --git a/src/session/Key.h b/src/session/Key.h index c69789e27f..d55f31f538 100644 --- a/src/session/Key.h +++ b/src/session/Key.h @@ -4,6 +4,7 @@ #include #include +#include #include "zeek/Hash.h" #include "zeek/IntrusivePtr.h" @@ -100,7 +101,14 @@ class ConnKey { public: virtual ~ConnKey() {} - // Init function called by analyzers or subclassed. + /** + * ConnKeys created from Vals may be invalid, Error() can be used to determine validity. + */ + virtual std::optional Error() const = 0; + + /** + * Initialization of this key with the current packet. + */ void Init(const Packet& pkt) { DoInit(pkt); } /** @@ -117,11 +125,6 @@ public: */ virtual void FillConnIdVal(RecordValPtr& conn_id) {}; - /** - * Populate a ConnKey instance from a script layer record value. - */ - virtual bool FromConnIdVal(const RecordValPtr& conn_id) = 0; - /** * They Key over which to compute a hash or use for comparison with other keys. * @@ -130,28 +133,16 @@ public: virtual zeek::Span Key() const = 0; /** - * View over the key data as a SessionKey. + * View over key data as returned by Key() as session::detail::Key instance. * - * Not sure this makes much sense, it might also be possible to switch - * the session_map to hold ConnKeyPtr instead with specialized hash and - * equals functions instead. + * Mostly for plumbing the session/Manager.h code. */ zeek::session::detail::Key SessionKey() const { auto span = Key(); return zeek::session::detail::Key(span.data(), span.size(), session::detail::Key::CONNECTION_KEY_TYPE); } - - // Support usage as IntrusivePtr. - int ref_cnt = 1; }; -inline void Ref(ConnKey* k) { ++k->ref_cnt; } - -inline void Unref(ConnKey* k) { - if ( --k->ref_cnt == 0 ) - delete k; -} - -using ConnKeyPtr = zeek::IntrusivePtr; +using ConnKeyPtr = std::unique_ptr; } // namespace zeek diff --git a/src/session/Manager.cc b/src/session/Manager.cc index 0e14dd1618..6433957087 100644 --- a/src/session/Manager.cc +++ b/src/session/Manager.cc @@ -91,31 +91,21 @@ Manager::~Manager() { void Manager::Done() {} Connection* Manager::FindConnection(Val* v) { - // zeek::detail::OLD_ConnKeyPtr conn_key = conntuple_mgr->GetBuilder().GetKey(v); + // XXX: Technically we should probably dispatch between different builders for different kinds of Val instances. + // conn_id is IP specific, so if ``v`` is something else, maybe we'd like to use a different builder. + auto conn_key = conntuple_mgr->GetBuilder().FromVal({zeek::NewRef{}, v}); - auto conn_key = conntuple_mgr->GetBuilder().NewConnKey(); - /** - // conn_key->LoadConnIdVal(v); - - This is IP specific stuff! - - if ( ! conn_key->Valid() ) { + if ( conn_key->Error() ) { // 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)); + zeek::emit_builtin_error(conn_key->Error()->c_str()); return nullptr; } - */ - return FindConnection(*conn_key); } Connection* Manager::FindConnection(const zeek::ConnKey& conn_key) { - detail::Key key{conn_key.SessionKey()}; + auto key = conn_key.SessionKey(); auto it = session_map.find(key); if ( it != session_map.end() ) diff --git a/src/session/Manager.h b/src/session/Manager.h index 8b209ae1cd..260cbf2ef6 100644 --- a/src/session/Manager.h +++ b/src/session/Manager.h @@ -93,7 +93,7 @@ public: size_t CurrentSessions() { return session_map.size(); } private: - using SessionMap = std::unordered_map; + using SessionMap = std::unordered_map; // Inserts a new connection into the sessions map. If a connection with // the same key already exists in the map, it will be overwritten by