| Message ID | 20210205094539.388854-2-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] libsepol/cil: fix NULL pointer dereference with empty macro argument | expand |
On Fri, Feb 5, 2021 at 4:54 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > OSS-Fuzz found a Null-dereference READ in the CIL compiler when trying > to compile the following policy: > > (<src_info>) > > In cil_gen_src_info(), parse_current->next is NULL even though the code > expects that both parse_current->next and parse_current->next->next > exists. > > Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28457 > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> This is an interesting case. All of the <SOMETHING> are generated internally. I never even thought about what would happen if they actually appeared in the policy. I'll have to think about what the best way to handle this is. Your patch works fine, but it might be better to check for and reject these in the parser. Jim > --- > libsepol/cil/src/cil_build_ast.c | 5 +++++ > libsepol/cil/src/cil_tree.c | 2 +- > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c > index 5094d62e2238..726f46cd478d 100644 > --- a/libsepol/cil/src/cil_build_ast.c > +++ b/libsepol/cil/src/cil_build_ast.c > @@ -6070,6 +6070,11 @@ int cil_gen_src_info(struct cil_tree_node *parse_current, struct cil_tree_node * > /* No need to check syntax, because this is auto generated */ > struct cil_src_info *info = NULL; > > + if (parse_current->next == NULL || parse_current->next->next == NULL) { > + cil_tree_log(parse_current, CIL_ERR, "Bad <src_info>"); > + return SEPOL_ERR; > + } > + > cil_src_info_init(&info); > > info->is_cil = (parse_current->next->data == CIL_KEY_SRC_CIL) ? CIL_TRUE : CIL_FALSE; > diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c > index 886412d1b974..3da972e9cf4e 100644 > --- a/libsepol/cil/src/cil_tree.c > +++ b/libsepol/cil/src/cil_tree.c > @@ -69,7 +69,7 @@ struct cil_tree_node *cil_tree_get_next_path(struct cil_tree_node *node, char ** > > while (node) { > if (node->flavor == CIL_NODE && node->data == NULL) { > - if (node->cl_head->data == CIL_KEY_SRC_INFO) { > + if (node->cl_head->data == CIL_KEY_SRC_INFO && node->cl_head->next != NULL && node->cl_head->next->next != NULL) { > /* Parse Tree */ > *path = node->cl_head->next->next->data; > *is_cil = (node->cl_head->next->data == CIL_KEY_SRC_CIL); > -- > 2.30.0 >
On Fri, Feb 5, 2021 at 5:02 PM James Carter <jwcart2@gmail.com> wrote: > > On Fri, Feb 5, 2021 at 4:54 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > > > OSS-Fuzz found a Null-dereference READ in the CIL compiler when trying > > to compile the following policy: > > > > (<src_info>) > > > > In cil_gen_src_info(), parse_current->next is NULL even though the code > > expects that both parse_current->next and parse_current->next->next > > exists. > > > > Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28457 > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> > > This is an interesting case. All of the <SOMETHING> are generated > internally. I never even thought about what would happen if they > actually appeared in the policy. I'll have to think about what the > best way to handle this is. Your patch works fine, but it might be > better to check for and reject these in the parser. > > Jim > Eventually, I would like to reject these in the parser, but this patch is fine. Acked-by: James Carter <jwcart2@gmail.com> Applied. Thanks, Jim > > --- > > libsepol/cil/src/cil_build_ast.c | 5 +++++ > > libsepol/cil/src/cil_tree.c | 2 +- > > 2 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c > > index 5094d62e2238..726f46cd478d 100644 > > --- a/libsepol/cil/src/cil_build_ast.c > > +++ b/libsepol/cil/src/cil_build_ast.c > > @@ -6070,6 +6070,11 @@ int cil_gen_src_info(struct cil_tree_node *parse_current, struct cil_tree_node * > > /* No need to check syntax, because this is auto generated */ > > struct cil_src_info *info = NULL; > > > > + if (parse_current->next == NULL || parse_current->next->next == NULL) { > > + cil_tree_log(parse_current, CIL_ERR, "Bad <src_info>"); > > + return SEPOL_ERR; > > + } > > + > > cil_src_info_init(&info); > > > > info->is_cil = (parse_current->next->data == CIL_KEY_SRC_CIL) ? CIL_TRUE : CIL_FALSE; > > diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c > > index 886412d1b974..3da972e9cf4e 100644 > > --- a/libsepol/cil/src/cil_tree.c > > +++ b/libsepol/cil/src/cil_tree.c > > @@ -69,7 +69,7 @@ struct cil_tree_node *cil_tree_get_next_path(struct cil_tree_node *node, char ** > > > > while (node) { > > if (node->flavor == CIL_NODE && node->data == NULL) { > > - if (node->cl_head->data == CIL_KEY_SRC_INFO) { > > + if (node->cl_head->data == CIL_KEY_SRC_INFO && node->cl_head->next != NULL && node->cl_head->next->next != NULL) { > > /* Parse Tree */ > > *path = node->cl_head->next->next->data; > > *is_cil = (node->cl_head->next->data == CIL_KEY_SRC_CIL); > > -- > > 2.30.0 > >
diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c index 5094d62e2238..726f46cd478d 100644 --- a/libsepol/cil/src/cil_build_ast.c +++ b/libsepol/cil/src/cil_build_ast.c @@ -6070,6 +6070,11 @@ int cil_gen_src_info(struct cil_tree_node *parse_current, struct cil_tree_node * /* No need to check syntax, because this is auto generated */ struct cil_src_info *info = NULL; + if (parse_current->next == NULL || parse_current->next->next == NULL) { + cil_tree_log(parse_current, CIL_ERR, "Bad <src_info>"); + return SEPOL_ERR; + } + cil_src_info_init(&info); info->is_cil = (parse_current->next->data == CIL_KEY_SRC_CIL) ? CIL_TRUE : CIL_FALSE; diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c index 886412d1b974..3da972e9cf4e 100644 --- a/libsepol/cil/src/cil_tree.c +++ b/libsepol/cil/src/cil_tree.c @@ -69,7 +69,7 @@ struct cil_tree_node *cil_tree_get_next_path(struct cil_tree_node *node, char ** while (node) { if (node->flavor == CIL_NODE && node->data == NULL) { - if (node->cl_head->data == CIL_KEY_SRC_INFO) { + if (node->cl_head->data == CIL_KEY_SRC_INFO && node->cl_head->next != NULL && node->cl_head->next->next != NULL) { /* Parse Tree */ *path = node->cl_head->next->next->data; *is_cil = (node->cl_head->next->data == CIL_KEY_SRC_CIL);
OSS-Fuzz found a Null-dereference READ in the CIL compiler when trying to compile the following policy: (<src_info>) In cil_gen_src_info(), parse_current->next is NULL even though the code expects that both parse_current->next and parse_current->next->next exists. Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28457 Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> --- libsepol/cil/src/cil_build_ast.c | 5 +++++ libsepol/cil/src/cil_tree.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)