Bug #2425
closedDNP3 memcpy buffer overflow
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
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!
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}};
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.
Updated by Philippe Antoine almost 7 years ago
- File p2425.diff p2425.diff added
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
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!
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
- File issue-2425-4.0.x.tar issue-2425-4.0.x.tar added
- File issue-2425-master.tar issue-2425-master.tar added
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.
Updated by Philippe Antoine over 6 years ago
Hi everybody, when can I expect the patch release ?
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.
Updated by Victor Julien over 6 years ago
- Target version changed from 4.0.4 to 4.1beta1
Updated by Victor Julien over 6 years ago
- Status changed from Assigned to Closed
- Private changed from Yes to No
Fixes merged into master.