Rework the packet flow through the IP-based analyzers

src/packet_analysis/protocol/icmp/ICMP.cc

@@ -35,33 +35,31 @@ ICMPAnalyzer::~ICMPAnalyzer()
 	{
 	}
 
-bool ICMPAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* packet)
+bool ICMPAnalyzer::BuildConnTuple(size_t len, const uint8_t* data, Packet* packet,
+                                  ConnTuple& tuple)
 	{
 	if ( ! CheckHeaderTrunc(ICMP_MINLEN, len, packet) )
 		return false;
 
-	ConnTuple id;
+	tuple.src_addr = packet->ip_hdr->SrcAddr();
-	id.src_addr = packet->ip_hdr->SrcAddr();
+	tuple.dst_addr = packet->ip_hdr->DstAddr();
-	id.dst_addr = packet->ip_hdr->DstAddr();
+	tuple.proto = TRANSPORT_ICMP;
-	id.proto = TRANSPORT_ICMP;
 
 	const struct icmp* icmpp = (const struct icmp *) data;
-	id.src_port = htons(icmpp->icmp_type);
+	tuple.src_port = htons(icmpp->icmp_type);
 
 	if ( packet->proto == IPPROTO_ICMP )
-		id.dst_port = htons(ICMP4_counterpart(icmpp->icmp_type, icmpp->icmp_code, id.is_one_way));
+		tuple.dst_port = htons(ICMP4_counterpart(icmpp->icmp_type, icmpp->icmp_code, tuple.is_one_way));
 	else if ( packet->proto == IPPROTO_ICMPV6 )
-		id.dst_port = htons(ICMP6_counterpart(icmpp->icmp_type, icmpp->icmp_code, id.is_one_way));
+		tuple.dst_port = htons(ICMP6_counterpart(icmpp->icmp_type, icmpp->icmp_code, tuple.is_one_way));
 	else
 		reporter->InternalError("Reached ICMP packet analyzer with unknown packet protocol %x",
 		                        packet->proto);
 
-	ProcessConnection(id, packet, len);
-
 	return true;
 	}
 
