Nicolas Iooss <nicolas.iooss@xxxxxxx> writes:
> On Mon, Feb 8, 2021 at 5:23 PM James Carter <jwcart2@xxxxxxxxx> wrote:
>>
>> Nicolas Iooss reports:
>> I am continuing to investigate OSS-Fuzz crashes and the following one
>> is quite complex. Here is a CIL policy which triggers a
>> heap-use-after-free error in the CIL compiler:
>>
>> (class CLASS (PERM2))
>> (classorder (CLASS))
>> (classpermission CLSPRM)
>> (optional o
>> (mlsvalidatetrans x (domby l1 h1))
>> (common CLSCOMMON (PERM1))
>> (classcommon CLASS CLSCOMMON)
>> )
>> (classpermissionset CLSPRM (CLASS (PERM1)))
>>
>> The issue is that the mlsvalidatetrans fails to resolve in pass
>> CIL_PASS_MISC3, which comes after the resolution of classcommon (in
>> pass CIL_PASS_MISC2). So:
>>
>> * In pass CIL_PASS_MISC2, the optional block still exists, the
>> classcommon is resolved and class CLASS is linked with common
>> CLSCOMMON.
>> * In pass CIL_PASS_MISC3, the optional block is destroyed, including
>> the common CLSCOMMON.
>> * When classpermissionset is resolved, function cil_resolve_classperms
>> uses "common_symtab = &class->common->perms;", which has been freed.
>> The use-after-free issue occurs in __cil_resolve_perms (in
>> libsepol/cil/src/cil_resolve_ast.c):
>>
>> // common_symtab was freed
>> rc = cil_symtab_get_datum(common_symtab, curr->data, &perm_datum);
>>
>> The fundamental problem here is that when the optional block is
>> disabled it is immediately destroyed in the middle of the pass, so
>> the class has not been reset and still refers to the now destroyed
>> common when the classpermissionset is resolved later in the same pass.
>>
>> Added a list, disabled_optionals, to struct cil_args_resolve which is
>> passed when resolving the tree. When optionals are disabled, they are
>> now added to this list and then are destroyed after the tree has been
>> reset between passes.
>>
>> Reported-by: Nicolas Iooss <nicolas.iooss@xxxxxxx>
>> Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
>
> I confirm the patch fixes the issue.
>
> Acked-by: Nicolas Iooss <nicolas.iooss@xxxxxxx>
>
This is merged now.
> Thanks!
> Nicolas
>
>> ---
>> libsepol/cil/src/cil_resolve_ast.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
>> index 0c85eabe..49a5d139 100644
>> --- a/libsepol/cil/src/cil_resolve_ast.c
>> +++ b/libsepol/cil/src/cil_resolve_ast.c
>> @@ -51,6 +51,7 @@ struct cil_args_resolve {
>> struct cil_db *db;
>> enum cil_pass pass;
>> uint32_t *changed;
>> + struct cil_list *disabled_optionals;
>> struct cil_tree_node *optstack;
>> struct cil_tree_node *boolif;
>> struct cil_tree_node *macro;
>> @@ -3863,7 +3864,7 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
>>
>> if (((struct cil_optional *)parent->data)->enabled == CIL_FALSE) {
>> *(args->changed) = CIL_TRUE;
>> - cil_tree_children_destroy(parent);
>> + cil_list_append(args->disabled_optionals, CIL_NODE, parent);
>> }
>>
>> /* pop off the stack */
>> @@ -3926,6 +3927,7 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
>> extra_args.in_list = NULL;
>> extra_args.blockstack = NULL;
>>
>> + cil_list_init(&extra_args.disabled_optionals, CIL_NODE);
>> cil_list_init(&extra_args.sidorder_lists, CIL_LIST_ITEM);
>> cil_list_init(&extra_args.classorder_lists, CIL_LIST_ITEM);
>> cil_list_init(&extra_args.unordered_classorder_lists, CIL_LIST_ITEM);
>> @@ -3993,6 +3995,7 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
>> }
>>
>> if (changed && (pass > CIL_PASS_CALL1)) {
>> + struct cil_list_item *item;
>> /* Need to re-resolve because an optional was disabled that contained
>> * one or more declarations. We only need to reset to the call1 pass
>> * because things done in the preceding passes aren't allowed in
>> @@ -4021,6 +4024,11 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
>> cil_log(CIL_ERR, "Failed to reset declarations\n");
>> goto exit;
>> }
>> + cil_list_for_each(item, extra_args.disabled_optionals) {
>> + cil_tree_children_destroy(item->data);
>> + }
>> + cil_list_destroy(&extra_args.disabled_optionals, CIL_FALSE);
>> + cil_list_init(&extra_args.disabled_optionals, CIL_NODE);
>> }
>>
>> /* reset the arguments */
>> @@ -4049,6 +4057,7 @@ exit:
>> __cil_ordered_lists_destroy(&extra_args.catorder_lists);
>> __cil_ordered_lists_destroy(&extra_args.sensitivityorder_lists);
>> __cil_ordered_lists_destroy(&extra_args.unordered_classorder_lists);
>> + cil_list_destroy(&extra_args.disabled_optionals, CIL_FALSE);
>> cil_list_destroy(&extra_args.in_list, CIL_FALSE);
>>
>> return rc;
>> --
>> 2.26.2
>>
