* [PATCH] libsepol/cil: Destroy disabled optional blocks after pass is complete
@ 2021-02-08 16:23 James Carter
2021-02-16 20:57 ` Nicolas Iooss
0 siblings, 1 reply; 3+ messages in thread
From: James Carter @ 2021-02-08 16:23 UTC (permalink / raw)
To: selinux; +Cc: nicolas.iooss, James Carter
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@m4x.org>
Signed-off-by: James Carter <jwcart2@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] libsepol/cil: Destroy disabled optional blocks after pass is complete
2021-02-08 16:23 [PATCH] libsepol/cil: Destroy disabled optional blocks after pass is complete James Carter
@ 2021-02-16 20:57 ` Nicolas Iooss
2021-02-17 17:28 ` Petr Lautrbach
0 siblings, 1 reply; 3+ messages in thread
From: Nicolas Iooss @ 2021-02-16 20:57 UTC (permalink / raw)
To: James Carter; +Cc: SElinux list
On Mon, Feb 8, 2021 at 5:23 PM James Carter <jwcart2@gmail.com> 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@m4x.org>
> Signed-off-by: James Carter <jwcart2@gmail.com>
I confirm the patch fixes the issue.
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] libsepol/cil: Destroy disabled optional blocks after pass is complete
2021-02-16 20:57 ` Nicolas Iooss
@ 2021-02-17 17:28 ` Petr Lautrbach
0 siblings, 0 replies; 3+ messages in thread
From: Petr Lautrbach @ 2021-02-17 17:28 UTC (permalink / raw)
To: SElinux list; +Cc: Nicolas Iooss, James Carter
Nicolas Iooss <nicolas.iooss@m4x.org> writes:
> On Mon, Feb 8, 2021 at 5:23 PM James Carter <jwcart2@gmail.com> 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@m4x.org>
>> Signed-off-by: James Carter <jwcart2@gmail.com>
>
> I confirm the patch fixes the issue.
>
> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
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
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-17 17:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-08 16:23 [PATCH] libsepol/cil: Destroy disabled optional blocks after pass is complete James Carter
2021-02-16 20:57 ` Nicolas Iooss
2021-02-17 17:28 ` Petr Lautrbach
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox