Merge remote-tracking branch 'origin/topic/jsiwek/fix-netbios-decode-bifs'

* origin/topic/jsiwek/fix-netbios-decode-bifs:
  Fixes to `decode_netbios_name` and `decode_netbios_name_type` BIFs

CHANGES

@@ -1,3 +1,26 @@
+4.1.0-dev.576 | 2021-04-30 09:40:18 -0700
+
+  * Fixes to `decode_netbios_name` and `decode_netbios_name_type` BIFs
+
+    Fixes to `decode_netbios_name`:
+
+    * Improve validation that input string is a NetBIOS encoding
+      (32 bytes, with characters ranging from 'A' to 'P').  This helps
+      prevent Undefined Behavior of left-shifting negative values.
+      Invalid encodings now cause a return-value of an empty string.
+
+    * More liberal in what decoded characters are allowed.  Namely,
+      spaces are now allowed (but any trailing null-bytes and spaces
+      are trimmed, similar to before).
+
+    Fixes to `decode_netbios_name_type`:
+
+    * Improve validation that input string is a NetBIOS encoding
+      (32 bytes, with characters ranging from 'A' to 'P').  This helps
+      prevent Undefined Behavior of left-shifting negative values and
+      a heap-buffer-overread when the input string is too small.
+      Invalid encodings now cause a return-value of 256. (Jon Siwek, Corelight)
+
 4.1.0-dev.573 | 2021-04-29 11:29:39 -0700
 
   * Rename ConnID and ConnIDKey (Tim Wojtulewicz, Corelight)

VERSION

@@ -1 +1 @@
-4.1.0-dev.573
+4.1.0-dev.576

scripts/base/protocols/dns/main.zeek

