From bac6ad62b5b9c646a8e336de0c86b39fdc7bf4e9 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Wed, 10 Apr 2024 16:33:30 -0700 Subject: [PATCH] Allow SMB_TCP record to contain multiple protocol identifiers/headers --- NEWS | 3 ++ src/analyzer/protocol/smb/smb.pac | 9 ++++-- src/analyzer/protocol/smb/smb2-protocol.pac | 6 ++++ .../analyzer.log | 12 +++++++ .../weird.log | 11 ------- .../out | 10 +++--- .../out | 6 ++++ .../btest/Traces/smb/smb2-multiple-pdus.pcap | Bin 0 -> 1100 bytes .../protocols/smb/smb1-OSS-fuzz-54883.test | 3 +- .../protocols/smb/smb2-multiple-pdus.test | 30 ++++++++++++++++++ .../external/commit-hash.zeek-testing-private | 2 +- 11 files changed, 72 insertions(+), 20 deletions(-) create mode 100644 testing/btest/Baseline/scripts.base.protocols.smb.smb1-OSS-fuzz-54883/analyzer.log delete mode 100644 testing/btest/Baseline/scripts.base.protocols.smb.smb1-OSS-fuzz-54883/weird.log create mode 100644 testing/btest/Baseline/scripts.base.protocols.smb.smb2-multiple-pdus/out create mode 100644 testing/btest/Traces/smb/smb2-multiple-pdus.pcap create mode 100644 testing/btest/scripts/base/protocols/smb/smb2-multiple-pdus.test diff --git a/NEWS b/NEWS index 38429acb34..ed82e649c5 100644 --- a/NEWS +++ b/NEWS @@ -36,6 +36,9 @@ New Functionality might be "7.0.0-dev.114-XXX-debug". For Docker builds, the ``LOCALVERSION`` environment variable configures the addition. +- SMB2 packets containing multiple PDUs now correctly parse all of the headers, + instead of just the first one and ignoring the rest. + Changed Functionality --------------------- diff --git a/src/analyzer/protocol/smb/smb.pac b/src/analyzer/protocol/smb/smb.pac index dd909552ab..7ca8efa5c5 100644 --- a/src/analyzer/protocol/smb/smb.pac +++ b/src/analyzer/protocol/smb/smb.pac @@ -115,7 +115,7 @@ type SMB_TCP(is_orig: bool) = record { len24 : uint24; body : case message_type of { # SMB/SMB2 packets are required to use NBSS session messages. - 0 -> nbss : SMB_Protocol_Identifier(is_orig, len); + 0 -> nbss : SMB_Protocol_Identifier(is_orig, len)[] &until($element.end_of_chain); # TODO: support more nbss message types? default -> skip : bytestring &transient &restofdata; @@ -126,7 +126,7 @@ type SMB_TCP(is_orig: bool) = record { type SMB_Protocol_Identifier(is_orig: bool, msg_len: uint32) = record { # Sort of cheating by reading this in as an integer instead of a string. - protocol : uint32 &byteorder=bigendian; + protocol : int32 &byteorder=bigendian; smb_1_or_2 : case protocol of { SMB1 -> smb1 : SMB_PDU(is_orig, msg_len); SMB2 -> smb2 : SMB2_PDU(is_orig); @@ -134,6 +134,11 @@ type SMB_Protocol_Identifier(is_orig: bool, msg_len: uint32) = record { SMB3 -> smb3 : SMB2_transform_header; default -> unknown : empty; }; +} &let { + # For smb2, a packet can contain multiple PDUs. Check for the end of the chain based + # on the smb2 header to end the array in SMB_TCP. If smb1 or smb3, it's always + # the end of the chain. + end_of_chain: bool = (protocol == SMB2) ? smb2.end_of_chain : true; }; %include smb1-protocol.pac diff --git a/src/analyzer/protocol/smb/smb2-protocol.pac b/src/analyzer/protocol/smb/smb2-protocol.pac index f8126ba3bf..b922c62f2b 100644 --- a/src/analyzer/protocol/smb/smb2-protocol.pac +++ b/src/analyzer/protocol/smb/smb2-protocol.pac @@ -166,6 +166,12 @@ type SMB2_PDU(is_orig: bool) = record { true -> err : SMB2_error_response(header); false -> msg : SMB2_Message(header, is_orig); }; + pad : case header.next_command of { + 0 -> none: empty; + default -> chain_pad: bytestring &length = header.next_command - header.head_length - @sizeof(message); + }; +} &let { + end_of_chain: bool = header.next_command == 0; }; type SMB2_Message(header: SMB2_Header, is_orig: bool) = case is_orig of { diff --git a/testing/btest/Baseline/scripts.base.protocols.smb.smb1-OSS-fuzz-54883/analyzer.log b/testing/btest/Baseline/scripts.base.protocols.smb.smb1-OSS-fuzz-54883/analyzer.log new file mode 100644 index 0000000000..75162babdb --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.smb.smb1-OSS-fuzz-54883/analyzer.log @@ -0,0 +1,12 @@ +### 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 SMB CHhAvVGS1DHFjwGM9 - 10.0.0.1 48026 10.0.0.2 139 Binpac exception: binpac exception: out_of_bound: SMB1_write_andx_request:data: 762064980 > 32 - +XXXXXXXXXX.XXXXXX violation protocol SMB CHhAvVGS1DHFjwGM9 - 10.0.0.1 48026 10.0.0.2 139 Binpac exception: binpac exception: out_of_bound: SMB1_write_andx_request:data: 4123 > 32 - +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/scripts.base.protocols.smb.smb1-OSS-fuzz-54883/weird.log b/testing/btest/Baseline/scripts.base.protocols.smb.smb1-OSS-fuzz-54883/weird.log deleted file mode 100644 index 948dd45b88..0000000000 --- a/testing/btest/Baseline/scripts.base.protocols.smb.smb1-OSS-fuzz-54883/weird.log +++ /dev/null @@ -1,11 +0,0 @@ -### 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 weird -#open XXXX-XX-XX-XX-XX-XX -#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p name addl notice peer source -#types time string addr port addr port string string bool string string -XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 10.0.0.1 48026 10.0.0.2 139 smb_tree_connect_andx_response_without_tree current_cmd=WRITE_ANDX F zeek - -#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/scripts.base.protocols.smb.smb2-max-pending-messages/out b/testing/btest/Baseline/scripts.base.protocols.smb.smb2-max-pending-messages/out index 524f00fa21..ac010c9782 100644 --- a/testing/btest/Baseline/scripts.base.protocols.smb.smb2-max-pending-messages/out +++ b/testing/btest/Baseline/scripts.base.protocols.smb.smb2-max-pending-messages/out @@ -9,17 +9,19 @@ smb2_discarded_messages_state before, tree, 20 smb2_discarded_messages_state after, tree, 0 smb2_discarded_messages_state before, tree, 20 smb2_discarded_messages_state after, tree, 0 -smb2_discarded_messages_state before, read, 15 +smb2_discarded_messages_state before, tree, 20 +smb2_discarded_messages_state after, tree, 0 +smb2_discarded_messages_state before, read, 17 smb2_discarded_messages_state after, read, 0 -smb2_discarded_messages_state before, tree, 5 +smb2_discarded_messages_state before, tree, 3 smb2_discarded_messages_state after, tree, 0 smb2_discarded_messages_state before, tree, 20 smb2_discarded_messages_state after, tree, 0 smb2_discarded_messages_state before, tree, 20 smb2_discarded_messages_state after, tree, 0 -smb2_discarded_messages_state before, read, 15 +smb2_discarded_messages_state before, read, 17 smb2_discarded_messages_state after, read, 0 -smb2_discarded_messages_state before, tree, 5 +smb2_discarded_messages_state before, tree, 3 smb2_discarded_messages_state after, tree, 0 smb2_discarded_messages_state before, tree, 20 smb2_discarded_messages_state after, tree, 0 diff --git a/testing/btest/Baseline/scripts.base.protocols.smb.smb2-multiple-pdus/out b/testing/btest/Baseline/scripts.base.protocols.smb.smb2-multiple-pdus/out new file mode 100644 index 0000000000..47cb42357e --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.smb.smb2-multiple-pdus/out @@ -0,0 +1,6 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +smb2_create_request, [credit_charge=1, status=0, command=5, credits=256, flags=0, message_id=920, process_id=65279, tree_id=400799417, session_id=3163393275, signature=\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00], [filename=testfile.txt, disposition=1, create_options=0] +smb2_file_delete, [credit_charge=1, status=0, command=17, credits=256, flags=4, message_id=921, process_id=65279, tree_id=400799417, session_id=3163393275, signature=\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00] +smb2_close_request, [credit_charge=1, status=0, command=6, credits=256, flags=4, message_id=922, process_id=65279, tree_id=400799417, session_id=3163393275, signature=\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00] +smb2_create_response, [credit_charge=1, status=0, command=5, credits=0, flags=1, message_id=920, process_id=65279, tree_id=400799417, session_id=3163393275, signature=\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00], [file_id=[persistent=1683729331, volatile=1370986173], size=12, times=[modified=XXXXXXXXXX.XXXXXX, modified_raw=133512003510343707, accessed=XXXXXXXXXX.XXXXXX, accessed_raw=133512016962187242, created=XXXXXXXXXX.XXXXXX, created_raw=133512003510341360, changed=XXXXXXXXXX.XXXXXX, changed_raw=133512003510343707], attrs=[read_only=F, hidden=F, system=F, directory=F, archive=T, normal=F, temporary=F, sparse_file=F, reparse_point=F, compressed=F, offline=F, not_content_indexed=F, encrypted=F, integrity_stream=F, no_scrub_data=F], create_action=1] +smb2_close_response, [credit_charge=1, status=0, command=6, credits=3, flags=5, message_id=922, process_id=65279, tree_id=400799417, session_id=3163393275, signature=\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00], [alloc_size=0, eof=0, times=[modified=-1.16444736e+10, modified_raw=0, accessed=-1.16444736e+10, accessed_raw=0, created=-1.16444736e+10, created_raw=0, changed=-1.16444736e+10, changed_raw=0], attrs=[read_only=F, hidden=F, system=F, directory=F, archive=F, normal=F, temporary=F, sparse_file=F, reparse_point=F, compressed=F, offline=F, not_content_indexed=F, encrypted=F, integrity_stream=F, no_scrub_data=F]] diff --git a/testing/btest/Traces/smb/smb2-multiple-pdus.pcap b/testing/btest/Traces/smb/smb2-multiple-pdus.pcap new file mode 100644 index 0000000000000000000000000000000000000000..498c807374ca4d5b1c9a8d65bc79a0167413c5bf GIT binary patch literal 1100 zcmca|c+)~A1{MYc;9y_`a@NnR!223H2g4-50!Y61t#C{R3V{JcT!O&_$O|g; zDFz9yvS2VjcCTSE_tMBKFV^QC1&JYt-Q80QZKj8NGj?7{d{p${$wiPn2q61g5a@4^ z9+10c66tS621Z_xzY>5L1pcD|n4gjDVT0NOQa_7GdqCj<0%*qn2c-u4zpM;mz`zG# zNIHO|2~cWa;)pm6N)3^(z$pQkA{h4`PzR-i%Rhf#Xpmr>tg;m79AIhyrUU^{iU9I1 zAiEZr8ogi;4sF%AM-tN~L8C}%Ku05P$t Z1C}F@!;_7H8Eg*7YGPyG2Beq*002hr<68g# literal 0 HcmV?d00001 diff --git a/testing/btest/scripts/base/protocols/smb/smb1-OSS-fuzz-54883.test b/testing/btest/scripts/base/protocols/smb/smb1-OSS-fuzz-54883.test index 6c1206ea2c..7267be9378 100644 --- a/testing/btest/scripts/base/protocols/smb/smb1-OSS-fuzz-54883.test +++ b/testing/btest/scripts/base/protocols/smb/smb1-OSS-fuzz-54883.test @@ -1,9 +1,8 @@ #@TEST-EXEC: zeek -b -C -r $TRACES/smb/smb1-OSS-fuzz-54883.pcap %INPUT -#@TEST-EXEC: btest-diff weird.log +#@TEST-EXEC: btest-diff analyzer.log #@TEST-EXEC: ! test -f reporter.log @load base/protocols/smb -@load base/frameworks/notice/weird # The traffic generated by OSS Fuzz is broken to the extreme, ensure # the analyzer isn't disabled so the original scripting issue triggers. diff --git a/testing/btest/scripts/base/protocols/smb/smb2-multiple-pdus.test b/testing/btest/scripts/base/protocols/smb/smb2-multiple-pdus.test new file mode 100644 index 0000000000..9e0eeee6fc --- /dev/null +++ b/testing/btest/scripts/base/protocols/smb/smb2-multiple-pdus.test @@ -0,0 +1,30 @@ +# @TEST-DOC: Tests handling of packets with mulitple SMB2 PDUs in them +# @TEST-EXEC: zeek -b -r $TRACES/smb/smb2-multiple-pdus.pcap %INPUT 2>&1 >out +# @TEST-EXEC: btest-diff out + +@load base/protocols/smb + +event smb2_file_delete(c: connection, hdr: SMB2::Header, file_id: SMB2::GUID, delete_pending: bool) +{ + print "smb2_file_delete", hdr; +} + +event smb2_create_request(c: connection, hdr: SMB2::Header, request: SMB2::CreateRequest) +{ + print "smb2_create_request", hdr, request; +} + +event smb2_close_request(c: connection, hdr: SMB2::Header, file_id: SMB2::GUID) +{ + print "smb2_close_request", hdr; +} + +event smb2_create_response(c: connection, hdr: SMB2::Header, response: SMB2::CreateResponse) +{ + print "smb2_create_response", hdr, response; +} + +event smb2_close_response(c: connection, hdr: SMB2::Header, response: SMB2::CloseResponse) +{ + print "smb2_close_response", hdr, response; +} diff --git a/testing/external/commit-hash.zeek-testing-private b/testing/external/commit-hash.zeek-testing-private index 54615157ba..2772a42b88 100644 --- a/testing/external/commit-hash.zeek-testing-private +++ b/testing/external/commit-hash.zeek-testing-private @@ -1 +1 @@ -43987517cd3af2ad34ae50b217591d392d76e012 +84059cf250af1277e01f16067570e8f535ad3573