Improve analysis of TCP SYN/SYN-ACK reversal situations.

- Since it's just the handshake packets out of order, they're no
  longer treated as partial connections, which some protocol analyzers
  immediately refuse to look at.

- The TCP_Reassembler "is_orig" state failed to change, which led to
  protocol analyzers sometimes using the wrong value for that.

- Add a unit test which exercises the Connection::FlipRoles() code
  path (i.e. the SYN/SYN-ACK reversal situation).

Addresses BIT-1148.

src/analyzer/protocol/pia/PIA.cc

@@ -331,11 +331,11 @@ void PIA_TCP::ActivateAnalyzer(analyzer::Tag tag, const Rule* rule)
 
 	tcp::TCP_Reassembler* reass_orig =
 		new tcp::TCP_Reassembler(this, tcp, tcp::TCP_Reassembler::Direct,
-					true, tcp->Orig());
+					tcp->Orig());
 
 	tcp::TCP_Reassembler* reass_resp =
 		new tcp::TCP_Reassembler(this, tcp, tcp::TCP_Reassembler::Direct,
-					false, tcp->Resp());
+					tcp->Resp());
 
 	int orig_seq = 0;
 	int resp_seq = 0;

src/analyzer/protocol/tcp/TCP.cc

@@ -95,9 +95,9 @@ void TCP_Analyzer::Done()
 void TCP_Analyzer::EnableReassembly()
 	{
 	SetReassembler(new TCP_Reassembler(this, this,
-					TCP_Reassembler::Forward, true, orig),
+	                                   TCP_Reassembler::Forward, orig),
 	               new TCP_Reassembler(this, this,
-					TCP_Reassembler::Forward, false, resp));
+	                                   TCP_Reassembler::Forward, resp));
 
 	reassembling = 1;
 
@@ -590,6 +590,7 @@ void TCP_Analyzer::UpdateInactiveState(double t,
 				// per the discussion in IsReuse.
 				// Flip the endpoints and establish
 				// the connection.
+				is_partial = 0;
 				Conn()->FlipRoles();
 				peer->SetState(TCP_ENDPOINT_ESTABLISHED);
 				}

src/analyzer/protocol/tcp/TCP_Reassembler.cc

@@ -32,13 +32,12 @@ static uint64 last_gap_bytes = 0;
 TCP_Reassembler::TCP_Reassembler(analyzer::Analyzer* arg_dst_analyzer,
 				TCP_Analyzer* arg_tcp_analyzer,
 				TCP_Reassembler::Type arg_type,
-				bool arg_is_orig, TCP_Endpoint* arg_endp)
+				TCP_Endpoint* arg_endp)
 	: Reassembler(1, REASSEM_TCP)
 	{
 	dst_analyzer = arg_dst_analyzer;
 	tcp_analyzer = arg_tcp_analyzer;
 	type = arg_type;
-	is_orig = arg_is_orig;
 	endp = arg_endp;
 	had_gap = false;
 	record_contents_file = 0;
@@ -165,10 +164,10 @@ void TCP_Reassembler::Undelivered(int up_to_seq)
 
 	if ( DEBUG_tcp_contents )
 		{
-		DEBUG_MSG("%.6f Undelivered: is_orig=%d up_to_seq=%d, last_reassm=%d, "
+		DEBUG_MSG("%.6f Undelivered: IsOrig()=%d up_to_seq=%d, last_reassm=%d, "
 		          "endp: FIN_cnt=%d, RST_cnt=%d, "
 		          "peer: FIN_cnt=%d, RST_cnt=%d\n",
-		          network_time, is_orig, up_to_seq, last_reassem_seq,
+		          network_time, IsOrig(), up_to_seq, last_reassem_seq,
 		          endpoint->FIN_cnt, endpoint->RST_cnt,
 		          peer->FIN_cnt, peer->RST_cnt);
 		}
@@ -196,9 +195,9 @@ void TCP_Reassembler::Undelivered(int up_to_seq)
 		{
 		if ( DEBUG_tcp_contents )
 			{
-			DEBUG_MSG("%.6f Undelivered: is_orig=%d, seq=%d, len=%d, "
+			DEBUG_MSG("%.6f Undelivered: IsOrig()=%d, seq=%d, len=%d, "
 					  "skip_deliveries=%d\n",
-					  network_time, is_orig, last_reassem_seq,
+					  network_time, IsOrig(), last_reassem_seq,
 					  seq_delta(up_to_seq, last_reassem_seq),
 					  skip_deliveries);
 			}
@@ -229,7 +228,7 @@ void TCP_Reassembler::Undelivered(int up_to_seq)
 				{
 				val_list* vl = new val_list;
 				vl->append(dst_analyzer->BuildConnVal());
-				vl->append(new Val(is_orig, TYPE_BOOL));
+				vl->append(new Val(IsOrig(), TYPE_BOOL));
 				vl->append(new Val(seq, TYPE_COUNT));
 				vl->append(new Val(len, TYPE_COUNT));
 
@@ -238,11 +237,11 @@ void TCP_Reassembler::Undelivered(int up_to_seq)
 
 			if ( type == Direct )
 				dst_analyzer->NextUndelivered(last_reassem_seq,
-								len, is_orig);
+								len, IsOrig());
 			else
 				{
 				dst_analyzer->ForwardUndelivered(last_reassem_seq,
-								len, is_orig);
+								len, IsOrig());
 				}
 			}
 
@@ -289,7 +288,7 @@ void TCP_Reassembler::MatchUndelivered(int up_to_seq)
 	for ( b = blocks; b && seq_delta(b->upper, last_reassem_seq) <= 0;
 	      b = b->next )
 	      tcp_analyzer->Conn()->Match(Rule::PAYLOAD, b->block, b->Size(),
-						false, false, is_orig, false);
+						false, false, IsOrig(), false);
 
 	ASSERT(b);
 	}