@@ -434,7 +434,11 @@ event dns_request(c: connection, msg: dns_msg, query: string, qtype: count, qcla
 	#       worked into the query/response in some fashion.
 	if ( c$id$resp_p == 137/udp )
 		{
-		query = decode_netbios_name(query);
+		local decoded_query = decode_netbios_name(query);
+
+		if ( |decoded_query| != 0 )
+			query = decoded_query;
+
 		if ( c$dns$qtype_name == "SRV" )
 			{
 			# The SRV RFC used the ID used for NetBios Status RRs.

src/analyzer/protocol/netbios/functions.bif

@@ -6,48 +6,88 @@
 ##
 ## name: The encoded NetBIOS name, e.g., ``"FEEIEFCAEOEFFEECEJEPFDCAEOEBENEF"``.
 ##
-## Returns: The decoded NetBIOS name, e.g., ``"THE NETBIOS NAME"``.
+## Returns: The decoded NetBIOS name, e.g., ``"THE NETBIOS NAM"``.  An empty
+##          string is returned if the argument is not a valid NetBIOS encoding
+##          (though an encoding that would decode to something that includes
+##          only null-bytes or space-characters also yields an empty string).
 ##
 ## .. zeek:see:: decode_netbios_name_type
 function decode_netbios_name%(name: string%): string
 	%{
+	if ( name->Len() != 32 )
+		return val_mgr->EmptyString();
+
 	char buf[16];
-	char result[16];
 	const u_char* s = name->Bytes();
 	int i, j;
+	int length = 0;
 
 	for ( i = 0, j = 0; i < 16; ++i )
 		{
-		char c0 = (j < name->Len()) ? toupper(s[j++]) : 'A';
-		char c1 = (j < name->Len()) ? toupper(s[j++]) : 'A';
-		buf[i] = ((c0 - 'A') << 4) + (c1 - 'A');
-		}
+		char c0 = toupper(s[j++]);
+		char c1 = toupper(s[j++]);
 
-	for ( i = 0; i < 15; ++i )
-		{
-		if ( isalnum(buf[i]) || ispunct(buf[i]) ||
+		if ( c0 < 'A' || c0 > 'P' || c1 < 'A' || c1 > 'P' )
+			return val_mgr->EmptyString();
+
+		buf[i] = ((c0 - 'A') << 4) + (c1 - 'A');
+
+		if ( isalnum(buf[i]) || ispunct(buf[i]) || buf[i] == ' ' ||
 		     // \x01\x02 is seen in at least one case as the first two bytes.
 		     // I think that any \x01 and \x02 should always be passed through.
 		     buf[i] < 3 )
-			result[i] = buf[i];
+			++length;
 		else
 			break;
 		}
 
-	return zeek::make_intrusive<zeek::StringVal>(i, result);
+
+	// The 16th byte indicates the suffix/type, so don't include it
+	if ( length == 16 )
+		length = 15;
+
+	// Walk back and remove any trailing spaces or nulls
+	for ( ; ; )
+		{
+		if ( length == 0 )
+			return val_mgr->EmptyString();
+
+		auto c = buf[length - 1];
+
+		if ( c != ' ' && c != 0 )
+			break;
+
+		--length;
+		}
+
+	return zeek::make_intrusive<zeek::StringVal>(length, buf);
 	%}
 
 ## Converts a NetBIOS name type to its corresponding numeric value.
 ## See https://en.wikipedia.org/wiki/NetBIOS#NetBIOS_Suffixes.
 ##
-## name: The NetBIOS name type.
+## name: An encoded NetBIOS name.
 ##
-## Returns: The numeric value of *name*.
+## Returns: The numeric value of *name* or 256 if it's not a valid encoding.
 ##
 ## .. zeek:see:: decode_netbios_name
 function decode_netbios_name_type%(name: string%): count
 	%{
+	if ( name->Len() != 32 )
+		return val_mgr->Count(256);
+
 	const u_char* s = name->Bytes();
-	char return_val = ((toupper(s[30]) - 'A') << 4) + (toupper(s[31]) - 'A');
+
+	for ( auto i = 0; i < 32; ++i )
+		{
+		char c = toupper(s[i]);
+
+		if ( c < 'A' || c > 'P' )
+			return val_mgr->Count(256);
+		}
+
+	char c0 = toupper(s[30]);
+	char c1 = toupper(s[31]);
+	char return_val = ((c0 - 'A') << 4) + (c1 - 'A');
 	return zeek::val_mgr->Count(return_val);
 	%}

testing/btest/Baseline/bifs.netbios-functions/out

@@ -1,9 +1,13 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-WORKGROUP
-27
-\x01\x02__MSBROWSE__\x02
-1
-MARTIN
-3
-ISATAP
-0
+0, 6, ISATAP
+27, 9, WORKGROUP
+1, 15, \x01\x02__MSBROWSE__\x02
+3, 6, MARTIN
+69, 15, THE NETBIOS NAM
+0, 15, !"#$%&'()*+,-.=
+0, 8, :;@^_{}~
+0, 0, 
+32, 0, 
+256, 0, 
+256, 0, 
+256, 0, 

testing/btest/bifs/netbios-functions.zeek

@@ -2,17 +2,26 @@
 # @TEST-EXEC: zeek -b %INPUT >out
 # @TEST-EXEC: btest-diff out
 
-event zeek_init()
+function decode_name(name: string)
 	{
-	local names_to_decode = set(
+	local dn = decode_netbios_name(name);
+	local suffix = decode_netbios_name_type(name);
+	print suffix, |dn|, dn;
+	}
+
+local encoded_names = vector(
 		"ejfdebfeebfacacacacacacacacacaaa", # ISATAP
 		"fhepfcelehfcepfffacacacacacacabl", # WORKGROUP
 		"abacfpfpenfdecfcepfhfdeffpfpacab", # \001\002__MSBROWSE__\002
-		"enebfcfeejeocacacacacacacacacaad"); # MARTIN
+		"enebfcfeejeocacacacacacacacacaad", # MARTIN
+		"FEEIEFCAEOEFFEECEJEPFDCAEOEBENEF", # THE NETBIOS NAM
+		"cbcccdcecfcgchcicjckclcmcncodnaa", # !"#$%&'()*+,-.=
+		"dkdleafofphlhnhoaaaaaaaaaaaaaaaa", # :;@^_{}~
+		"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", # empty
+		"cacacacacacacacacacacacacacacaca", # empty
+		"abcd",                             # invalid length
+		"~jfdebfeebfacacacacacacacacacaaa", # invalid alphabet
+		"0jfdebfeebfacacacacacacacacacaaa");# invalid alphabet
 
-	for ( name in names_to_decode )
-		{
-		print decode_netbios_name(name);
-		print decode_netbios_name_type(name);
-		}
-	}
+for ( i in encoded_names )
+	decode_name(encoded_names[i]);

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

@@ -1 +1 @@
-d15d95ad14e8974d828f9ee64fcd6cb313f004a2
+2e7a42892a8cf429787246dbba3927685799b56f