Merge remote-tracking branch 'origin/topic/johanna/ssh-server-banners-can-be-wild'

* origin/topic/johanna/ssh-server-banners-can-be-wild:
  SSH analyzer - tiny aesthetic fixes
  SSH: make banner parsing more robust
  SSH: split banner into client/server parts

CHANGES

@@ -1,3 +1,20 @@
+7.2.0-dev.387 | 2025-03-19 15:28:08 +0000
+
+  * SSH: make banner parsing more robust (Johanna Amann, Corelight)
+
+    This change revamps SSH banner parsing.  The previous behavior was both
+    a bit too strict in some regards, and too permissive in other.
+
+    Specifically, clients are now required to send a line starting with
+    "SSH-" as the first line.  This is in line with the RFC, as well with
+    observed behavior. This also prevents the creation of `ssh.log` for
+    non-SSH traffic on port 22.
+
+    For the server side, we now accept text before the SSH banner. This
+    previously led to a protocol violation but is allowed by the spec.
+
+    New tests are added to cover these cases.
+
 7.2.0-dev.382 | 2025-03-18 11:52:03 -0700
 
   * Add analyzer registration from VLAN to VNTAG (Tim Wojtulewicz, Corelight)

VERSION

@@ -1 +1 @@
-7.2.0-dev.382
+7.2.0-dev.387

src/analyzer/protocol/ssh/events.bif

@@ -12,7 +12,7 @@
 ##    ssh_capabilities ssh2_server_host_key ssh1_server_host_key
 ##    ssh_server_host_key ssh_encrypted_packet ssh2_dh_server_params
 ##    ssh2_gss_error ssh2_ecc_key ssh2_ecc_init ssh2_dh_gex_init
-##    ssh2_gss_init ssh2_rsa_secret
+##    ssh2_gss_init ssh2_rsa_secret ssh_server_pre_banner_data
 event ssh_server_version%(c: connection, version: string%);
 
 ## An :abbr:`SSH (Secure Shell)` Protocol Version Exchange message
@@ -336,3 +336,18 @@ event ssh2_gss_init%(c: connection, is_orig: bool%);
 ##    ssh2_gss_error ssh2_ecc_key ssh2_ecc_init ssh2_dh_gex_init
 ##    ssh2_gss_init ssh2_rsa_secret
 event ssh2_rsa_secret%(c: connection, is_orig: bool%);
+
+## SSH servers can send textual data to the client before sending
+## a banner. The primary use case of this are error messages of TCP
+## wrappers.
+##
+## As this event happens before the SSH banner is exchanged, it is
+## possible that it contains data from different protocols; e.g. if
+## an SSH client connects to a non-SSH-server.
+##
+## c: The connection.
+##
+## data: The pre-banner data.
+##
+## .. zeek:see:: ssh_server_version
+event ssh_server_pre_banner_data%(c: connection, data: string%);

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

@@ -58,20 +58,28 @@ const char* fingerprint_md5(const unsigned char* d)
 %}
 
 refine flow SSH_Flow += {
-	function proc_ssh_version(msg: SSH_Version): bool
+	function proc_ssh_version_client(msg: SSH_Version_Client): bool
 		%{
-		if ( ssh_client_version && ${msg.is_orig } )
+		if ( ssh_client_version )
-			{
 			zeek::BifEvent::enqueue_ssh_client_version(connection()->zeek_analyzer(),
 				connection()->zeek_analyzer()->Conn(),
 				to_stringval(${msg.version}));
-			}
+		return true;
-		else if ( ssh_server_version )
+		%}
+
+	function proc_ssh_version_server(msg: SSH_Version_Server): bool
+		%{
+		if ( ssh_server_version && ${msg.version}.length() > 0 )
 			{
 			zeek::BifEvent::enqueue_ssh_server_version(connection()->zeek_analyzer(),
 				connection()->zeek_analyzer()->Conn(),
 				to_stringval(${msg.version}));
 			}
+		else if ( ssh_server_pre_banner_data )
+			{
+				zeek::BifEvent::enqueue_ssh_server_pre_banner_data(connection()->zeek_analyzer(),
+				connection()->zeek_analyzer()->Conn(), to_stringval(${msg.nonversiondata}));
+			}
 		return true;
 		%}
 
@@ -267,8 +275,12 @@ refine flow SSH_Flow += {
 		%}
 };
 
