Merge remote-tracking branch 'origin/topic/robin/gh-2426-flipping'

* origin/topic/robin/gh-2426-flipping:
  Fixing productive connections with missing SYN still considered partial after flipping direction.
  Add some missing bits when flipping endpoints.

CHANGES

@@ -1,3 +1,24 @@
+5.2.0-dev.339 | 2022-11-18 11:49:47 +0100
+
+  * GH-2426: Fixing productive connections with missing SYN still considered partial after flipping direction. (Robin Sommer, Corelight)
+
+    In https://github.com/zeek/zeek/pull/2191, we added endpoint flipping
+    for cases where a connection starts with a SYN/ACK followed by ACK or
+    data. The goal was to treat the connection as productive and go ahead
+    and parse it. But the TCP analyzer could continue to consider it
+    partial after flipping, meaning that app layers would bail out. #2426
+    shows such a case: HTTP gets correctly activated after flipping
+    through content inspection, but it won't process anything because
+    `IsPartial()` returns true. As the is-partial state reflects
+    whether we saw the first packets each in direction, this patch now
+    overrides that state for the originally missing SYN after flipping.
+
+    We actually had the same problem at a couple of other locations already
+    as well. One of that only happened to work because of the originally
+    inconsistent state flipping that was fixed in the previous commit. The
+    corresponding unit test now broke after that change. This commit
+    updates that logic as well to override the state.
+
 5.2.0-dev.336 | 2022-11-17 16:28:40 -0800
 
   * Publish container images to ECR in addition to docker.io. (Benjamin Bannier, Corelight)

NEWS

@@ -138,6 +138,10 @@ Changed Functionality
   DPD ``max_violations`` script-level logic has a chance to run and disable
   the problematic analyzer.
 
+- The TCP analyzer now continues processing payload for some
+  connections missing initial packets where it would previously have
+  stopped. This fixes a few cases where we already had the logic to
+  continue in place, but we still ended up considering them partial.
 
 Deprecated Functionality
 ------------------------

VERSION

@@ -1 +1 @@
-5.2.0-dev.336
+5.2.0-dev.339

src/analyzer/Analyzer.h

@@ -862,6 +862,12 @@ public:
 	 */
 	void ForwardUndelivered(uint64_t seq, int len, bool orig) override;
 
+	/**
+	 * Signals that Zeek has flipped the direction of the connection, meaning
+	 * that originator and responder state need to be swapped.
+	 */
+	void FlipRoles() override { orig = ! orig; }
+
 protected:
 	friend class Analyzer;
 

src/analyzer/protocol/tcp/TCP_Endpoint.cc

@@ -275,9 +275,11 @@ bool TCP_Endpoint::CheckHistory(uint32_t mask, char code)
 		// since if those elicit anything, it should be a RST.
 		//
 		// Thus, at this stage we go ahead and flip the connection.
-		// We then fix up the history (which will initially be "H^").
+		// We then fix up the history (which will initially be "H^") and
+		// pretend we have actually seen that missing first packet.
 		conn->FlipRoles();
 		conn->ReplaceHistory("^h");
+		tcp_analyzer->SetFirstPacketSeen(true);
 		}
 
 	if ( ! IsOrig() )

src/packet_analysis/protocol/tcp/TCPSessionAdapter.cc

@@ -787,6 +787,11 @@ void TCPSessionAdapter::SetPartialStatus(analyzer::tcp::TCP_Flags flags, bool is
 		}
 	}
 
+void TCPSessionAdapter::SetFirstPacketSeen(bool is_orig)
+	{
+	first_packet_seen |= (is_orig ? ORIG : RESP);
+	}
+
 void TCPSessionAdapter::UpdateInactiveState(double t, analyzer::tcp::TCP_Endpoint* endpoint,
                                             analyzer::tcp::TCP_Endpoint* peer, uint32_t base_seq,
                                             uint32_t ack_seq, int len, bool is_orig,
@@ -829,6 +834,7 @@ void TCPSessionAdapter::UpdateInactiveState(double t, analyzer::tcp::TCP_Endpoin
 				is_partial = 0;
 				Conn()->FlipRoles();
 				peer->SetState(analyzer::tcp::TCP_ENDPOINT_ESTABLISHED);
+				SetFirstPacketSeen(true);
 				}
 
 			else
@@ -913,6 +919,7 @@ void TCPSessionAdapter::UpdateInactiveState(double t, analyzer::tcp::TCP_Endpoin
 			// as partial and instead establish the connection.
 			endpoint->SetState(analyzer::tcp::TCP_ENDPOINT_ESTABLISHED);
 			is_partial = 0;
+			SetFirstPacketSeen(is_orig);
 			}
 
 		else
@@ -1162,6 +1169,9 @@ void TCPSessionAdapter::FlipRoles()
 	orig = tmp_ep;
 	orig->is_orig = ! orig->is_orig;
 	resp->is_orig = ! resp->is_orig;
+	first_packet_seen = ((first_packet_seen & ORIG) ? RESP : 0) |
+	                    ((first_packet_seen & RESP) ? ORIG : 0);
+	is_partial = 0; // resetting, it may be re-established later
 	}
 
 void TCPSessionAdapter::UpdateConnVal(RecordVal* conn_val)

src/packet_analysis/protocol/tcp/TCPSessionAdapter.h

@@ -80,6 +80,7 @@ public:
 
 protected:
 	friend class analyzer::tcp::TCP_ApplicationAnalyzer;
+	friend class analyzer::tcp::TCP_Endpoint;
 	friend class analyzer::tcp::TCP_Reassembler;
 	friend class analyzer::pia::PIA_TCP;
 	friend class packet_analysis::TCP::TCPAnalyzer;
@@ -95,6 +96,7 @@ protected:
 	bool IsReuse(double t, const u_char* pkt) override;
 
 	void SetPartialStatus(analyzer::tcp::TCP_Flags flags, bool is_orig);
+	void SetFirstPacketSeen(bool is_orig);
 
 	// Update the state machine of the TCPs based on the activity.  This
 	// includes our pseudo-states such as TCP_ENDPOINT_PARTIAL.

testing/btest/Baseline/core.tcp.flip-without-syn/conn.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	conn
+#open XXXX-XX-XX-XX-XX-XX
+#fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	proto	service	duration	orig_bytes	resp_bytes	conn_state	local_orig	local_resp	missed_bytes	history	orig_pkts	orig_ip_bytes	resp_pkts	resp_ip_bytes	tunnel_parents
+#types	time	string	addr	port	addr	port	enum	string	interval	count	count	string	bool	bool	count	string	count	count	count	count	set[string]
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	141.142.228.5	6669	192.150.187.43	80	tcp	http	0.141744	136	5007	SF	-	-	0	^hADadFf	6	456	7	5371	-
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/core/tcp/flip-without-syn.zeek

@@ -0,0 +1,4 @@
+# @TEST-EXEC: zeek -r $TRACES/tcp/http-on-irc-port-missing-syn.pcap %INPUT
+# @TEST-EXEC: btest-diff conn.log
+
+# @TEST-DOC: Regression test for #2191: missing SYN and misleading well-known port. Zeek flips, and DPD still figures out the protocol ok.