-void ICMPAnalyzer::ContinueProcessing(Connection* c, double t, bool is_orig, int remaining, Packet* pkt)
+void ICMPAnalyzer::DeliverPacket(Connection* c, double t, bool is_orig, int remaining, Packet* pkt)
 	{
 	auto* ta = static_cast<ICMPTransportAnalyzer*>(c->GetRootAnalyzer());
 

src/packet_analysis/protocol/icmp/ICMP.h

@@ -24,8 +24,6 @@ public:
 	ICMPAnalyzer();
 	~ICMPAnalyzer() override;
 
-	bool AnalyzePacket(size_t len, const uint8_t* data, Packet* packet) override;
-
 	static zeek::packet_analysis::AnalyzerPtr Instantiate()
 		{
 		return std::make_shared<ICMPAnalyzer>();
@@ -36,7 +34,13 @@ public:
 
 protected:
 
-	void ContinueProcessing(Connection* c, double t, bool is_orig, int remaining,
+	/**
+	 * Parse the header from the packet into a ConnTuple object.
+	 */
+	bool BuildConnTuple(size_t len, const uint8_t* data, Packet* packet,
+	                    ConnTuple& tuple) override;
+
+	void DeliverPacket(Connection* c, double t, bool is_orig, int remaining,
 	                        Packet* pkt) override;
 
 private:

src/packet_analysis/protocol/ip/IPBasedAnalyzer.cc

@@ -22,16 +22,20 @@ IPBasedAnalyzer::~IPBasedAnalyzer()
 	{
 	}
 
-void IPBasedAnalyzer::ProcessConnection(const ConnTuple& conn_id, Packet* pkt, size_t remaining)
+bool IPBasedAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* pkt)
 	{
+	ConnTuple tuple;
+	if ( ! BuildConnTuple(len, data, pkt, tuple) )
+		return false;
+
 	const std::unique_ptr<IP_Hdr>& ip_hdr = pkt->ip_hdr;
-	detail::ConnKey key(conn_id);
+	detail::ConnKey key(tuple);
 
 	Connection* conn = session_mgr->FindConnection(key);
 
 	if ( ! conn )
 		{
-		conn = NewConn(&conn_id, key, pkt);
+		conn = NewConn(&tuple, key, pkt);
 		if ( conn )
 			session_mgr->Insert(conn, false);
 		}
@@ -42,7 +46,7 @@ void IPBasedAnalyzer::ProcessConnection(const ConnTuple& conn_id, Packet* pkt, s
 			conn->Event(connection_reused, nullptr);
 
 			session_mgr->Remove(conn);
-			conn = NewConn(&conn_id, key, pkt);
+			conn = NewConn(&tuple, key, pkt);
 			if ( conn )
 				session_mgr->Insert(conn, false);
 			}
@@ -53,10 +57,10 @@ void IPBasedAnalyzer::ProcessConnection(const ConnTuple& conn_id, Packet* pkt, s
 		}
 
 	if ( ! conn )
-		return;
+		return false;
 
-	bool is_orig = (conn_id.src_addr == conn->OrigAddr()) &&
+	bool is_orig = (tuple.src_addr == conn->OrigAddr()) &&
-		(conn_id.src_port == conn->OrigPort());
+	               (tuple.src_port == conn->OrigPort());
 
 	conn->CheckFlowLabel(is_orig, ip_hdr->FlowLabel());
 
@@ -85,9 +89,9 @@ void IPBasedAnalyzer::ProcessConnection(const ConnTuple& conn_id, Packet* pkt, s
 
 		// TODO: Does this actually mean anything?
 		if ( conn->Skipping() )
-			return;
+			return true;
 
-		ContinueProcessing(conn, run_state::processing_start_time, is_orig, remaining, pkt);
+		DeliverPacket(conn, run_state::processing_start_time, is_orig, len, pkt);
 
 		run_state::current_timestamp = 0;
 		run_state::current_pkt = nullptr;
@@ -114,7 +118,7 @@ void IPBasedAnalyzer::ProcessConnection(const ConnTuple& conn_id, Packet* pkt, s
 		const u_char* data = pkt->ip_hdr->Payload();
 
 		conn->NextPacket(run_state::processing_start_time, is_orig, ip_hdr.get(), ip_hdr->PayloadLen(),
-		                 remaining, data, record_packet, record_content, pkt);
+		                 len, data, record_packet, record_content, pkt);
 
 		// If the packet is reassembled, disable packet dumping because the
 		// pointer math to dump the data wouldn't work.
@@ -130,6 +134,8 @@ void IPBasedAnalyzer::ProcessConnection(const ConnTuple& conn_id, Packet* pkt, s
 				pkt->dump_size = data - pkt->data;
 			}
 		}
+
+	return true;
 	}
 
 bool IPBasedAnalyzer::CheckHeaderTrunc(size_t min_hdr_len, size_t remaining, Packet* packet)

src/packet_analysis/protocol/ip/IPBasedAnalyzer.h

@@ -22,6 +22,8 @@ class IPBasedAnalyzer : public Analyzer {
 public:
 	~IPBasedAnalyzer() override;
 
+	bool AnalyzePacket(size_t len, const uint8_t* data, Packet* packet) override;
+
 	/**
 	 * Returns true if the analyzer determines that in fact a new
 	 * connection has started without the connection statement having
@@ -56,26 +58,24 @@ protected:
 	                bool report_unknown_protocols);
 
 	/**
-	 * Entry point for child classes to call to do the actual heavy lifting for
+	 * Parse the header from the packet into a ConnTuple object.
-	 * processing a packet and extracting a connection out of it.
-	 *
-	 * @param conn_id The connection ID generated by the child class.
-	 * @param pkt The packet being processed.
-	 * @param remaining The number of bytes remaining to be processed in the packet.
 	 */
-	void ProcessConnection(const ConnTuple& conn_id, Packet* pkt, size_t remaining);
+	virtual bool BuildConnTuple(size_t len, const uint8_t* data, Packet* packet,
+	                            ConnTuple& tuple) = 0;
 
 	/**
-	 * Verifies that there is enough data in the packet to process the header
+	 * Continues process of packet after the connection has been inserted into the
-	 * length requested.
+	 * session manager. This should be implemented by all child classes.
 	 *
-	 * @param min_hdr_len The minimum data in bytes that needs to exist.
+	 * @param conn The connection currently being processed.
-	 * @param remaining The remaining number of bytes in the packet reported by
+	 * @param t The timestamp for the current packet.
-	 * previous analyzer.
+	 * @param is_orig Flag denoting whether this packet is from the originator of
-	 * @param packet The packet being processed. This will be used to pull out the
+	 * the connection.
-	 * number of bytes the IP header says we have remaining.
+	 * @param remaining The remaining about of data in the packet.
+	 * @param pkt The packet being processed.
 	 */
-	bool CheckHeaderTrunc(size_t min_hdr_len, size_t remaining, Packet* packet);
+	virtual void DeliverPacket(Connection* conn, double t, bool is_orig, int remaining,
+	                           Packet* pkt) {}
 
 	/**
 	 * Upon seeing the first packet of a connection, checks whether we want
@@ -96,6 +96,18 @@ protected:
 		return true;
 		}
 
+	/**
+	 * Verifies that there is enough data in the packet to process the header
+	 * length requested.
+	 *
+	 * @param min_hdr_len The minimum data in bytes that needs to exist.
+	 * @param remaining The remaining number of bytes in the packet reported by
+	 * previous analyzer.
+	 * @param packet The packet being processed. This will be used to pull out the
+	 * number of bytes the IP header says we have remaining.
+	 */
+	bool CheckHeaderTrunc(size_t min_hdr_len, size_t remaining, Packet* packet);
+
 	/**
 	 * Returns true if the port corresponds to an application for which there
 	 * is a Zeek analyzer (even if it might not be used by the present policy
@@ -105,19 +117,6 @@ protected:
 	 */
 	bool IsLikelyServerPort(uint32_t port) const;
 
-	/**
-	 * Continues process of packet after the connection has been inserted into the
-	 * session manager. This should be implemented by all child classes.
-	 *
-	 * @param conn The connection currently being processed.
-	 * @param t The timestamp for the current packet.
-	 * @param is_orig Flag denoting whether this packet is from the originator of
-	 * the connection.
-	 * @param remaining The remaining about of data in the packet.
-	 * @param pkt The packet being processed.
-	 */
-	virtual void ContinueProcessing(Connection* conn, double t, bool is_orig, int remaining,
-	                                Packet* pkt) {}
 
 	// TODO: temporary, until all of the plugins are implemented
 	bool new_plugin = false;

src/packet_analysis/protocol/tcp/TCP.cc

@@ -14,25 +14,23 @@ TCPAnalyzer::~TCPAnalyzer()
 	{
 	}
 
-bool TCPAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* packet)
+bool TCPAnalyzer::BuildConnTuple(size_t len, const uint8_t* data, Packet* packet,
+                                 ConnTuple& tuple)
 	{
 	uint32_t min_hdr_len = sizeof(struct tcphdr);
 	if ( ! CheckHeaderTrunc(min_hdr_len, len, packet) )
 		return false;
 
-	ConnTuple id;
+	tuple.src_addr = packet->ip_hdr->SrcAddr();
-	id.src_addr = packet->ip_hdr->SrcAddr();
+	tuple.dst_addr = packet->ip_hdr->DstAddr();
-	id.dst_addr = packet->ip_hdr->DstAddr();
 
 	data = packet->ip_hdr->Payload();
 
 	const struct tcphdr* tp = (const struct tcphdr *) data;
-	id.src_port = tp->th_sport;
+	tuple.src_port = tp->th_sport;
-	id.dst_port = tp->th_dport;
+	tuple.dst_port = tp->th_dport;
-	id.is_one_way = false;
+	tuple.is_one_way = false;
-	id.proto = TRANSPORT_TCP;
+	tuple.proto = TRANSPORT_TCP;
-
-	ProcessConnection(id, packet, len);
 
 	return true;
 	}

src/packet_analysis/protocol/tcp/TCP.h

@@ -13,8 +13,6 @@ public:
 	TCPAnalyzer();
 	~TCPAnalyzer() override;
 
-	bool AnalyzePacket(size_t len, const uint8_t* data, Packet* packet) override;
-
 	static zeek::packet_analysis::AnalyzerPtr Instantiate()
 		{
 		return std::make_shared<TCPAnalyzer>();
@@ -25,6 +23,12 @@ public:
 
 protected:
 
+	/**
+	 * Parse the header from the packet into a ConnTuple object.
+	 */
+	bool BuildConnTuple(size_t len, const uint8_t* data, Packet* packet,
+	                    ConnTuple& tuple) override;
+
 	/**
 	 * Upon seeing the first packet of a connection, checks whether we want
 	 * to analyze it (e.g. we may not want to look at partial connections)

src/packet_analysis/protocol/udp/UDP.cc

@@ -33,26 +33,6 @@ UDPAnalyzer::~UDPAnalyzer()
 	{
 	}
 
-bool UDPAnalyzer::AnalyzePacket(size_t len, const uint8_t* data, Packet* packet)
-	{
-	uint32_t min_hdr_len = sizeof(struct udphdr);
-	if ( ! CheckHeaderTrunc(min_hdr_len, len, packet) )
-		return false;
-
-	ConnTuple id;
-	id.src_addr = packet->ip_hdr->SrcAddr();
-	id.dst_addr = packet->ip_hdr->DstAddr();
-	const struct udphdr* up = (const struct udphdr *) packet->ip_hdr->Payload();
-	id.src_port = up->uh_sport;
-	id.dst_port = up->uh_dport;
-	id.is_one_way = false;
-	id.proto = TRANSPORT_UDP;
-
-	ProcessConnection(id, packet, len);
-
-	return true;
-	}
-
 void UDPAnalyzer::CreateTransportAnalyzer(Connection* conn, IPBasedTransportAnalyzer*& root,
                                           analyzer::pia::PIA*& pia, bool& check_port)
 	{
@@ -89,7 +69,26 @@ bool UDPAnalyzer::WantConnection(uint16_t src_port, uint16_t dst_port,
 	return true;
 	}
 
-void UDPAnalyzer::ContinueProcessing(Connection* c, double t, bool is_orig, int remaining, Packet* pkt)
+bool UDPAnalyzer::BuildConnTuple(size_t len, const uint8_t* data, Packet* packet,
+                                 ConnTuple& tuple)
+	{
+	uint32_t min_hdr_len = sizeof(struct udphdr);
+	if ( ! CheckHeaderTrunc(min_hdr_len, len, packet) )
+		return false;
+
+	tuple.src_addr = packet->ip_hdr->SrcAddr();
+	tuple.dst_addr = packet->ip_hdr->DstAddr();
+
+	const struct udphdr* up = (const struct udphdr *) packet->ip_hdr->Payload();
+	tuple.src_port = up->uh_sport;
+	tuple.dst_port = up->uh_dport;
+	tuple.is_one_way = false;
+	tuple.proto = TRANSPORT_UDP;
+
+	return true;
+	}
+
+void UDPAnalyzer::DeliverPacket(Connection* c, double t, bool is_orig, int remaining, Packet* pkt)
 	{
 	conn = c;
 

src/packet_analysis/protocol/udp/UDP.h

@@ -13,8 +13,6 @@ public:
 	UDPAnalyzer();
 	~UDPAnalyzer() override;
 
-	bool AnalyzePacket(size_t len, const uint8_t* data, Packet* packet) override;
-
 	static zeek::packet_analysis::AnalyzerPtr Instantiate()
 		{
 		return std::make_shared<UDPAnalyzer>();
@@ -32,6 +30,15 @@ public:
 
 protected:
 
+	/**
+	 * Parse the header from the packet into a ConnTuple object.
+	 */
+	bool BuildConnTuple(size_t len, const uint8_t* data, Packet* packet,
+	                    ConnTuple& tuple) override;
+
+	void DeliverPacket(Connection* c, double t, bool is_orig, int remaining,
+	                        Packet* pkt) override;
+
 	/**
 	 * Upon seeing the first packet of a connection, checks whether we want
 	 * to analyze it (e.g. we may not want to look at partial connections)
@@ -47,9 +54,6 @@ protected:
 	bool WantConnection(uint16_t src_port, uint16_t dst_port,
 	                    const u_char* data, bool& flip_roles) const override;
 
-	void ContinueProcessing(Connection* c, double t, bool is_orig, int remaining,
-	                        Packet* pkt) override;
-
 private:
 
 	// Returns true if the checksum is valid, false if not