Hui Lin_DNP3 analyzer not working in current version of zeek

Hi

It seemed that the DNP3 analyzer is not working properly in the current version of zeek. I have a pcap file containing around 400 DNP3 read requests and responses. I have included a print message in both “dnp3_application_request_header” and “dnp3_application_response_header” event handlers, but only two messages are print out for the request packets. Actually for the same pcap, in a version that I git last year, bro works fine by printing all messages. Any idea what happens? If needed, I can provide the pcap for the testing.

Thank you and best regards,

Hui Lin

Not sure, please either try to debug / explore the diffs, or provide a
pcap and say which was the last known working version.

- Jon

Found the difference:

This behavior changed in Bro 2.5.4 (really what changed is in BinPAC
0.49, not Bro), but the new parsing behavior is legitimate. The old
behavior just caused broken protocol grammars to possibly parse more
things than they should have, such as in cases where there wasn't enough
data to fill an array. So it appeared to be working for you, but it
was not.

In this case, the DNP3 protocol grammar we use is either incomplete or
needs a further fix. With some debugging for this example pcap, you can
see where the parsing is failing:

protocol violation, [orig_h=10.0.0.3, orig_p=37147/tcp,
resp_h=10.0.0.1, resp_p=20000/tcp], Analyzer::ANALYZER_DNP3_TCP,
Binpac exception: binpac exception: out_of_bound:
Request_Objects:ojbects: 8 > 0
protocol violation, [orig_h=10.0.0.3, orig_p=55021/tcp,
resp_h=10.0.0.2, resp_p=20000/tcp], Analyzer::ANALYZER_DNP3_TCP,
Binpac exception: binpac exception: out_of_bound:
Request_Objects:ojbects: 8 > 0

That's here:

You can look in the pcap (e.g. Wireshark) and see in the first READ
request that there's no objects being sent for us to parse even though
our protocol definition is written to expect that. So that protocol
violation is legit in the sense that we've defined the protocol in a way
that differs from what's being sent on the wire. And a protocol
violation in the case of DNP3 disables all further analysis.

You maybe understand DNP3 better than me, so please create an issue
or pull request if you come up with a fix that improves the DNP3 parser.
Attached is a naive patch that seems to generate the same number of
requests/responses as before Bro 2.5.4; maybe it helps as a
starting point or reference.

- Jon

dnp3-naive.patch (1.2 KB)

Hi Jon,

This is a very confusing part of DNP3 protocol, but only in the request. The “number_of_item” obtained from “object_header” in the request is mainly applied to the response, telling outstation that this is the number of items that you should include in the response. But there are a few exceptions where the request is also using the “number_of_item”, especially the control operations, meaning that these are the operations that we would like to apply to the outstation.

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.

For me, the workaround should not be difficult (also I am pretty sure that Robin and Seth would like to keep the binpac works as it is now). As there are only a very few exceptions, I just include those exceptions directly there (similar to what you did in the patch), and then default most other requests to empty objects.

I probably will do that on July as I am catching a deadline on July 1st. I will contact you privately as I may need you to help me to input a different public key for upload as I have already graduated.

Best,

Hui Lin

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

dnp3-less-naive.patch (1.69 KB)