| Message ID | 20210131231240.58838-1-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libsepol/cil: fix heap-use-after-free when using optional blockinherit | expand |
On Sun, Jan 31, 2021 at 6:14 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > 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). > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> The patch is good, but I prefer the work to be done in cil_destroy_blockinherit(), so I just sent out a patch based on this one to do that. Jim > --- > libsepol/cil/src/cil_tree.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c > index 886412d1b974..72c4892de707 100644 > --- a/libsepol/cil/src/cil_tree.c > +++ b/libsepol/cil/src/cil_tree.c > @@ -223,11 +223,20 @@ void cil_tree_node_init(struct cil_tree_node **node) > void cil_tree_node_destroy(struct cil_tree_node **node) > { > struct cil_symtab_datum *datum; > + struct cil_blockinherit *inherit; > > if (node == NULL || *node == NULL) { > return; > } > > + if ((*node)->flavor == CIL_BLOCKINHERIT) { > + inherit = (*node)->data; > + if (inherit->block != NULL && inherit->block->bi_nodes != NULL) { > + // Unlink the blockinherit from the block > + cil_list_remove(inherit->block->bi_nodes, CIL_NODE, *node, CIL_FALSE); > + } > + } > + > if ((*node)->flavor >= CIL_MIN_DECLARATIVE) { > datum = (*node)->data; > cil_symtab_datum_remove_node(datum, *node); > -- > 2.30.0 >
diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c index 886412d1b974..72c4892de707 100644 --- a/libsepol/cil/src/cil_tree.c +++ b/libsepol/cil/src/cil_tree.c @@ -223,11 +223,20 @@ void cil_tree_node_init(struct cil_tree_node **node) void cil_tree_node_destroy(struct cil_tree_node **node) { struct cil_symtab_datum *datum; + struct cil_blockinherit *inherit; if (node == NULL || *node == NULL) { return; } + if ((*node)->flavor == CIL_BLOCKINHERIT) { + inherit = (*node)->data; + if (inherit->block != NULL && inherit->block->bi_nodes != NULL) { + // Unlink the blockinherit from the block + cil_list_remove(inherit->block->bi_nodes, CIL_NODE, *node, CIL_FALSE); + } + } + if ((*node)->flavor >= CIL_MIN_DECLARATIVE) { datum = (*node)->data; cil_symtab_datum_remove_node(datum, *node);
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). Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> --- libsepol/cil/src/cil_tree.c | 9 +++++++++ 1 file changed, 9 insertions(+)