bplist: Fix OOB write on heap buffer and improve recursion check
Issue #92 pointed out an problem with (invalid) bplist files which have
exactly one structured node whose subnode reference itself.
The recursion check used a fixed size array with the size of the total number
of objects. In this case the number of objects is 1 but the recursion check
code wanted to set the node_index for the level 1 which leads to an OOB write
on the heap. This commit fixes/improves two things:
1) Prevent OOB write by using a dynamic data storage for the used node
indexes (plist_t of type PLIST_ARRAY)
2) Reduces the memory usage of large binary plists, because not the total
number of nodes in the binary plist, but the number of recursion levels
is important for the recursion check.
diff --git a/src/bplist.c b/src/bplist.c
index 0cfe5fe..d2a1ccb 100644
--- a/src/bplist.c
+++ b/src/bplist.c
@@ -187,7 +187,7 @@
uint8_t offset_size;
const char* offset_table;
uint32_t level;
- uint32_t *used_indexes;
+ plist_t used_indexes;
};
static plist_t parse_bin_node_at_index(struct bplist_data *bplist, uint32_t node_index);
@@ -645,11 +645,20 @@
}
/* store node_index for current recursion level */
- bplist->used_indexes[bplist->level] = node_index;
+ if (plist_array_get_size(bplist->used_indexes) < bplist->level+1) {
+ while (plist_array_get_size(bplist->used_indexes) < bplist->level+1) {
+ plist_array_append_item(bplist->used_indexes, plist_new_uint(node_index));
+ }
+ } else {
+ plist_array_set_item(bplist->used_indexes, plist_new_uint(node_index), bplist->level);
+ }
+
/* recursion check */
if (bplist->level > 0) {
for (i = bplist->level-1; i >= 0; i--) {
- if (bplist->used_indexes[i] == bplist->used_indexes[bplist->level]) {
+ plist_t node_i = plist_array_get_item(bplist->used_indexes, i);
+ plist_t node_level = plist_array_get_item(bplist->used_indexes, bplist->level);
+ if (plist_compare_node_value(node_i, node_level)) {
fprintf(stderr, "Recursion detected in binary plist. Aborting.\n");
return NULL;
}
@@ -709,9 +718,6 @@
if (offset_table + num_objects * offset_size >= plist_bin + length)
return;
- if (sizeof(uint32_t) * num_objects < num_objects)
- return;
-
struct bplist_data bplist;
bplist.data = plist_bin;
bplist.size = length;
@@ -720,14 +726,14 @@
bplist.offset_size = offset_size;
bplist.offset_table = offset_table;
bplist.level = 0;
- bplist.used_indexes = (uint32_t*)malloc(sizeof(uint32_t) * num_objects);
+ bplist.used_indexes = plist_new_array();
if (!bplist.used_indexes)
return;
*plist = parse_bin_node_at_index(&bplist, root_object);
- free(bplist.used_indexes);
+ plist_free(bplist.used_indexes);
}
static unsigned int plist_data_hash(const void* key)
