Re: [PATCH V2] libsepol/cil: Fix heap-use-after-free when using optional blockinherit

[Petr Lautrbach <plautrba@redhat.com>]

Nicolas Iooss <nicolas.iooss@m4x.org> writes:

> On Tue, Feb 2, 2021 at 9:54 PM James Carter <jwcart2@gmail.com> wrote:
>>
>> This is based on a patch by Nicolas Iooss. He writes:
>>     When secilc compiles the following policy:
>>
>>         (block b1
>>             (optional o1
>>                 (blockinherit b1)
>>                 (blockinherit x)
>>             )
>>         )
>>
>>     it disables the optional block at pass 3 (CIL_PASS_BLKIN_LINK)
>>     because the block "x" does not exist.
>>     __cil_resolve_ast_last_child_helper() calls
>>     cil_tree_children_destroy() on the optional block, which destroys
>>     the two blockinherit statements. But the (blockinherit b1) node
>>     was referenced inside (block b1) node, in its block->bi_nodes list.
>>     Therefore, when this list is used at pass 4 (CIL_PASS_BLKIN_COPY),
>>     it contains a node which was freed: this triggers a use-after-free
>>     issue
>>
>>     Fix this issue by removing blockinherit nodes from their lists of
>>     nodes block->bi_nodes when they are being destroyed. As
>>     cil_destroy_blockinherit() does not have a reference to the node
>>     containing the blockinherit data, implement this new logic in
>>     cil_tree_node_destroy().
>>
>>     This issue was found while investigating a testcase from an OSS-Fuzz
>>     issue which seems unrelated (a Null-dereference READ in
>>     cil_symtab_get_datum,
>>     https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29861).
>>
>> Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>> Signed-off-by: James Carter <jwcart2@gmail.com>
>
> I tested the patch and confirm it fixes the issue.
>
> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>

Merged and released in 3.2-rc2

Thanks!


>> ---
>>  libsepol/cil/src/cil_build_ast.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
>> index 02481558..ab0efd53 100644
>> --- a/libsepol/cil/src/cil_build_ast.c
>> +++ b/libsepol/cil/src/cil_build_ast.c
>> @@ -283,6 +283,19 @@ void cil_destroy_blockinherit(struct cil_blockinherit *inherit)
>>                 return;
>>         }
>>
>> +       if (inherit->block != NULL && inherit->block->bi_nodes != NULL) {
>> +               struct cil_tree_node *node;
>> +               struct cil_list_item *item;
>> +
>> +               cil_list_for_each(item, inherit->block->bi_nodes) {
>> +                       node = item->data;
>> +                       if (node->data == inherit) {
>> +                               cil_list_remove(inherit->block->bi_nodes, CIL_NODE, node, CIL_FALSE);
>> +                               break;
>> +                       }
>> +               }
>> +       }
>> +
>>         free(inherit);
>>  }
>>
>> --
>> 2.26.2
>>