Add test case for binpac flowbuffer frame length parsing bug

aux/binpac

@@ -1 +1 @@
-Subproject commit 35406f20555a3978a7ad56f6f977004f6bf47f9b
+Subproject commit 31203a3458baa6bdfc355e260a102f9402fbe3e2

testing/btest/Baseline/plugins.binpac-flowbuffer-frame-length/output

@@ -0,0 +1 @@
+foo_message, 22, 17, 224

testing/btest/plugins/binpac-flowbuffer-frame-length-plugin/CMakeLists.txt

@@ -0,0 +1,19 @@
+
+project(Zeek-Plugin-Foo-FOO)
+
+cmake_minimum_required(VERSION 2.6.3)
+
+if ( NOT ZEEK_DIST )
+    message(FATAL_ERROR "ZEEK_DIST not set")
+endif ()
+
+set(CMAKE_MODULE_PATH ${ZEEK_DIST}/cmake)
+
+include(ZeekPlugin)
+
+zeek_plugin_begin(Foo FOO)
+zeek_plugin_cc(src/Plugin.cc)
+zeek_plugin_cc(src/FOO.cc)
+zeek_plugin_bif(src/foo.bif)
+zeek_plugin_pac(src/foo.pac)
+zeek_plugin_end()

testing/btest/plugins/binpac-flowbuffer-frame-length-plugin/scripts/__load__.zeek

@@ -0,0 +1 @@
+@load ./main

testing/btest/plugins/binpac-flowbuffer-frame-length-plugin/scripts/main.zeek

@@ -0,0 +1,11 @@
+module Foo;
+
+const ports = { 102/tcp };
+
+redef likely_server_ports += { ports };
+
+event zeek_init() &priority=5
+	{
+    Analyzer::register_for_ports(Analyzer::ANALYZER_FOO, ports);
+	}
+

testing/btest/plugins/binpac-flowbuffer-frame-length-plugin/src/FOO.cc

@@ -0,0 +1,65 @@
+#include "FOO.h"
+
+#include "analyzer/protocol/tcp/TCP_Reassembler.h"
+
+#include "Reporter.h"
+
+#include "foo.bif.h"
+
+using namespace analyzer::FOO;
+
+FOO_Analyzer::FOO_Analyzer(Connection* c) : tcp::TCP_ApplicationAnalyzer("FOO", c)
+	{
+	interp = new binpac::FOO::FOO_Conn(this);
+	had_gap = false;
+	}
+
+FOO_Analyzer::~FOO_Analyzer()
+	{
+	delete interp;
+	}
+
+void FOO_Analyzer::Done()
+	{
+	tcp::TCP_ApplicationAnalyzer::Done();
+
+	interp->FlowEOF(true);
+	interp->FlowEOF(false);
+	}
+
+void FOO_Analyzer::EndpointEOF(bool is_orig)
+	{
+	tcp::TCP_ApplicationAnalyzer::EndpointEOF(is_orig);
+	interp->FlowEOF(is_orig);
+	}
+
+void FOO_Analyzer::DeliverStream(int len, const u_char* data, bool orig)
+	{
+	tcp::TCP_ApplicationAnalyzer::DeliverStream(len, data, orig);
+
+	assert(TCP());
+	if ( TCP()->IsPartial() )
+		return;
+
+	if ( had_gap )
+		// If only one side had a content gap, we could still try to
+		// deliver data to the other side if the script layer can handle this.
+		return;
+
+	try
+		{
+		interp->NewData(orig, data, data + len);
+		}
+	catch ( const binpac::Exception& e )
+		{
+		printf("Exception: %s\n", e.c_msg());
+		ProtocolViolation(fmt("Binpac exception: %s", e.c_msg()));
+		}
+	}
+
+void FOO_Analyzer::Undelivered(uint64_t seq, int len, bool orig)
+	{
+	tcp::TCP_ApplicationAnalyzer::Undelivered(seq, len, orig);
+	had_gap = true;
+	interp->NewGap(orig, len);
+	}

testing/btest/plugins/binpac-flowbuffer-frame-length-plugin/src/FOO.h

@@ -0,0 +1,37 @@
+#ifndef ANALYZER_PROTOCOL_FOO_FOO_H
+#define ANALYZER_PROTOCOL_FOO_FOO_H
+
+#include "foo.bif.h"
+
+#include "analyzer/protocol/tcp/TCP.h"
+
+#include "foo_pac.h"
+
+namespace analyzer { namespace FOO {
+
+class FOO_Analyzer : public tcp::TCP_ApplicationAnalyzer {
+public:
+	FOO_Analyzer(Connection* conn);
+	virtual ~FOO_Analyzer();
+
+	// Overriden from Analyzer.
+	virtual void Done();
+
+	virtual void DeliverStream(int len, const u_char* data, bool orig);
+	virtual void Undelivered(uint64_t seq, int len, bool orig);
+
+	// Overriden from tcp::TCP_ApplicationAnalyzer.
+	virtual void EndpointEOF(bool is_orig);
+
+	static analyzer::Analyzer* InstantiateAnalyzer(Connection* conn)
+		{ return new FOO_Analyzer(conn); }
+
+protected:
+	binpac::FOO::FOO_Conn* interp;
+	bool had_gap;
+
+};
+
+} } // namespace analyzer::FOO
+
+#endif

