From 9f8eb682b1bfa5ef2bfa80bd4a868fdbdaf3c77b Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Mon, 27 Mar 2023 13:54:09 +0200 Subject: [PATCH 1/3] zeek-setup: Load scrips before running unit tests 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. --- src/ZeekString.cc | 5 ----- src/zeek-setup.cc | 25 +++++++++++++------------ 2 files changed, 13 insertions(+), 17 deletions(-) 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/zeek-setup.cc b/src/zeek-setup.cc index 98e87cfcbf..7b2878653f 100644 --- a/src/zeek-setup.cc +++ b/src/zeek-setup.cc @@ -679,7 +679,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 ) @@ -726,17 +726,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); @@ -808,6 +797,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() ) { From ea80f21e1d58a0c4e1c52ab4f9cca8bd44cb7bb0 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Mon, 27 Mar 2023 12:50:52 +0200 Subject: [PATCH 2/3] Reassem: Reject blocks overflowing 64bit upper 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). --- src/Reassem.cc | 13 +++++ src/analyzer/protocol/tcp/TCP_Reassembler.cc | 1 + src/file_analysis/FileReassembler.cc | 58 +++++++++++++++++++- 3 files changed, 71 insertions(+), 1 deletion(-) 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/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/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); + } + } From fbdc433386ef88b13a64b5d79f1e889c39c3b825 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Mon, 27 Mar 2023 15:12:27 +0200 Subject: [PATCH 3/3] file_analysis/File: Report overflowing chunks as weird and discard/truncate 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 --- src/file_analysis/File.cc | 10 ++++++++++ 1 file changed, 10 insertions(+) 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 ) {