dnp3: added sanity checks to avoid crashes in a case of malformed data

[ilya-bakhtin]

Make sure these boxes are signed before submitting your Pull Request -- thank you.

• I have read the contributing guide lines at https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing
• I have signed the Open Information Security Foundation contribution agreement at https://suricata-ids.org/about/contribution-agreement/
• I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

Link to redmine ticket:

Describe changes:

DNP3 parser code contains some unsafe pointer arithmetic. It basically works in a case of correct data; however if the input is malformed then it crashes.
The fix adds some sanity checks to avoid the processing of malformed/incomplete data.

PRScript output (if applicable):

#suricata-verify-pr:
#suricata-verify-repo:
#suricata-verify-branch:
#suricata-update-pr:
#suricata-update-repo:
#suricata-update-branch:
#libhtp-pr:
#libhtp-repo:
#libhtp-branch:

[catenacyber]

This is already tested at line 1060

[ilya-bakhtin]

No, it's not tested there since input_len is tested at 1060 but frame_len is passed to DNP3HandleUserDataRequest.
Well, probably frame_len is tested inside DNP3CalculateLinkLength so the test at 843 is not necessary.
Please see my reply to your second note with a complete explanation.

[catenacyber]

Indeed but line 1067 adds test if (input_len < frame_len) { after if (input_len < sizeof(DNP3LinkHeader)) {
So, this line 843 test looks redundant to me.

[ilya-bakhtin]

line 1067 forces waiting for the data if frame_len is bigger than input_len.
However, line 843 tests quite different condition, i.e. if frame_len is small (smaller than header length).
Well, I agree that 843 is redundant. But it's only because of DNP3CalculateLinkLength never returns value lesser than header length (or zero).
There is no other validation and 1067 would not do the trick if DNP3CalculateLinkLength could return any value.

[catenacyber]

Ok, I understand better what you mean.
DNP3CalculateLinkLength is indeed the good function checking that we have enough space in the frame (to use the name used in the source), even if we have more frames in the buffer.

One can use DEBUG_VALIDATE_BUG_ON to add these safety checks, even if it is redundant today

[ilya-bakhtin]

Well, after some analysis i found only one place where an additional check is mandatory.
It's at line 1035. I provided a unit test that demonstrates (tx->response_buffer_len < offset).
So (tx->response_buffer_len - offset) becomes extremely big and DNP3DecodeApplicationObjects iterates over block of memory with unexpected data. The behavior is undefined; in my case it often crashes with segfault.
Initially, working at a crash in a field, i did not expect the randomly filled data. So checks like "object->count = object->stop >= object->start ? object->stop - object->start + 1 : 0;" were added. Probably they are redundant if check at line 1035 is performed.
I did not have enough time to full research so i just added checks in all places where unsafe arithmetics is performed and the values of operands can not be estimated from several strings of code above :-)
Anyhow the check at line 1035 is mandatory. The unit test is absolutely synthetic, however we should not crash with any input data.

[catenacyber]

Thanks for the reproducer.
There is indeed a bug but it seems to me to be rather in DNP3HasUserData using the header when its caller like DNP3HandleResponseLinkLayer use implicitly the flow direction to check for IIN.

So, I would patch like this, to make DNP3HasUserData use the flow direction consistently with the rest of the code :

@@ -551,9 +556,9 @@ static int DNP3IsUserData(const DNP3LinkHeader *header)
  *
  * \retval 1 if user data exists, otherwise 0.
  */
-static int DNP3HasUserData(const DNP3LinkHeader *header)
+static int DNP3HasUserData(const DNP3LinkHeader *header, uint8_t direction)
 {
-    if (DNP3_LINK_DIR(header->control)) {
+    if (direction == STREAM_TOSERVER) {
         return header->len >= DNP3_LINK_HDR_LEN + sizeof(DNP3TransportHeader) +
             sizeof(DNP3ApplicationHeader);
     }
@@ -1076,7 +1081,7 @@ static int DNP3HandleRequestLinkLayer(DNP3State *dnp3, const uint8_t *input,
 
         /* Make sure the header length is large enough for transport and
          * application headers. */
-        if (!DNP3HasUserData(header)) {
+        if (!DNP3HasUserData(header, STREAM_TOSERVER)) {
             DNP3SetEvent(dnp3, DNP3_DECODER_EVENT_LEN_TOO_SMALL);
             goto next;
         }
@@ -1215,7 +1220,7 @@ static int DNP3HandleResponseLinkLayer(DNP3State *dnp3, const uint8_t *input,
 
         /* Make sure the header length is large enough for transport and
          * application headers. */
-        if (!DNP3HasUserData(header)) {
+        if (!DNP3HasUserData(header, STREAM_TOCLIENT)) {
             DNP3SetEvent(dnp3, DNP3_DECODER_EVENT_LEN_TOO_SMALL);
             goto error;
         }

What do you think ?

[catenacyber]

@ilya-bakhtin I compiled this fix with the probing parser direction fix and other things in #5093

[ilya-bakhtin]

I agree the root reason for the issue was inspecting a request on a response handling path.
So your latest changes look good. DNP3HasUserData behavior does not depend now on the data content and i'm happy :-)

[catenacyber]

Closing in favor of #5093