Merge remote-tracking branch 'origin/topic/bbannier/issue-1590'

* origin/topic/bbannier/issue-1590:
  Change SSH version field to be `&optional`.
  Reformat function in SSH base script.

Changes during merge:
  * rename weirds to fit into our naming scheme
  * add NEWS entry

Closes GH-1590

CHANGES

@@ -1,4 +1,16 @@
 
+4.1.0-dev.731 | 2021-06-17 10:40:58 +0100
+
+  * Change SSH version field to be `&optional`.
+
+    In version 3.3.0-dev.537 we added handling for SSH version 1.99 which
+    used a SSH version of 0 to indicate weird cases where no version could be
+    determined.
+
+    This patch is a fixup for that patch. We now use an `&optional` version value.
+    If no SSH version can be eixtracted the version will be unset; additionally a
+    `conn_weird` event will be raised. See GH-1590. (Benjamin Bannier, Corelight)
+
 4.1.0-dev.727 | 2021-06-14 16:19:34 -0700
 
   * Bump Highwayhash submodule to pull in fix for FreeBSD (Christian Kreibich, Corelight)

NEWS

@@ -75,6 +75,10 @@ Changed Functionality
   analyzers in the future that better align with the packet analysis framework
   than they do with session analysis.
 
+
+- The version field in ssh.log is now optional and will not be set if we cannot
+  determine the version that was negotiated by the client and server.
+
 Removed Functionality
 ---------------------
 

VERSION

@@ -1 +1 @@
-4.1.0-dev.727
+4.1.0-dev.731

scripts/base/protocols/ssh/main.zeek

@@ -20,8 +20,12 @@ export {
 		uid:             string       &log;
 		## The connection's 4-tuple of endpoint addresses/ports.
 		id:              conn_id      &log;
-		## SSH major version (1 or 2)
+		## SSH major version (1, 2, or unset). The version can be unset if the
-		version:         count        &log;
+		## client and server version strings are unset, malformed or incompatible
+		## so no common version can be extracted. If no version can be extracted
+		## even though both client and server versions are set a weird
+		## will be generated.
+		version:         count        &log &optional;
 		## Authentication result (T=success, F=failure, unset=unknown)
 		auth_success:    bool         &log &optional;
 		## The number of authentication attemps we observed. There's always
@@ -155,65 +159,82 @@ function set_session(c: connection)
 		}
 	}
 