testing/btest/plugins/binpac-flowbuffer-frame-length-plugin/src/Plugin.cc

@@ -0,0 +1,26 @@
+#include "plugin/Plugin.h"
+#include "analyzer/Component.h"
+
+#include "FOO.h"
+
+namespace plugin {
+namespace Foo_FOO {
+
+class Plugin : public plugin::Plugin {
+public:
+	plugin::Configuration Configure()
+		{
+		AddComponent(new ::analyzer::Component("FOO",
+		             ::analyzer::FOO::FOO_Analyzer::InstantiateAnalyzer));
+
+		plugin::Configuration config;
+		config.name = "FOO::Foo";
+		config.description = "Foo Analyzer analyzer";
+		config.version.major = 1;
+		config.version.minor = 0;
+		return config;
+		}
+} plugin;
+
+}
+}

testing/btest/plugins/binpac-flowbuffer-frame-length-plugin/src/foo.bif

@@ -0,0 +1,4 @@
+module Foo;
+
+event foo_message%(c: connection, is_orig: bool, len: count, plen: count, ptype: count%);
+

testing/btest/plugins/binpac-flowbuffer-frame-length-plugin/src/foo.pac

@@ -0,0 +1,60 @@
+%include binpac.pac
+%include bro.pac
+
+%extern{
+	#include "foo.bif.h"
+%}
+
+analyzer FOO withcontext {
+	connection: FOO_Conn;
+	flow:       FOO_Flow;
+};
+
+# Our connection consists of two flows, one in each direction.
+connection FOO_Conn(bro_analyzer: BroAnalyzer) {
+	upflow   = FOO_Flow(true);
+	downflow = FOO_Flow(false);
+};
+
+type HDR = record {
+    version:    uint8;
+    reserved:   uint8;
+    len:        uint16;
+} &byteorder=bigendian;
+
+type FOO_PDU(is_orig: bool) = record {
+    hdr:        HDR;
+    plen:       uint8;
+    ptype:      uint8;
+    something:  bytestring &restofdata;
+} &byteorder=bigendian, &length=hdr.len;
+
+# Now we define the flow:
+flow FOO_Flow(is_orig: bool) {
+
+	flowunit = FOO_PDU(is_orig) withcontext(connection, this);
+	# datagram = FOO_PDU(is_orig) withcontext(connection, this);
+
+};
+
+refine flow FOO_Flow += {
+    function proc_foo_message(msg: FOO_PDU): bool
+        %{
+        // printf("FOO %d %d\n", msg->hdr()->len(), msg->hdr_len());
+        connection()->bro_analyzer()->ProtocolConfirmation();
+        BifEvent::Foo::generate_foo_message(
+                        connection()->bro_analyzer(),
+                        connection()->bro_analyzer()->Conn(),
+                        is_orig(),
+                        msg->hdr()->len(),
+                        msg->plen(),
+                        msg->ptype());
+        return true;
+        %}
+
+};
+
+refine typeattr FOO_PDU += &let {
+    proc: bool = $context.flow.proc_foo_message(this);
+};
+

testing/btest/plugins/binpac-flowbuffer-frame-length.zeek

@@ -0,0 +1,40 @@
+# This test exercises a previous bug in binpac flowbuffer frame length
+# boundary checks:
+
+# Incremental flowbuffer parsing sought to first parse the "minimum header
+# length" required to get the full frame length, possibly from a record
+# field, but generating the logic to parse that field could greedily
+# bundle in additional boundary-checks for all subsequent fields of
+# known-size.
+#
+# E.g. for flowunit parsing of this:
+#
+#     type HDR = record {
+#         version:    uint8;
+#         reserved:   uint8;
+#         len:        uint16;
+#     } &byteorder=bigendian;
+#
+#     type FOO_PDU(is_orig: bool) = record {
+#         hdr:        HDR;
+#         plen:       uint8;
+#         ptype:      uint8;
+#         something:  bytestring &restofdata;
+#     } &byteorder=bigendian, &length=hdr.len;
+
+# The flowbuffer was correctly seeking to buffer 4 bytes and parse the
+# "hdr.len" field, but the generated parsing logic for "hdr.len" included
+# a boundary check all the way up to include "plen" and "ptype".
+
+# This causes out-of-bounds exceptions to be thrown for inputs that should
+# actually be possible to incrementally parse via flowbuffer.
+
+# @TEST-EXEC: ${DIST}/aux/zeek-aux/plugin-support/init-plugin -u . Foo FOO
+# @TEST-EXEC: cp -r %DIR/binpac-flowbuffer-frame-length-plugin/* .
+# @TEST-EXEC: ./configure --zeek-dist=${DIST} && make
+# @TEST-EXEC: ZEEK_PLUGIN_PATH=`pwd` zeek -r $TRACES/mmsX.pcap %INPUT >output
+
+event Foo::foo_message(c: connection, is_orig: bool, len: count, plen: count, ptype: count)
+	{
+	print "foo_message", len, plen, ptype;
+	}