Merge remote-tracking branch 'security/topic/awelzel/155-reassem-validate-seq-upper-overflow'

* security/topic/awelzel/155-reassem-validate-seq-upper-overflow:
  file_analysis/File: Report overflowing chunks as weird and discard/truncate
  Reassem: Reject blocks overflowing 64bit upper
  zeek-setup: Load scrips before running unit tests

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)

VERSION

@@ -1 +1 @@
-6.0.0-dev.344
+6.0.0-dev.348

src/Reassem.cc

@@ -5,8 +5,10 @@
 #include "zeek/zeek-config.h"
 
 #include <algorithm>
+#include <limits>
 
 #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<uint64_t>::max() - seq < len )
+		{
+		zeek::reporter->InternalWarning("Reassembler::NewBlock() truncating block at seq %" PRIx64
+		                                " from length %" PRIu64 " to %" PRIu64,
+		                                seq, len, std::numeric_limits<uint64_t>::max() - seq);
+		len = std::numeric_limits<uint64_t>::max() - seq;
+		}
+
 	if ( len == 0 )
 		return;
 

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()), "<string-with-NUL>");
 
-	delete zeek::reporter;
-
 	zeek::byte_vec text5 = (zeek::byte_vec)malloc(7);
 	memcpy(text5, text.c_str(), 7);
 	zeek::String s10{true, text5, 6};

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;
 

src/file_analysis/File.cc

@@ -2,6 +2,7 @@
 
 #include "zeek/file_analysis/File.h"
 
+#include <limits>
 #include <utility>
 
 #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<uint64_t>::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<uint64_t>::max() - offset;
+		}
+
 	// Potentially handle reassembly and deliver to the stream analyzers.
 	if ( file_reassembler )
 		{

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<TestFile>("test_file_id", "test_source_name");
+	auto r = std::make_unique<zeek::file_analysis::FileReassembler>(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);
+		}
+	}

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() )
 			{