From 76fb1e7fd0ef2057b05fbe8cddc2479019de78b8 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Tue, 27 Apr 2021 15:27:04 -0700 Subject: [PATCH] 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. --- scripts/base/protocols/dns/main.zeek | 6 +- src/analyzer/protocol/netbios/functions.bif | 68 +++++++++++++++---- .../btest/Baseline/bifs.netbios-functions/out | 20 +++--- testing/btest/bifs/netbios-functions.zeek | 27 +++++--- .../external/commit-hash.zeek-testing-private | 2 +- 5 files changed, 90 insertions(+), 33 deletions(-) diff --git a/scripts/base/protocols/dns/main.zeek b/scripts/base/protocols/dns/main.zeek index 4f48e510bc..85c90efadc 100644 --- a/scripts/base/protocols/dns/main.zeek +++ b/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. diff --git a/src/analyzer/protocol/netbios/functions.bif b/src/analyzer/protocol/netbios/functions.bif index 9c5cd2468e..8ea506b4c1 100644 --- a/src/analyzer/protocol/netbios/functions.bif +++ b/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(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(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); %} diff --git a/testing/btest/Baseline/bifs.netbios-functions/out b/testing/btest/Baseline/bifs.netbios-functions/out index 10231f8ae3..f67f434df7 100644 --- a/testing/btest/Baseline/bifs.netbios-functions/out +++ b/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, diff --git a/testing/btest/bifs/netbios-functions.zeek b/testing/btest/bifs/netbios-functions.zeek index c3e951ffa8..402c06c07f 100644 --- a/testing/btest/bifs/netbios-functions.zeek +++ b/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]); diff --git a/testing/external/commit-hash.zeek-testing-private b/testing/external/commit-hash.zeek-testing-private index f15b634368..69b3e7dfd4 100644 --- a/testing/external/commit-hash.zeek-testing-private +++ b/testing/external/commit-hash.zeek-testing-private @@ -1 +1 @@ -d15d95ad14e8974d828f9ee64fcd6cb313f004a2 +2e7a42892a8cf429787246dbba3927685799b56f