Project

General

Profile

Actions

Bug #2425

closed

DNP3 memcpy buffer overflow

Added by Philippe Antoine almost 7 years ago. Updated over 6 years ago.

Status:
Closed
Priority:
High
Assignee:
Target version:
Affected Versions:
Effort:
Difficulty:
Label:

Description

In the file in src/app-layer-dnp3-objects.c, there are buffer overflows due to memcpy executions after checks on the written-to buffer, but not the read from buffer.

For instance in function DNP3DecodeObjectG70V3, we have the following code

        if (!DNP3ReadUint16(buf, len, &object->filename_size)) {
            goto error;
        }
       //other DNP3ReadUintXX...
        if (object->filename_size > 0) {
            memcpy(object->filename, *buf, object->filename_size);
            *buf += object->filename_size;
            *len -= object->filename_size;
        }

The check is missing on buffer *buf length (ie *len) versus object->filename_size (which comes from the packet).

The error comes form the python script generating the code.
Patch should be like

diff --git a/scripts/dnp3-gen/dnp3-gen.py b/scripts/dnp3-gen/dnp3-gen.py
index cc2aa8fc..f0bf1c0a 100755
--- a/scripts/dnp3-gen/dnp3-gen.py
+++ b/scripts/dnp3-gen/dnp3-gen.py
@@ -186,6 +186,10 @@ void OutputJsonDNP3SetItem(json_t *js, DNP3Object *object,
             json_object_set_new(js, "data->{{field.name}}", json_string(data->{{field.name}}));
 {% elif field.type == "chararray" %}
             if (data->{{field.len_field}} > 0) {
+                if (*len < data->{{field.len_field}}) {
+                    /* Not enough data. */
+                    goto error;
+                }
                 /* First create a null terminated string as not all versions
                  * of jansson have json_stringn. */
                 char tmpbuf[data->{{field.len_field}} + 1];
@@ -527,6 +531,10 @@ static int DNP3DecodeObjectG{{object.group}}V{{object.variation}}(const uint8_t
         object->{{field.len_field}} = prefix - (offset - *len);
 {% endif %}
         if (object->{{field.len_field}} > 0) {
+            if (*len < object->{{field.len_field}}) {
+                /* Not enough data. */
+                goto error;
+            }
             memcpy(object->{{field.name}}, *buf, object->{{field.len_field}});
             *buf += object->{{field.len_field}};
             *len -= object->{{field.len_field}};


Files

off.pcap (146 Bytes) off.pcap Pcap triggering the overflow with config file setting midstream:true Philippe Antoine, 01/25/2018 02:20 AM
p2425.diff (8.02 KB) p2425.diff Proposed patch Philippe Antoine, 01/25/2018 05:04 AM
0001-Fixes-Bug-2425.patch (7.34 KB) 0001-Fixes-Bug-2425.patch patch with git format-patch Philippe Antoine, 01/25/2018 10:03 AM
issue-2425-4.0.x.tar (20 KB) issue-2425-4.0.x.tar 4.0.x patch set. Jason Ish, 01/26/2018 04:23 PM
issue-2425-master.tar (20 KB) issue-2425-master.tar Master patch set. Jason Ish, 01/26/2018 04:23 PM
Actions #1

Updated by Victor Julien almost 7 years ago

  • Status changed from New to Assigned
  • Assignee set to Jason Ish
  • Priority changed from Normal to High
  • Target version set to 4.0.4
  • Private changed from No to Yes

Jason can you confirm this issue and do fixes for 4.0.x and master branches? Thanks!

Actions #2

Updated by Philippe Antoine almost 7 years ago

Sorry, the proposed patch was too much, the good one is


diff --git a/scripts/dnp3-gen/dnp3-gen.py b/scripts/dnp3-gen/dnp3-gen.py
index cc2aa8fc..0ebb39c3 100755
--- a/scripts/dnp3-gen/dnp3-gen.py
+++ b/scripts/dnp3-gen/dnp3-gen.py
@@ -527,6 +527,10 @@ static int DNP3DecodeObjectG{{object.group}}V{{object.variation}}(const uint8_t
         object->{{field.len_field}} = prefix - (offset - *len);
 {% endif %}
         if (object->{{field.len_field}} > 0) {
+            if (*len < object->{{field.len_field}}) {
+                /* Not enough data. */
+                goto error;
+            }
             memcpy(object->{{field.name}}, *buf, object->{{field.len_field}});
             *buf += object->{{field.len_field}};
             *len -= object->{{field.len_field}};
Actions #3

Updated by Victor Julien almost 7 years ago

Hi Philippe, thanks for your report. I've set the ticket to private as I think this is a security issue. Will make it public when a fixed version is released.

Actions #4

Updated by Philippe Antoine almost 7 years ago

Hi Victor and everybody,

Thanks.
I did not see the button to set the ticket as private.

So, I am not submitting the patch publicly with a GitHub pull request, but here.
To get it to work on my computer, I had also to modify dnp3-gen.py yo work with jinja 2.10
I do not know how to integrate the pcap to the tests...

Best regards

Actions #5

Updated by Jason Ish almost 7 years ago

Philippe, your patch looks correct. Would you want to prepare it in git and export it with format-patch so the commit author is yours?

Please don't include the updates to the jinja2 templating yet. I do not see why that is required. The current code works fine with Jinja 2.10 for me (using Python 2.7). And the namespace stuff is not in Jinja 2.9 which is still the version found on may distros right now.

Thanks!

Actions #6

Updated by Philippe Antoine almost 7 years ago

Hi Jason,

Here is the patch. I hope it is well formatted.

For jinja, I had the problem that shift was always 0 (for bit value extraction)
My quick patch was inspired by https://github.com/pallets/jinja/pull/684
But I am surely no jinja expert ;-) and this is not critical

Updated by Jason Ish almost 7 years ago

Here are patch sets for git master and 4.0.x.

@Philippe B: I did need the jinja2 patch. One of the commits in the patch set I altered to come from you for that credit. Please review.

Actions #8

Updated by Philippe Antoine almost 7 years ago

Looks good to me :-)

Actions #9

Updated by Philippe Antoine over 6 years ago

Hi everybody, when can I expect the patch release ?

Actions #10

Updated by Victor Julien over 6 years ago

4.0.4 is planned for Wednesday and this fix will be included. I will set the ticket to public post-release.

Actions #11

Updated by Victor Julien over 6 years ago

  • Target version changed from 4.0.4 to 4.1beta1
Actions #12

Updated by Victor Julien over 6 years ago

  • Status changed from Assigned to Closed
  • Private changed from Yes to No

Fixes merged into master.

Actions

Also available in: Atom PDF