From ea80f21e1d58a0c4e1c52ab4f9cca8bd44cb7bb0 Mon Sep 17 00:00:00 2001
From: Arne Welzel <arne.welzel@corelight.com>
Date: Mon, 27 Mar 2023 12:50:52 +0200
Subject: [PATCH] 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 <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;
 
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<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);
+		}
+	}