@@ -410,7 +409,7 @@ void TCP_Reassembler::BlockInserted(DataBlock* start_block)
 void TCP_Reassembler::Overlap(const u_char* b1, const u_char* b2, int n)
 	{
 	if ( DEBUG_tcp_contents )
-		DEBUG_MSG("%.6f TCP contents overlap: %d is_orig=%d\n", network_time,  n, is_orig);
+		DEBUG_MSG("%.6f TCP contents overlap: %d IsOrig()=%d\n", network_time,  n, IsOrig());
 
 	if ( rexmit_inconsistency &&
 	     memcmp((const void*) b1, (const void*) b2, n) &&
@@ -442,9 +441,9 @@ bool TCP_Reassembler::DoUnserialize(UnserialInfo* info)
 void TCP_Reassembler::Deliver(int seq, int len, const u_char* data)
 	{
 	if ( type == Direct )
-		dst_analyzer->NextStream(len, data, is_orig);
+		dst_analyzer->NextStream(len, data, IsOrig());
 	else
-		dst_analyzer->ForwardStream(len, data, is_orig);
+		dst_analyzer->ForwardStream(len, data, IsOrig());
 	}
 
 int TCP_Reassembler::DataSent(double t, int seq, int len,
@@ -455,8 +454,8 @@ int TCP_Reassembler::DataSent(double t, int seq, int len,
 
 	if ( DEBUG_tcp_contents )
 		{
-		DEBUG_MSG("%.6f DataSent: is_orig=%d seq=%d upper=%d ack=%d\n",
-		          network_time, is_orig, seq, upper_seq, ack);
+		DEBUG_MSG("%.6f DataSent: IsOrig()=%d seq=%d upper=%d ack=%d\n",
+		          network_time, IsOrig(), seq, upper_seq, ack);
 		}
 
 	if ( skip_deliveries )

src/analyzer/protocol/tcp/TCP_Reassembler.h

@@ -28,9 +28,8 @@ public:
 		Forward,	// forward to destination analyzer's children
 	};
 
-	TCP_Reassembler(Analyzer* arg_dst_analyzer,
-			TCP_Analyzer* arg_tcp_analyzer, Type arg_type,
-			bool arg_is_orig, TCP_Endpoint* arg_endp);
+	TCP_Reassembler(Analyzer* arg_dst_analyzer, TCP_Analyzer* arg_tcp_analyzer,
+	                Type arg_type, TCP_Endpoint* arg_endp);
 
 	virtual ~TCP_Reassembler();
 
@@ -135,7 +134,6 @@ private:
 	TCP_Analyzer* tcp_analyzer;
 
 	Type type;
-	bool is_orig;
 };
 
 } } // namespace analyzer::* 

testing/btest/Baseline/core.connection_flip_roles/out

@@ -0,0 +1,4 @@
+schedule_analyzer, current conn_id, [orig_h=192.150.187.43, orig_p=80/tcp, resp_h=141.142.228.5, resp_p=59856/tcp]
+http_request, 1.1, GET, /download/CHANGES.bro-aux.txt
+http_reply, 1.1, 200, OK
+connection_state_remove, [orig_h=141.142.228.5, orig_p=59856/tcp, resp_h=192.150.187.43, resp_p=80/tcp]

testing/btest/core/connection_flip_roles.bro

@@ -0,0 +1,39 @@
+# @TEST-EXEC: bro -b -r $TRACES/tcp/handshake-reorder.trace %INPUT >out
+# @TEST-EXEC: btest-diff out
+
+# This tests the Connection::FlipRoles code path (SYN/SYN-ACK reversal).
+
+# The check of likely_server_ports is before Connection::FlipRoles, so
+# need to make sure that isn't the mechanism used to flip src/dst stuff.
+redef likely_server_ports = {};
+
+global first_packet: bool = T;
+
+event new_packet(c: connection, p: pkt_hdr)
+	{
+	if ( ! first_packet )
+		return;
+
+	first_packet = F;
+
+	print "schedule_analyzer, current conn_id", c$id;
+	# Anticipate roles getting flipped in next packet.
+	Analyzer::schedule_analyzer(141.142.228.5, 192.150.187.43, 80/tcp,
+	                            Analyzer::ANALYZER_HTTP, 2mins);
+	}
+
+event connection_state_remove(c: connection)
+	{
+	print "connection_state_remove", c$id;
+	}
+
+event http_request(c: connection, method: string, original_URI: string,
+                   unescaped_URI: string, version: string)
+	{
+	print "http_request", version, method, original_URI;
+	}
+
+event http_reply(c: connection, version: string, code: count, reason: string)
+	{
+	print "http_reply", version, code, reason;
+	}