diff --git a/CHANGES b/CHANGES index 54419ebb8c..63d59ffd20 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,30 @@ +6.0.0-dev.348 | 2023-04-11 15:30:45 -0700 + + * file_analysis/File: Report overflowing chunks as weird and discard/truncate (Arne Welzel, Corelight) + + This is one level above the Reassembler where we still have information + about the file and source. A weird entry may looks as follows: + + 1679759398.237353 ... file_offset_overflow FXPLGt4SeMmlMKahJc: offset=fffffffffffffff7 len=10 F zeek HTTP + + * Reassem: Reject blocks overflowing 64bit upper (Arne Welzel, Corelight) + + The reassembler logic isn't wrap around safe, so just truncate or + reject such blocks. For files specifically, a byte offset in the + 2**64 bytes represents 16EiB which is the maximum size supported + by BTRFS or NTFS (and probably nothing we'd ever see in practice). + + * zeek-setup: Load scrips before running unit tests (Arne Welzel, Corelight) + + It is currently not possible to call a->Conn()->GetVal() or construct a + zeek/file_analysis/File object from within doctests, as these quickly + reference the unpopulated zeek::id namespace to construct Val objects + of various types, making it hard write basic tests without completely + re-organizing. + + Move running of the unit tests after parsing the scripts, so it is possible + for some basic exercising of File objects within tests. + 6.0.0-dev.344 | 2023-04-11 15:23:42 -0700 * RDP: Instantiate SSL analyzer instead of PIA (Tim Wojtulewicz, Corelight) diff --git a/VERSION b/VERSION index 727bbd8994..abb8dc0823 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -6.0.0-dev.344 +6.0.0-dev.348 diff --git a/src/Reassem.cc b/src/Reassem.cc index 5ab359211e..4c6834b473 100644 --- a/src/Reassem.cc +++ b/src/Reassem.cc @@ -5,8 +5,10 @@ #include "zeek/zeek-config.h" #include +#include #include "zeek/Desc.h" +#include "zeek/Reporter.h" using std::min; @@ -322,6 +324,17 @@ void Reassembler::CheckOverlap(const DataBlockList& list, uint64_t seq, uint64_t void Reassembler::NewBlock(double t, uint64_t seq, uint64_t len, const u_char* data) { + + // Check for overflows - this should be handled by the caller + // and possibly reported as a weird or violation if applicable. + if ( std::numeric_limits::max() - seq < len ) + { + zeek::reporter->InternalWarning("Reassembler::NewBlock() truncating block at seq %" PRIx64 + " from length %" PRIu64 " to %" PRIu64, + seq, len, std::numeric_limits::max() - seq); + len = std::numeric_limits::max() - seq; + } + if ( len == 0 ) return; diff --git a/src/ZeekString.cc b/src/ZeekString.cc index 860d444de1..2abc1c7cb9 100644 --- a/src/ZeekString.cc +++ b/src/ZeekString.cc @@ -535,9 +535,6 @@ TEST_CASE("construction") CHECK_EQ(s7.Len(), 6); CHECK_EQ(s7.Bytes(), text2); - // Construct a temporary reporter object for the next two tests - zeek::reporter = new zeek::Reporter(false); - zeek::byte_vec text3 = new u_char[7]; memcpy(text3, text.c_str(), 7); zeek::String s8{false, text3, 6}; @@ -549,8 +546,6 @@ TEST_CASE("construction") zeek::String s9{false, text4, 6}; CHECK_EQ(std::string(s9.CheckString()), ""); - delete zeek::reporter; - zeek::byte_vec text5 = (zeek::byte_vec)malloc(7); memcpy(text5, text.c_str(), 7); zeek::String s10{true, text5, 6}; diff --git a/src/analyzer/protocol/tcp/TCP_Reassembler.cc b/src/analyzer/protocol/tcp/TCP_Reassembler.cc index 889696f755..4258fb15ca 100644 --- a/src/analyzer/protocol/tcp/TCP_Reassembler.cc +++ b/src/analyzer/protocol/tcp/TCP_Reassembler.cc @@ -371,6 +371,7 @@ void TCP_Reassembler::BlockInserted(DataBlockMap::const_iterator it) { const auto& start_block = it->second; + assert(start_block.seq < start_block.upper); if ( start_block.seq > last_reassem_seq || start_block.upper <= last_reassem_seq ) return; diff --git a/src/file_analysis/File.cc b/src/file_analysis/File.cc index 88500e9e50..845bc9a4d8 100644 --- a/src/file_analysis/File.cc +++ b/src/file_analysis/File.cc @@ -2,6 +2,7 @@ #include "zeek/file_analysis/File.h" +#include #include #include "zeek/Event.h" @@ -431,6 +432,15 @@ void File::DeliverStream(const u_char* data, uint64_t len) void File::DeliverChunk(const u_char* data, uint64_t len, uint64_t offset) { + if ( std::numeric_limits::max() - offset < len ) + { + reporter->Weird(this, "file_offset_overflow", + zeek::util::fmt("offset=%" PRIx64 " len=%" PRIx64, offset, len), + GetSource().c_str()); + + len = std::numeric_limits::max() - offset; + } + // Potentially handle reassembly and deliver to the stream analyzers. if ( file_reassembler ) { diff --git a/src/file_analysis/FileReassembler.cc b/src/file_analysis/FileReassembler.cc index b95d02cdeb..0cb2b87aec 100644 --- a/src/file_analysis/FileReassembler.cc +++ b/src/file_analysis/FileReassembler.cc @@ -2,6 +2,7 @@ #include "zeek/file_analysis/FileReassembler.h" +#include "zeek/3rdparty/doctest.h" #include "zeek/file_analysis/File.h" namespace zeek::file_analysis @@ -46,7 +47,7 @@ uint64_t FileReassembler::FlushTo(uint64_t sequence) void FileReassembler::BlockInserted(DataBlockMap::const_iterator it) { const auto& start_block = it->second; - + assert(start_block.seq < start_block.upper); if ( start_block.seq > last_reassem_seq || start_block.upper <= last_reassem_seq ) return; @@ -113,3 +114,58 @@ void FileReassembler::Overlap(const u_char* b1, const u_char* b2, uint64_t n) // Not doing anything here yet. } } // end file_analysis + +// Test reassembler logic through FileReassembler. +TEST_CASE("file reassembler") + { + // Can not construct due to protected constructor. + class TestFile : public zeek::file_analysis::File + { + public: + TestFile(const std::string& file_id, const std::string& source_name) + : zeek::file_analysis::File(file_id, source_name) + { + } + }; + + auto f = std::make_unique("test_file_id", "test_source_name"); + auto r = std::make_unique(f.get(), 0); + + const u_char* data = (u_char*)("0123456789ABCDEF"); + + SUBCASE("block overlap and 64bit overflow") + { + r->NewBlock(0.0, 0xfffffffffffffff7, 3, data); + r->NewBlock(0.0, 0xfffffffffffffff7, 15, data); + r->NewBlock(0.0, 0xfffffffffffffff3, 15, data); + + // 0xfffffffffffffff3 through 0xffffffffffffffff + CHECK_EQ(r->TotalSize(), 12); + + // This previously hung with an endless loop. + r->Flush(); + CHECK_FALSE(r->HasBlocks()); + CHECK_EQ(r->TotalSize(), 0); + } + + SUBCASE("reject NewBlock() at 64 bit limit") + { + r->NewBlock(0.0, 0xffffffffffffffff, 4, data); + CHECK_FALSE(r->HasBlocks()); + CHECK_EQ(r->TotalSize(), 0); + } + + SUBCASE("truncate NewBlock() to upper 64 bit limit") + { + r->NewBlock(0.0, 0xfffffffffffffffa, 8, data); + CHECK(r->HasBlocks()); + CHECK_EQ(r->TotalSize(), 5); + } + + SUBCASE("no truncation") + { + r->NewBlock(0.0, 0xfffffffffffffff7, 8, data); + CHECK(r->HasBlocks()); + CHECK_EQ(r->TotalSize(), 8); + } + } diff --git a/src/zeek-setup.cc b/src/zeek-setup.cc index 8a3fedbedb..67b3203d94 100644 --- a/src/zeek-setup.cc +++ b/src/zeek-setup.cc @@ -682,7 +682,7 @@ SetupResult setup(int argc, char** argv, Options* zopts) if ( options.plugins_to_load.empty() && options.scripts_to_load.empty() && options.script_options_to_set.empty() && ! options.pcap_file && ! options.interface && ! options.identifier_to_print && ! command_line_policy && ! options.print_plugins && - ! options.supervisor_mode && ! Supervisor::ThisNode() ) + ! options.supervisor_mode && ! Supervisor::ThisNode() && ! options.run_unit_tests ) add_input_file("-"); for ( const auto& script_option : options.script_options_to_set ) @@ -729,17 +729,6 @@ SetupResult setup(int argc, char** argv, Options* zopts) plugin_mgr->ActivateDynamicPlugins(! options.bare_mode); - // Delay the unit test until here so that plugins have been loaded. - if ( options.run_unit_tests ) - { - set_signal_mask(false); // Allow ctrl-c to abort the tests early - doctest::Context context; - auto dargs = to_cargs(options.doctest_args); - context.applyCommandLine(dargs.size(), dargs.data()); - ZEEK_LSAN_ENABLE(); - exit(context.run()); - } - // Print usage after plugins load so that any path extensions are properly shown. if ( options.print_usage ) usage(argv[0], 0); @@ -811,6 +800,18 @@ SetupResult setup(int argc, char** argv, Options* zopts) init_net_var(); run_bif_initializers(); + // Delay the unit test until here so that plugins and script + // types have been fully loaded. + if ( options.run_unit_tests ) + { + set_signal_mask(false); // Allow ctrl-c to abort the tests early + doctest::Context context; + auto dargs = to_cargs(options.doctest_args); + context.applyCommandLine(dargs.size(), dargs.data()); + ZEEK_LSAN_ENABLE(); + exit(context.run()); + } + // Assign the script_args for command line processing in Zeek scripts. if ( ! options.script_args.empty() ) {