-function set_version(c: connection, version: string)
+function set_version(c: connection)
-    {
+	{
-    if ( c$ssh?$server && c$ssh?$client && |c$ssh$client| > 4 && |c$ssh$server| > 4 )
+	# We always either set the version field to a concrete value, or unset it.
-        {
+	delete c$ssh$version;
-        if ( c$ssh$client[4] == "1" && c$ssh$server[4] == "2" )
+
-            {
+	# If either the client or server string is unset we cannot compute a
-            # SSH199 vs SSH2 -> 2
+	# version and return early. We do not raise a weird in this case as we
-            if ( ( |c$ssh$client| > 7 ) && ( c$ssh$client[6] == "9" ) && ( c$ssh$client[7] == "9" ) )
+	# might arrive here while having only seen one side of the handshake.
-                c$ssh$version = 2;
+	const has_server = c$ssh?$server && |c$ssh$server| > 0;
-            # SSH1 vs SSH2 -> Undefined
+	const has_client = c$ssh?$client && |c$ssh$client| > 0;
-            else
+	if ( ! ( has_server && has_client ) )
-                c$ssh$version = 0;
+		return;
-            }
+
-        else if ( c$ssh$client[4] == "2" && c$ssh$server[4] == "1" )
+	if ( |c$ssh$client| > 4 && |c$ssh$server| > 4 )
-            {
+		{
-            # SSH2 vs SSH199 -> 2
+		if ( c$ssh$client[4] == "1" && c$ssh$server[4] == "2" )
-            if ( ( |c$ssh$server| > 7 ) && ( c$ssh$server[6] == "9" ) && ( c$ssh$server[7] == "9" ) )
+			{
-                c$ssh$version = 2;
+			# SSH199 vs SSH2 -> 2
-            else
+			if ( ( |c$ssh$client| > 7 ) && ( c$ssh$client[6] == "9" ) && ( c$ssh$client[7] == "9" ) )
-            # SSH2 vs SSH1 -> Undefined
+				c$ssh$version = 2;
-                c$ssh$version = 0;
+			# SSH1 vs SSH2 -> Undefined
-            }
+			else
-        else if ( c$ssh$client[4] == "1" && c$ssh$server[4] == "1" )
+				Reporter::conn_weird("SSH_version_mismatch", c, fmt("%s vs %s", c$ssh$server, c$ssh$client));
-            {
+				return;
-            # SSH1 vs SSH199 -> 1
+			}
-            if ( ( |c$ssh$server| > 7 ) && ( c$ssh$server[6] == "9" ) && ( c$ssh$server[7] == "9" ) )
+		else if ( c$ssh$client[4] == "2" && c$ssh$server[4] == "1" )
-                {
+			{
-                # SSH199 vs SSH199
+			# SSH2 vs SSH199 -> 2
-                if (( |c$ssh$client| > 7 ) && ( c$ssh$client[6] == "9" ) && ( c$ssh$client[7] == "9" ))
+			if ( ( |c$ssh$server| > 7 ) && ( c$ssh$server[6] == "9" ) && ( c$ssh$server[7] == "9" ) )
-                    c$ssh$version = 2;
+				c$ssh$version = 2;
-                else
+			else
-                    c$ssh$version = 1;
+				# SSH2 vs SSH1 -> Undefined
-                }
+				Reporter::conn_weird("SSH_version_mismatch", c, fmt("%s vs %s", c$ssh$server, c$ssh$client));
-            else
+				return;
-                {
+			}
-                # SSH1 vs SSH1 -> 1
+		else if ( c$ssh$client[4] == "1" && c$ssh$server[4] == "1" )
-                c$ssh$version = 1;
+			{
-                }
+			# SSH1 vs SSH199 -> 1
-            }
+			if ( ( |c$ssh$server| > 7 ) && ( c$ssh$server[6] == "9" ) && ( c$ssh$server[7] == "9" ) )
-        # SSH2 vs SSH2
+				{
-        else if (c$ssh$client[4] == "2" && c$ssh$server[4] == "2" )
+				# SSH199 vs SSH199
-            {
+				if (( |c$ssh$client| > 7 ) && ( c$ssh$client[6] == "9" ) && ( c$ssh$client[7] == "9" ))
-            c$ssh$version = 2;
+					c$ssh$version = 2;
-            }
+				else
-        }
+					c$ssh$version = 1;
-    }
+				}
+			else
+				{
+				# SSH1 vs SSH1 -> 1
+				c$ssh$version = 1;
+				}
+			}
+		# SSH2 vs SSH2
+		else if (c$ssh$client[4] == "2" && c$ssh$server[4] == "2" )
+			{
+			c$ssh$version = 2;
+			}
+
+		return;
+		}
+
+	Reporter::conn_weird("SSH_cannot_determine_version", c, fmt("%s vs %s", c$ssh$server, c$ssh$client));
+	}
 
 event ssh_server_version(c: connection, version: string)
 	{
 	set_session(c);
 	c$ssh$server = version;
-	set_version(c, version);
+	set_version(c);
 	}
 
 event ssh_client_version(c: connection, version: string)
 	{
 	set_session(c);
 	c$ssh$client = version;
-	set_version(c, version);
+	set_version(c);
 	}
 
 event ssh_auth_attempted(c: connection, authenticated: bool) &priority=5

testing/btest/Baseline/scripts.base.protocols.ssh.set_version/output

@@ -0,0 +1,16 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+SSH1 vs SSH1, 1
+SSH199 vs SSH1, 1
+SSH2 vs SSH2, 2
+SSH199 vs SSH2, 2
+unset vs unset, F
+client unset, F
+server unset, F
+incomplete server version, F
+incomplete client version, F
+SSH1 vs SSH2, F
+SSH2 vs SSH1, F
+conn_weird:, SSH_cannot_determine_version, [orig_h=127.0.0.1, orig_p=40/tcp, resp_h=127.0.0.1, resp_p=40/tcp], SSH vs SSH-1.5-OpenSSH_6.2, 
+conn_weird:, SSH_cannot_determine_version, [orig_h=127.0.0.1, orig_p=40/tcp, resp_h=127.0.0.1, resp_p=40/tcp], SSH-1.5-OpenSSH_6.2 vs SSH, 
+conn_weird:, SSH_version_mismatch, [orig_h=127.0.0.1, orig_p=40/tcp, resp_h=127.0.0.1, resp_p=40/tcp], SSH-1.5-OpenSSH_6.2 vs SSH-2.0-OpenSSH_5.9, 
+conn_weird:, SSH_version_mismatch, [orig_h=127.0.0.1, orig_p=40/tcp, resp_h=127.0.0.1, resp_p=40/tcp], SSH-2.0-OpenSSH_5.9 vs SSH-1.5-OpenSSH_6.2, 

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