-refine typeattr SSH_Version += &let {
+refine typeattr SSH_Version_Client += &let {
-	proc: bool = $context.flow.proc_ssh_version(this);
+	proc: bool = $context.flow.proc_ssh_version_client(this);
+};
+
+refine typeattr SSH_Version_Server += &let {
+	proc: bool = $context.flow.proc_ssh_version_server(this);
 };
 
 refine typeattr SSH2_KEXINIT += &let {

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

@@ -20,16 +20,32 @@ 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);
+	VERSION_EXCHANGE -> version   : SSH_Version_Switch(is_orig);
 	ENCRYPTED        -> encrypted : EncryptedByte(is_orig);
 	default          -> kex       : SSH_Key_Exchange(is_orig);
 } &byteorder=bigendian;
 
-type SSH_Version(is_orig: bool) = record {
+type SSH_Version_Switch(is_orig: bool) = case is_orig of {
-	version : bytestring &oneline;
+	true -> client_version : SSH_Version_Client;
+	false -> server_version: SSH_Version_Server;
+};
+
+# SSH servers can have banners before their SSH version. Which... fun.
+type SSH_Version_Server = record {
+	version: RE/(SSH-.*)?/;
+	# only UTF-8 data. This doesn't catch all bad cases, but some
+	nonversiondata: RE/([^[\xC0-\xC1]|[\xF5-\xFF]])*/;
 } &let {
-	update_state   : bool = $context.connection.update_state(KEX_INIT, is_orig);
+	update_state   : bool = $context.connection.update_state(KEX_INIT, false) &if(sizeof(version) > 0);
-	update_version : bool = $context.connection.update_version(version, is_orig);
+	update_version : bool = $context.connection.update_version(version, false) &if(sizeof(version) > 0);
+} &oneline;
+
+# SSH clients _always_ have to send a line starting with SSH- first.
+type SSH_Version_Client = record {
+	version : RE/SSH-.*/ &oneline;
+} &let {
+	update_state   : bool = $context.connection.update_state(KEX_INIT, true);
+	update_version : bool = $context.connection.update_version(version, true);
 };
 
 type SSH_Key_Exchange(is_orig: bool) = record {

testing/btest/Baseline/scripts.base.protocols.ssh.http-port-22/analyzer.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	analyzer
+#open XXXX-XX-XX-XX-XX-XX
+#fields	ts	cause	analyzer_kind	analyzer_name	uid	fuid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	failure_reason	failure_data
+#types	time	string	string	string	string	string	addr	port	addr	port	string	string
+XXXXXXXXXX.XXXXXX	violation	protocol	SSH	CHhAvVGS1DHFjwGM9	-	10.0.0.1	51889	192.168.0.1	22	Binpac exception: binpac exception: string mismatch at <...>/ssh-protocol.pac:45: \x0aexpected pattern: "SSH-.*"\x0aactual data: "GET / HTTP/1.1"	-
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ssh.http-port-22/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	ip_proto
+#types	time	string	addr	port	addr	port	enum	string	interval	count	count	string	bool	bool	count	string	count	count	count	count	set[string]	count
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	10.0.0.1	51889	192.168.0.1	22	tcp	http	0.000260	18	12649	SF	T	T	0	ShADadFf	15	618	13	13169	-	6
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ssh.http-port-22/http.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	http
+#open XXXX-XX-XX-XX-XX-XX
+#fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	trans_depth	method	host	uri	referrer	version	user_agent	origin	request_body_len	response_body_len	status_code	status_msg	info_code	info_msg	tags	username	password	proxied	orig_fuids	orig_filenames	orig_mime_types	resp_fuids	resp_filenames	resp_mime_types
+#types	time	string	addr	port	addr	port	count	string	string	string	string	string	string	string	count	count	count	string	count	string	set[enum]	string	string	set[string]	vector[string]	vector[string]	vector[string]	vector[string]	vector[string]	vector[string]
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	10.0.0.1	51889	192.168.0.1	22	1	GET	-	/	-	1.1	-	-	0	12632	200	OK	-	-	(empty)	-	-	-	-	-	-	FsaSIr11Ze8VUH5yPj	-	text/plain
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ssh.pre-banner/.stdout

@@ -0,0 +1,36 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+ _____________
+< Hi stranger >
+ -------------
+   \             \
+    \             \_
+     \             \\
+      \             \\/\
+       \            _\\/
+        \         /   -\
+         \      /  oo   -\
+          \   /           \
+             |    ---\    -\
+             \--/     \     \
+                       |      -\
+                        \       -\         -------------\    /-\
+                         \        \-------/              ---/    \
+                          \                                  |\   \
+                           |                                 / |  |
+                           \                                |  \  |
+                            |                              /    \ |
+                            |                             /     \ |
+                             \                             \     \|
+                              -              /--------\    |      o
+                               \+   +---------          \   |
+                                |   |                   |   \
+                                |   |                    \   |
+                                |   |                    |   \
+                                |   |                     \   |
+                                 \  |                     |   |
+                                 |  |                      \  \
+                                 |  |                      |   |
+                                 +--+                       ---+
+Habit is habit, and not to be flung out of the window by any man, but coaxed
+down-stairs a step at a time.
+\x09\x09-- Mark Twain, "Pudd'nhead Wilson's Calendar

testing/btest/Baseline/scripts.base.protocols.ssh.pre-banner/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	ip_proto
+#types	time	string	addr	port	addr	port	enum	string	interval	count	count	string	bool	bool	count	string	count	count	count	count	set[string]	count
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	10.0.2.1	55343	10.0.2.10	22	tcp	ssh	0.201784	2869	4728	S1	T	T	0	ShADad	21	3973	15	5516	-	6
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline/scripts.base.protocols.ssh.pre-banner/ssh.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	ssh
+#open XXXX-XX-XX-XX-XX-XX
+#fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	version	auth_success	auth_attempts	direction	client	server	cipher_alg	mac_alg	compression_alg	kex_alg	host_key_alg	host_key
+#types	time	string	addr	port	addr	port	count	bool	count	enum	string	string	string	string	string	string	string	string
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	10.0.2.1	55343	10.0.2.10	22	2	-	0	-	SSH-2.0-OpenSSH_9.7	SSH-2.0-OpenSSH_9.2p1 Debian-2+deb12u5	aes192-ctr	hmac-sha2-256	zlib@openssh.com	sntrup761x25519-sha512@openssh.com	ssh-ed25519	27:27:33:7a:1a:4f:46:b2:58:1c:04:c2:ad:6d:8a:86
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/scripts/base/protocols/ssh/http-port-22.test

@@ -0,0 +1,7 @@
+# Validate that a text-based protocol pn port 22 does not generate a ssh logfile.
+
+# @TEST-EXEC: zeek -r $TRACES/http/http-single-conn-22.pcap %INPUT
+# @TEST-EXEC: test ! -f ssh.log
+# @TEST-EXEC: btest-diff http.log
+# @TEST-EXEC: btest-diff conn.log
+# @TEST-EXEC: TEST_DIFF_CANONIFIER="$SCRIPTS/diff-remove-abspath | $SCRIPTS/diff-remove-timestamps" btest-diff analyzer.log

testing/btest/scripts/base/protocols/ssh/pre-banner.test

@@ -0,0 +1,11 @@
+# This tests a trace that has data before the banner.
+
+# @TEST-EXEC: zeek -r $TRACES/ssh/server-pre-banner-data.pcap %INPUT
+# @TEST-EXEC: btest-diff ssh.log
+# @TEST-EXEC: btest-diff conn.log
+# @TEST-EXEC: btest-diff .stdout
+
+event ssh_server_pre_banner_data(c: connection, data: string)
+	{
+	print data;
+	}

testing/external/commit-hash.zeek-testing-private

@@ -1 +1 @@
-296a3b2bfd36a74c8aa22f175cea4c00a9f4d079
+d20f3027e30434d340f1d3b45b5f86c84e5c74e0