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>
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
>
