Merge remote-tracking branch 'origin/topic/timw/4281-unconditionally-forward-out-of-snap'

* origin/topic/timw/4281-unconditionally-forward-out-of-snap:
  Make SNAP analyzer use both OUI and protocol for forwarding
  Change packet analyzer identifiers to be 64-bit

src/packet_analysis/Analyzer.cc

@@ -53,12 +53,12 @@ bool Analyzer::IsAnalyzer(const char* name) {
     return packet_mgr->GetComponentName(tag) == name;
 }
 
-const AnalyzerPtr& Analyzer::Lookup(uint32_t identifier) const { return dispatcher.Lookup(identifier); }
+const AnalyzerPtr& Analyzer::Lookup(uint64_t identifier) const { return dispatcher.Lookup(identifier); }
 
 // Find the next inner analyzer using identifier or via DetectProtocol(),
 // otherwise return the default analyzer.
 const AnalyzerPtr& Analyzer::FindInnerAnalyzer(size_t len, const uint8_t* data, Packet* packet,
-                                               uint32_t identifier) const {
+                                               uint64_t identifier) const {
     const auto& identifier_based_analyzer = Lookup(identifier);
     if ( identifier_based_analyzer )
         return identifier_based_analyzer;
@@ -92,11 +92,11 @@ const AnalyzerPtr& Analyzer::DetectInnerAnalyzer(size_t len, const uint8_t* data
     return nil;
 }
 
-bool Analyzer::ForwardPacket(size_t len, const uint8_t* data, Packet* packet, uint32_t identifier) const {
+bool Analyzer::ForwardPacket(size_t len, const uint8_t* data, Packet* packet, uint64_t identifier) const {
     const auto& inner_analyzer = FindInnerAnalyzer(len, data, packet, identifier);
 
     if ( ! inner_analyzer ) {
-        DBG_LOG(DBG_PACKET_ANALYSIS, "Analysis in %s failed, could not find analyzer for identifier %#x.",
+        DBG_LOG(DBG_PACKET_ANALYSIS, "Analysis in %s failed, could not find analyzer for identifier %#" PRIx64 ".",
                 GetAnalyzerName(), identifier);
 
         if ( report_unknown_protocols )
@@ -106,12 +106,12 @@ bool Analyzer::ForwardPacket(size_t len, const uint8_t* data, Packet* packet, ui
     }
 
     if ( ! inner_analyzer->IsEnabled() ) {
-        DBG_LOG(DBG_PACKET_ANALYSIS, "Analysis in %s found disabled next layer analyzer %s for identifier %#x",
+        DBG_LOG(DBG_PACKET_ANALYSIS, "Analysis in %s found disabled next layer analyzer %s for identifier %#" PRIx64,
                 GetAnalyzerName(), inner_analyzer->GetAnalyzerName(), identifier);
         return false;
     }
 
-    DBG_LOG(DBG_PACKET_ANALYSIS, "Analysis in %s succeeded, next layer identifier is %#x.", GetAnalyzerName(),
+    DBG_LOG(DBG_PACKET_ANALYSIS, "Analysis in %s succeeded, next layer identifier is %#" PRIx64 ".", GetAnalyzerName(),
             identifier);
 
     packet_mgr->TrackAnalyzer(inner_analyzer.get(), len, data);
@@ -141,7 +141,7 @@ void Analyzer::DumpDebug() const {
 #endif
 }
 
-void Analyzer::RegisterProtocol(uint32_t identifier, AnalyzerPtr child) {
+void Analyzer::RegisterProtocol(uint64_t identifier, AnalyzerPtr child) {
     if ( run_state::detail::zeek_init_done )
         reporter->FatalError("Packet protocols cannot be registered after zeek_init has finished.");
 

src/packet_analysis/Analyzer.h

@@ -108,7 +108,7 @@ public:
      * @param child The analyzer that will be called for the new protocol during
      * forwarding.
      */
-    void RegisterProtocol(uint32_t identifier, AnalyzerPtr child);
+    void RegisterProtocol(uint64_t identifier, AnalyzerPtr child);
 
     /**
      * Registers an analyzer to use for protocol detection if identifier
@@ -202,7 +202,7 @@ protected:
      * @return The analyzer registered for the given identifier. Returns a
      * nullptr if no analyzer is registered.
      */
-    const AnalyzerPtr& Lookup(uint32_t identifier) const;
+    const AnalyzerPtr& Lookup(uint64_t identifier) const;
 
     /**
      * Returns an analyzer based on a script-land definition.
@@ -238,7 +238,7 @@ protected:
      *
      * @return false if the analysis failed, else true.
      */
-    bool ForwardPacket(size_t len, const uint8_t* data, Packet* packet, uint32_t identifier) const;
+    bool ForwardPacket(size_t len, const uint8_t* data, Packet* packet, uint64_t identifier) const;
 
     /**
      * Triggers default analysis of the encapsulated packet if the default analyzer
@@ -266,7 +266,7 @@ private:
                                       const zeek::Tag& arg_tag);
 
     // Internal helpers to find an appropriate next inner analyzer.
-    const AnalyzerPtr& FindInnerAnalyzer(size_t len, const uint8_t* data, Packet* packet, uint32_t identifier) const;
+    const AnalyzerPtr& FindInnerAnalyzer(size_t len, const uint8_t* data, Packet* packet, uint64_t identifier) const;
     const AnalyzerPtr& FindInnerAnalyzer(size_t len, const uint8_t* data, Packet* packet) const;
     const AnalyzerPtr& DetectInnerAnalyzer(size_t len, const uint8_t* data, Packet* packet) const;
 

src/packet_analysis/Dispatcher.cc

@@ -12,7 +12,7 @@ namespace zeek::packet_analysis::detail {
 
 Dispatcher::~Dispatcher() { FreeValues(); }
 
-void Dispatcher::Register(uint32_t identifier, AnalyzerPtr analyzer) {
+void Dispatcher::Register(uint64_t identifier, AnalyzerPtr analyzer) {
     // If the table has size 1 and the entry is nullptr, there was nothing added yet. Just add it.
     if ( table.size() == 1 && table[0] == nullptr ) {
         table[0] = std::move(analyzer);
@@ -27,7 +27,7 @@ void Dispatcher::Register(uint32_t identifier, AnalyzerPtr analyzer) {
     }
     else if ( identifier < lowest_identifier ) {
         // Lower than the lowest registered identifier. Shift up by lowerBound - identifier
-        uint32_t distance = lowest_identifier - identifier;
+        uint64_t distance = lowest_identifier - identifier;
         table.resize(table.size() + distance, nullptr);
 
         // Shift values
@@ -48,7 +48,7 @@ void Dispatcher::Register(uint32_t identifier, AnalyzerPtr analyzer) {
     table[index] = std::move(analyzer);
 }
 
-const AnalyzerPtr& Dispatcher::Lookup(uint32_t identifier) const {
+const AnalyzerPtr& Dispatcher::Lookup(uint64_t identifier) const {
     int64_t index = identifier - lowest_identifier;
     if ( index >= 0 && index < static_cast<int64_t>(table.size()) )
         return table[index];
@@ -75,7 +75,7 @@ void Dispatcher::DumpDebug() const {
     DBG_LOG(DBG_PACKET_ANALYSIS, "Dispatcher elements (used/total): %lu/%lu", Count(), table.size());
     for ( size_t i = 0; i < table.size(); i++ ) {
         if ( table[i] != nullptr )
-            DBG_LOG(DBG_PACKET_ANALYSIS, "%#8lx => %s", i + lowest_identifier, table[i]->GetAnalyzerName());
+            DBG_LOG(DBG_PACKET_ANALYSIS, "%#8" PRIx64 " => %s", i + lowest_identifier, table[i]->GetAnalyzerName());
     }
 #endif
 }

src/packet_analysis/Dispatcher.h

@@ -28,7 +28,7 @@ public:
      * @param identifier The identifier.
      * @param analyzer The analyzer to register.
      */
-    void Register(uint32_t identifier, AnalyzerPtr analyzer);
+    void Register(uint64_t identifier, AnalyzerPtr analyzer);
 
     /**
      * Looks up the analyzer for an identifier.
@@ -37,7 +37,7 @@ public:
      * @return The analyzer registered for the given identifier. Returns a
      * nullptr if no analyzer is registered.
      */
-    const AnalyzerPtr& Lookup(uint32_t identifier) const;
+    const AnalyzerPtr& Lookup(uint64_t identifier) const;
 
     /**
      * Returns the number of registered analyzers.
@@ -56,12 +56,12 @@ public:
     void DumpDebug() const;
 
 private:
-    uint32_t lowest_identifier = 0;
+    uint64_t lowest_identifier = 0;
     std::vector<AnalyzerPtr> table;
 
     void FreeValues();
 
-    inline uint32_t GetHighestIdentifier() const { return lowest_identifier + table.size() - 1; }
+    inline uint64_t GetHighestIdentifier() const { return lowest_identifier + table.size() - 1; }
 };
 
 } // namespace detail

src/packet_analysis/protocol/snap/SNAP.cc

@@ -36,11 +36,11 @@ bool SNAPAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* packet
     data += 5;
     len -= 5;
 
-    if ( oui == 0 ) {
-        // If the OUI is zero, the protocol is a standard ethertype and can be
-        // forwarded as such.
-        return ForwardPacket(len, data, packet, protocol);
-    }
+    // Protocol values for SNAP can differ based what OUI publishes them, so use a
+    // combination of them for the identifier used to forward.
+    int64_t identifier = oui;
+    identifier <<= 16;
+    identifier |= protocol;
 
-    return true;
+    return ForwardPacket(len, data, packet, identifier);
 }

testing/btest/Baseline/scripts.base.protocols.snap.snap-cdp/unknown_protocols.log

@@ -0,0 +1,11 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+#separator \x09
+#set_separator	,
+#empty_field	(empty)
+#unset_field	-
+#path	unknown_protocols
+#open XXXX-XX-XX-XX-XX-XX
+#fields	ts	analyzer	protocol_id	first_bytes	analyzer_history
+#types	time	string	string	string	vector[string]
+XXXXXXXXXX.XXXXXX	SNAP	0xc2000	01b4dff0000100065231	ETHERNET,SNAP
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Traces/README

@@ -48,3 +48,5 @@ Trace Index/Sources:
   https://zeekorg.slack.com/archives/CSZBXF6TH/p1738261449655049
 - tunnels/geneve-tagged-udp-packet.pcap
   Provided by Eldon Koyle Corelight for testing.
+- cdp-v1.pcap
+  From the Wireshark library of captures at https://wiki.wireshark.org/samplecaptures.

testing/btest/scripts/base/protocols/snap/snap-cdp.test

@@ -0,0 +1,4 @@
+# @TEST-EXEC: zeek -r $TRACES/cdp-v1.pcap %INPUT
+# @TEST-EXEC: btest-diff unknown_protocols.log
+
+@load policy/misc/unknown-protocols