When merging file entries in process_file_entries(), an error condition can leave the entries list in a bad state which can cause invalid reads and/or double frees when freeing the codomain and entries list memory.
The problem comes from the need to sort the entries linked list. An array of pointers is created to represent the linked list, then the array is sorted, then the linked list and the array coexist while the entries are merged, then the linked list is reconstructed and the array is freed. While the entries are being merged, an error condition can occur and the function can return while the linked list is partially modified. The solution is to complete the sorting, reconstruct the linked list, and free the array immediately. Once the linked list is in a good state, the entries can be merged. Care is taken to adjust the linked list pointers as entries are merged. An error condition can occur but the linked list is always in a good state and proper cleanup can be performed without any memory access issues. Signed-off-by: Tyler Hicks <[email protected]> --- parser/parser_merge.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/parser/parser_merge.c b/parser/parser_merge.c index 04ff6df..bada358 100644 --- a/parser/parser_merge.c +++ b/parser/parser_merge.c @@ -101,44 +101,34 @@ static int process_file_entries(struct codomain *cod) qsort(table, count, sizeof(struct cod_entry *), file_comp); table[count] = NULL; - + for (n = 0; n < count; n++) + table[n]->next = table[n + 1]; + cod->entries = table[0]; + free(table); /* walk the sorted table merging similar entries */ - for (cur = table[0], next = table[1], n = 1; next != NULL; n++, next = table[n]) { + for (cur = cod->entries, next = cur->next; next != NULL; next = cur->next) { if (file_comp(&cur, &next) == 0) { /* check for merged x consistency */ if (!is_merged_x_consistent(cur->mode, next->mode)) { PERROR(_("profile %s: has merged rule %s with " "conflicting x modifiers\n"), cod->name, cur->name); - free(table); return 0; } //if (next->audit) //fprintf(stderr, "warning: merging rule 0x%x %s\n", next->audit, next->name); cur->mode |= next->mode; cur->audit |= next->audit; + cur->next = next->next; + next->next = NULL; free_cod_entries(next); - table[n] = NULL; } else { cur = next; } } - /* rebuild the file_entry chain */ - cur = table[0]; - for (n = 1; n < count; n++) { - if (table[n] != NULL) { - cur->next = table[n]; - cur = table[n]; - } - } - cur->next = NULL; - cod->entries = table[0]; - - free(table); - return 1; } -- 1.8.3.2 -- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