@@ -0,0 +1,117 @@
+# @TEST-EXEC: zeek -b %INPUT >output
+# @TEST-EXEC: btest-diff output
+
+@load base/bif/event.bif.zeek
+@load base/protocols/ssh
+
+module SSH;
+
+# Creates a mock connection. This connection is good enough for e.g.,
+# `SSH::set_version`, but not in line with what Zeek considers active
+# connections.
+function make_conn(server: string, client: string): connection
+	{
+	local c: connection;
+	c$uid = "uid";
+
+	local id: conn_id;
+	id$orig_h = 127.0.0.1;
+	id$resp_h = 127.0.0.1;
+	id$orig_p = 40/tcp;
+	id$resp_p = 40/tcp;
+	c$id = id;
+
+	local ssh: SSH::Info;
+	ssh$ts = network_time();
+	ssh$server = server;
+	ssh$client = client;
+	c$ssh = ssh;
+
+	SSH::set_session(c);
+
+	delete c$ssh$version;
+	return c;
+	}
+
+# While `SSH::set_version` triggers a `conn_weird` we are dealing with mock
+# connections which since they are injected are always considered expired by
+# Zeek.
+event expired_conn_weird(name: string, id: conn_id, uid: string, addl: string, source: string)
+	{
+	print "conn_weird:", name, id, addl, source;
+	}
+
+const v1 = "SSH-1.5-OpenSSH_6.2";
+const v199 = "SSH-1.99-OpenSSH_3.1p1";
+const v2 = "SSH-2.0-OpenSSH_5.9";
+
+event zeek_init()
+	{
+	local c: connection;
+
+	# Good cases.
+		{
+		# SSH1 vs SSH1 -> 1.
+		c = make_conn(v1, v1);
+		SSH::set_version(c);
+		print "SSH1 vs SSH1", c$ssh$version;
+
+		# SSH199 vs SSH1 -> 1.
+		c = make_conn(v1, v199);
+		SSH::set_version(c);
+		print "SSH199 vs SSH1", c$ssh$version; # 1.
+
+		# SSH2 vs SSH2 -> 2.
+		c = make_conn(v2, v2);
+		SSH::set_version(c);
+		print "SSH2 vs SSH2", c$ssh$version; # 2.
+
+		# SSH199 vs SSH2 -> 2.
+		c = make_conn(v2, v199);
+		SSH::set_version(c);
+		print "SSH199 vs SSH2", c$ssh$version; # 2.
+		}
+
+	# Error cases.
+		{
+		# Unset vs unset -> unset.
+		c = make_conn("", "");
+		c$ssh$version = 42;
+		SSH::set_version(c);
+		print "unset vs unset", c$ssh?$version; # Unset.
+
+		# Client unset.
+		c = make_conn(v2, "");
+		c$ssh$version = 42;
+		SSH::set_version(c);
+		print "client unset", c$ssh?$version; # Unset.
+
+		# Server unset.
+		c = make_conn("", v2);
+		c$ssh$version = 42;
+		SSH::set_version(c);
+		print "server unset", c$ssh?$version; # Unset.
+
+		# Unable to extract full server version.
+		c = make_conn("SSH", v1);
+		c$ssh$version = 42;
+		SSH::set_version(c);
+		print "incomplete server version", c$ssh?$version;
+
+		# Unable to extract full client version.
+		c = make_conn(v1, "SSH");
+		c$ssh$version = 42;
+		SSH::set_version(c);
+		print "incomplete client version", c$ssh?$version;
+
+		# SSH1 vs SSH2.
+		c = make_conn(v1, v2);
+		SSH::set_version(c);
+		print "SSH1 vs SSH2", c$ssh?$version; # Unset.
+
+		# SSH2 vs SSH1.
+		c = make_conn(v2, v1);
+		SSH::set_version(c);
+		print "SSH2 vs SSH1", c$ssh?$version; # Unset.
+		}
+	}