Re: Hui Lin_DNP3 analyzer not working in current version of zeek

On Mon, Jun 17, 2019 at 9:19 PM Hui Lin (Hugo) <[email protected]> wrote:

> Back then when I had implemented it, I use a common record type, Request_Data_Object, to hide those differences. In the READ request, there still should be Request_Data_Object[8]. However, according to the value of the function code (which is the input of the Request_Data_Object), the size of each Request_Data_Object becomes 0 if it is READ request. Even though there are 8 objects, but each of them has 0 bytes, so totally there are no data coming. That is why actually there are no more data coming, which is not a mistake. I guess that it is probably how binpac changes the way to handle this type of situation now, making the analyzer fail to work.

Based on that description, I attached a patch that's possibly less
naive in case it helps give a good starting point for a proper fix.

An "array of empty objects" does (at first) seem like something that
should work, and it may have worked before (possibly for the wrong
reasons), but I think the current behavior of assuming array elements
have a minimum size of 1-byte is Good "for security reasons".
Specifically, DoS vulnerabilities become trivial when allowing for an
"array of empty objects".  For example:

type Message = record {
  flag: uint8;
  num: uint32;
  objs: Object(flag)[num];
};

type Object(flag: uint8) = case flag of {
  true -> empty;
  false -> uint8;
};

There, we don't statically know the size of an Object, so have to
parse each one, and a person can easily set "num" to 4 billion, not
actually have to send 4 billion bytes to back it up because they
intend for the Objects to all be empty, but yet leave us chugging away
for 4 billion iterations parsing out empty Objects.

> I probably will do that on July as I am catching a deadline on July 1st.

Thanks for reporting the issue and offering to take a look, let me
know what you come up with.

- Jon

_______________________________________________
Zeek mailing list
[email protected]
http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/zeek

diff --git a/src/analyzer/protocol/dnp3/dnp3-protocol.pac b/src/analyzer/protocol/dnp3/dnp3-protocol.pac
index 154fdc8be..5202cfb1f 100644
--- a/src/analyzer/protocol/dnp3/dnp3-protocol.pac
+++ b/src/analyzer/protocol/dnp3/dnp3-protocol.pac
@@ -88,14 +88,23 @@ type DNP3_Application_Response_Header = record {
 	internal_indications : uint16;
 } &length = 4;
 
-type Request_Objects(function_code: uint8) = record {
-	object_header: Object_Header(function_code);
+type NonEmptyRequestObjects(function_code: uint8, object_header: Object_Header) = record {
 	data: case (object_header.object_type_field) of {
 		0x0c03 -> bocmd_PM: Request_Data_Object(function_code, object_header.qualifier_field, object_header.object_type_field )[ ( object_header.number_of_item / 8 ) + 1*( object_header.number_of_item > ( (object_header.number_of_item / 8)*8 ) ) ];
 		0x3202 -> time_interval_ojbects: Request_Data_Object(function_code, object_header.qualifier_field, object_header.object_type_field )[ object_header.number_of_item];
 						# &check( object_header.qualifier_field == 0x0f && object_header.number_of_item == 0x01);
 		default -> ojbects: Request_Data_Object(function_code, object_header.qualifier_field, object_header.object_type_field )[ object_header.number_of_item];
 	};
+};
+
+type Request_Objects(function_code: uint8) = record {
+	object_header: Object_Header(function_code);
+
+	opt_objs: case ( function_code ) of {
+		READ    -> no_objs: empty;
+		default -> data: NonEmptyRequestObjects(function_code, object_header);
+	};
+
 	# dump_data is always empty; I intend to use it for checking some conditions;
 	# However, in the current binpac implementation, &check is not implemented
 	dump_data: case (function_code) of {