Merge remote-tracking branch 'origin/topic/jsiwek/gh-566-fix-ssh-encrypted-packet'

* origin/topic/jsiwek/gh-566-fix-ssh-encrypted-packet:
  GH-566: fix cases where ssh_encrypted_packet event wasn't raised

CHANGES

@@ -1,4 +1,12 @@
 
+3.1.0-dev.118 | 2019-09-17 17:21:58 +0000
+
+  * GH-566: Fix cases where ssh_encrypted_packet event wasn't raised.
+    When encrypted data was bundled within the same segment as the
+    NewKeys message, it wasn't not reported via a
+    ssh_encrypted_package event as it should have been. (Jon Siwek,
+    Corelight)
+
 3.1.0-dev.116 | 2019-09-17 10:08:38 -0700
 
   * Switch from header guards to pragma once (Dominik Charousset, Corelight)

VERSION

@@ -1 +1 @@
-3.1.0-dev.116
+3.1.0-dev.118

src/analyzer/protocol/ssh/SSH.cc

@@ -18,6 +18,7 @@ SSH_Analyzer::SSH_Analyzer(Connection* c)
 	had_gap = false;
 	auth_decision_made = false;
 	skipped_banner = false;
+	saw_encrypted_client_data = false;
 	service_accept_size = 0;
 	userauth_failure_size = 0;
 	}
@@ -56,16 +57,12 @@ void SSH_Analyzer::DeliverStream(int len, const u_char* data, bool orig)
 
 	if ( interp->get_state(orig) == binpac::SSH::ENCRYPTED )
 		{
-		if ( ssh_encrypted_packet )
+		ProcessEncryptedSegment(len, orig);
-			BifEvent::generate_ssh_encrypted_packet(interp->bro_analyzer(), interp->bro_analyzer()->Conn(),
-				orig, len);
-
-		if ( ! auth_decision_made )
-			ProcessEncrypted(len, orig);
-
 		return;
 		}
 
+	interp->clear_encrypted_byte_count_in_current_segment();
+
 	try
 		{
 		interp->NewData(orig, data, data + len);
@@ -74,6 +71,14 @@ void SSH_Analyzer::DeliverStream(int len, const u_char* data, bool orig)
 		{
 		ProtocolViolation(fmt("Binpac exception: %s", e.c_msg()));
 		}
+
+	auto encrypted_len = interp->get_encrypted_bytes_in_current_segment();
+
+	if ( encrypted_len > 0 )
+		// We must have transitioned into the encrypted state during this
+		// delivery, but also had some portion of the segment be comprised
+		// of encrypted data, so process the encrypted segment length.
+		ProcessEncryptedSegment(encrypted_len, orig);
 	}
 
 void SSH_Analyzer::Undelivered(uint64_t seq, int len, bool orig)
@@ -83,11 +88,31 @@ void SSH_Analyzer::Undelivered(uint64_t seq, int len, bool orig)
 	interp->NewGap(orig, len);
 	}
 
+void SSH_Analyzer::ProcessEncryptedSegment(int len, bool orig)
+	{
+	if ( ssh_encrypted_packet )
+		BifEvent::generate_ssh_encrypted_packet(interp->bro_analyzer(),
+		                                        interp->bro_analyzer()->Conn(),
+		                                        orig, len);
+
+	if ( ! auth_decision_made )
+		ProcessEncrypted(len, orig);
+	}
+
 void SSH_Analyzer::ProcessEncrypted(int len, bool orig)
 	{
-	// We're interested in messages from the server for SSH2
+	if ( interp->get_version() != binpac::SSH::SSH2 )
-	if ( ! orig && (interp->get_version() == binpac::SSH::SSH2) )
+		return;
+
+	if ( orig )
+		saw_encrypted_client_data = true;
+	else
 		{
+		// If the client hasn't sent any encrypted data yet, but the
+		// server is, just ignore it until seeing encrypted client data.
+		if ( ! saw_encrypted_client_data )
+			return;
+
 		// The first thing we see and want to know is the length of
 		// SSH_MSG_SERVICE_REQUEST, which has a fixed (decrypted) size
 		// of 24 bytes (17 for content pad-aligned to 8-byte

src/analyzer/protocol/ssh/SSH.h

@@ -30,12 +30,14 @@ namespace analyzer {
 			binpac::SSH::SSH_Conn* interp;
 
 			void ProcessEncrypted(int len, bool orig);
+			void ProcessEncryptedSegment(int len, bool orig);
 
 			bool had_gap;
 
 			// Packet analysis stuff
 			bool auth_decision_made;
 			bool skipped_banner;
+			bool saw_encrypted_client_data;
 
 			int service_accept_size;
 			int userauth_failure_size;

src/analyzer/protocol/ssh/ssh-protocol.pac

@@ -9,9 +9,15 @@
 #  - Encrypted messages have no usable data, so we'll just ignore them as best we can.
 #  - Finally, key exchange messages have a common format.
 
+type EncryptedByte(is_orig: bool) = record {
+	encrypted : bytestring &length=1 &transient;
+} &let {
+proc: bool = $context.connection.inc_encrypted_byte_count_in_current_segment();
+};
+
 type SSH_PDU(is_orig: bool) = case $context.connection.get_state(is_orig) of {
 	VERSION_EXCHANGE -> version   : SSH_Version(is_orig);
-	ENCRYPTED        -> encrypted : bytestring &length=1 &transient;
+	ENCRYPTED        -> encrypted : EncryptedByte(is_orig);
 	default          -> kex       : SSH_Key_Exchange(is_orig);
 } &byteorder=bigendian;
 
@@ -265,6 +271,7 @@ refine connection SSH_Conn += {
 		int state_up_;
 		int state_down_;
 		int version_;
+		int encrypted_bytes_in_current_segment_;
 
 		bool kex_orig_;
 		bool kex_seen_;
@@ -276,6 +283,7 @@ refine connection SSH_Conn += {
 		state_up_   = VERSION_EXCHANGE;
 		state_down_ = VERSION_EXCHANGE;
 		version_    = UNK;
+		encrypted_bytes_in_current_segment_ = 0;
 
 		kex_seen_ = false;
 		kex_orig_ = false;
@@ -288,6 +296,23 @@ refine connection SSH_Conn += {
 		kex_algs_cache_.free();
 	%}
 
+	function clear_encrypted_byte_count_in_current_segment() : bool
+		%{
+		encrypted_bytes_in_current_segment_ = 0;
+		return true;
+		%}
+
+	function inc_encrypted_byte_count_in_current_segment() : bool
+		%{
+		++encrypted_bytes_in_current_segment_;
+		return true;
+		%}
+
+	function get_encrypted_bytes_in_current_segment() : int
+		%{
+		return encrypted_bytes_in_current_segment_;
+		%}
+
 	function get_state(is_orig: bool) : int
 		%{
 		if ( is_orig )

testing/btest/Baseline/scripts.base.protocols.ssh.ssh_segmented_encryption_transition/client.out

@@ -0,0 +1,18 @@
+0, T, 64
+1, F, 64
+2, T, 80
+3, F, 80
+4, T, 272
+5, F, 48
+6, T, 80
+7, F, 528
+8, F, 64
+9, T, 176
+10, F, 160
+11, F, 736
+12, F, 96
+13, T, 64
+14, F, 64
+15, F, 64
+16, F, 48
+17, F, 128

testing/btest/Baseline/scripts.base.protocols.ssh.ssh_segmented_encryption_transition/server.out

@@ -0,0 +1,46 @@
+0, F, 172
+1, T, 44
+2, F, 44
+3, T, 68
+4, F, 52
+5, T, 148
+6, F, 28
+7, T, 112
+8, F, 500
+9, F, 44
+10, T, 460
+11, F, 108
+12, F, 100
+13, F, 36
+14, F, 36
+15, F, 76
+16, F, 36
+17, F, 84
+18, F, 36
+19, F, 84
+20, F, 36
+21, F, 36
+22, F, 36
+23, F, 92
+24, F, 36
+25, F, 108
+26, F, 36
+27, F, 68
+28, F, 36
+29, F, 36
+30, F, 68
+31, F, 36
+32, F, 68
+33, F, 36
+34, F, 36
+35, F, 68
+36, F, 36
+37, F, 92
+38, F, 36
+39, F, 100
+40, T, 36
+41, F, 44
+42, F, 36
+43, F, 140
+44, T, 36
+45, T, 60

testing/btest/scripts/base/protocols/ssh/ssh_segmented_encryption_transition.zeek

@@ -0,0 +1,21 @@
+# In the pcaps used here, the first encrypted packet is sent along with NEWKEYS
+# message of either the client (1st pcap) or the server (2nd pcap) instead of
+# separately.  The "ssh_encrypted_packet" should be raised for such encrypted
+# data appearing within the same tcp segment delivery as other non-encrypted
+# messages.
+
+# @TEST-EXEC: zeek -b -C -r $TRACES/ssh/ssh_client_sends_first_enc_pkt_with_newkeys.pcap %INPUT > client.out
+# @TEST-EXEC: zeek -b -C -r $TRACES/ssh/ssh_server_sends_first_enc_pkt_with_newkeys.pcap %INPUT > server.out
+# @TEST-EXEC: btest-diff client.out
+# @TEST-EXEC: btest-diff server.out
+
+@load base/protocols/ssh
+
+global pkts: count = 0;
+redef SSH::disable_analyzer_after_detection = F;
+
+event ssh_encrypted_packet(c: connection, orig: bool, len: count)
+	{
+	print pkts, orig, len;
+	++pkts;
+	}