* [PATCH 1/3] libsepol/cil: fix NULL pointer dereference with empty macro argument
@ 2021-02-05 9:45 Nicolas Iooss
2021-02-05 9:45 ` [PATCH 2/3] libsepol/cil: be more robust when encountering <src_info> Nicolas Iooss
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Nicolas Iooss @ 2021-02-05 9:45 UTC (permalink / raw)
To: selinux
OSS-Fuzz found a Null-dereference READ in the CIL compiler when trying
to compile the following policy:
(macro m((name n))) (call m(()))
When calling the macro, the name (in variable "pc") is NULL, which
triggers a NULL pointer dereference when using it as a key in
__cil_insert_name(). The stack trace is:
#0 0x7f4662655a85 in __strlen_avx2 (/usr/lib/libc.so.6+0x162a85)
#1 0x556d0b6d150c in __interceptor_strlen.part.0 (/selinux/libsepol/fuzz/fuzz-secilc+0x44850c)
#2 0x556d0ba74ed6 in symhash /selinux/libsepol/src/symtab.c:22:9
#3 0x556d0b9ef50d in hashtab_search /selinux/libsepol/src/hashtab.c:186:11
#4 0x556d0b928e1f in cil_symtab_get_datum /selinux/libsepol/src/../cil/src/cil_symtab.c:121:37
#5 0x556d0b8f28f4 in __cil_insert_name /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:96:2
#6 0x556d0b908184 in cil_resolve_call1 /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:2835:12
#7 0x556d0b91b404 in __cil_resolve_ast_node /selinux/libsepol/src/../cil/src/cil_resolve_ast.c
#8 0x556d0b91380f in __cil_resolve_ast_node_helper /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:3773:7
#9 0x556d0b932230 in cil_tree_walk_core /selinux/libsepol/src/../cil/src/cil_tree.c:263:9
#10 0x556d0b932230 in cil_tree_walk /selinux/libsepol/src/../cil/src/cil_tree.c:307:7
#11 0x556d0b932326 in cil_tree_walk_core /selinux/libsepol/src/../cil/src/cil_tree.c:275:9
#12 0x556d0b932326 in cil_tree_walk /selinux/libsepol/src/../cil/src/cil_tree.c:307:7
#13 0x556d0b911189 in cil_resolve_ast /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:3941:8
#14 0x556d0b798729 in cil_compile /selinux/libsepol/src/../cil/src/cil.c:550:7
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28544
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
libsepol/cil/src/cil_resolve_ast.c | 79 ++++++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 0c85eabe5a81..9300cd2be9be 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -2828,6 +2828,12 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
switch (flavor) {
case CIL_NAME: {
struct cil_name *name;
+ if (pc->data == NULL) {
+ cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
+ cil_destroy_args(new_arg);
+ rc = SEPOL_ERR;
+ goto exit;
+ }
name = __cil_insert_name(args->db, pc->data, current);
if (name != NULL) {
new_arg->arg = (struct cil_symtab_datum *)name;
@@ -2837,21 +2843,57 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
}
break;
case CIL_TYPE:
+ if (pc->data == NULL) {
+ cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
+ cil_destroy_args(new_arg);
+ rc = SEPOL_ERR;
+ goto exit;
+ }
new_arg->arg_str = pc->data;
break;
case CIL_ROLE:
+ if (pc->data == NULL) {
+ cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
+ cil_destroy_args(new_arg);
+ rc = SEPOL_ERR;
+ goto exit;
+ }
new_arg->arg_str = pc->data;
break;
case CIL_USER:
+ if (pc->data == NULL) {
+ cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
+ cil_destroy_args(new_arg);
+ rc = SEPOL_ERR;
+ goto exit;
+ }
new_arg->arg_str = pc->data;
break;
case CIL_SENS:
+ if (pc->data == NULL) {
+ cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
+ cil_destroy_args(new_arg);
+ rc = SEPOL_ERR;
+ goto exit;
+ }
new_arg->arg_str = pc->data;
break;
case CIL_CAT:
+ if (pc->data == NULL) {
+ cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
+ cil_destroy_args(new_arg);
+ rc = SEPOL_ERR;
+ goto exit;
+ }
new_arg->arg_str = pc->data;
break;
case CIL_BOOL:
+ if (pc->data == NULL) {
+ cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
+ cil_destroy_args(new_arg);
+ rc = SEPOL_ERR;
+ goto exit;
+ }
new_arg->arg_str = pc->data;
break;
case CIL_CATSET: {
@@ -2871,6 +2913,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
cil_list_append(((struct cil_symtab_datum*)catset)->nodes,
CIL_LIST_ITEM, cat_node);
new_arg->arg = (struct cil_symtab_datum*)catset;
+ } else if (pc->data == NULL) {
+ cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
+ cil_destroy_args(new_arg);
+ rc = SEPOL_ERR;
+ goto exit;
} else {
new_arg->arg_str = pc->data;
}
@@ -2896,6 +2943,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
cil_list_append(((struct cil_symtab_datum*)level)->nodes,
CIL_LIST_ITEM, lvl_node);
new_arg->arg = (struct cil_symtab_datum*)level;
+ } else if (pc->data == NULL) {
+ cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
+ cil_destroy_args(new_arg);
+ rc = SEPOL_ERR;
+ goto exit;
} else {
new_arg->arg_str = pc->data;
}
@@ -2921,6 +2973,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
cil_list_append(((struct cil_symtab_datum*)range)->nodes,
CIL_LIST_ITEM, range_node);
new_arg->arg = (struct cil_symtab_datum*)range;
+ } else if (pc->data == NULL) {
+ cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
+ cil_destroy_args(new_arg);
+ rc = SEPOL_ERR;
+ goto exit;
} else {
new_arg->arg_str = pc->data;
}
@@ -2946,6 +3003,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
cil_list_append(((struct cil_symtab_datum*)ipaddr)->nodes,
CIL_LIST_ITEM, addr_node);
new_arg->arg = (struct cil_symtab_datum*)ipaddr;
+ } else if (pc->data == NULL) {
+ cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
+ cil_destroy_args(new_arg);
+ rc = SEPOL_ERR;
+ goto exit;
} else {
new_arg->arg_str = pc->data;
}
@@ -2953,9 +3015,21 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
break;
}
case CIL_CLASS:
+ if (pc->data == NULL) {
+ cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
+ cil_destroy_args(new_arg);
+ rc = SEPOL_ERR;
+ goto exit;
+ }
new_arg->arg_str = pc->data;
break;
case CIL_MAP_CLASS:
+ if (pc->data == NULL) {
+ cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
+ cil_destroy_args(new_arg);
+ rc = SEPOL_ERR;
+ goto exit;
+ }
new_arg->arg_str = pc->data;
break;
case CIL_CLASSPERMISSION: {
@@ -2976,6 +3050,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
cp_node->data = cp;
cil_list_append(cp->datum.nodes, CIL_LIST_ITEM, cp_node);
new_arg->arg = (struct cil_symtab_datum*)cp;
+ } else if (pc->data == NULL) {
+ cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
+ cil_destroy_args(new_arg);
+ rc = SEPOL_ERR;
+ goto exit;
} else {
new_arg->arg_str = pc->data;
}
--
2.30.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] libsepol/cil: be more robust when encountering <src_info>
2021-02-05 9:45 [PATCH 1/3] libsepol/cil: fix NULL pointer dereference with empty macro argument Nicolas Iooss
@ 2021-02-05 9:45 ` Nicolas Iooss
2021-02-05 22:02 ` James Carter
2021-02-05 9:45 ` [PATCH 3/3] libsepol/cil: introduce intermediate cast to silence -Wvoid-pointer-to-enum-cast Nicolas Iooss
2021-02-05 21:59 ` [PATCH 1/3] libsepol/cil: fix NULL pointer dereference with empty macro argument James Carter
2 siblings, 1 reply; 9+ messages in thread
From: Nicolas Iooss @ 2021-02-05 9:45 UTC (permalink / raw)
To: selinux
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(-)
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] libsepol/cil: introduce intermediate cast to silence -Wvoid-pointer-to-enum-cast
2021-02-05 9:45 [PATCH 1/3] libsepol/cil: fix NULL pointer dereference with empty macro argument Nicolas Iooss
2021-02-05 9:45 ` [PATCH 2/3] libsepol/cil: be more robust when encountering <src_info> Nicolas Iooss
@ 2021-02-05 9:45 ` Nicolas Iooss
2021-02-05 22:02 ` James Carter
2021-02-05 21:59 ` [PATCH 1/3] libsepol/cil: fix NULL pointer dereference with empty macro argument James Carter
2 siblings, 1 reply; 9+ messages in thread
From: Nicolas Iooss @ 2021-02-05 9:45 UTC (permalink / raw)
To: selinux
clang 11.0.0 reports the following warning several times (when building
with "make CC=clang" from libsepol directory, in the default
configuration of the git repository):
../cil/src/cil_binary.c:1980:8: error: cast to smaller integer type
'enum cil_flavor' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast]
op = (enum cil_flavor)curr->data;
^~~~~~~~~~~~~~~~~~~~~~~~~~~
Silence this warning by casting the pointer to an integer the cast to
enum cil_flavor.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
libsepol/cil/src/cil_binary.c | 12 ++++++------
libsepol/cil/src/cil_policy.c | 12 ++++++------
libsepol/cil/src/cil_post.c | 2 +-
libsepol/cil/src/cil_resolve_ast.c | 2 +-
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index 7ba2098b61ea..f80d84679f85 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -1977,7 +1977,7 @@ static void __cil_expr_to_string(struct cil_list *expr, enum cil_flavor flavor,
curr = expr->head;
if (curr->flavor == CIL_OP) {
- op = (enum cil_flavor)curr->data;
+ op = (enum cil_flavor)(uintptr_t)curr->data;
if (op == CIL_ALL) {
*out = cil_strdup(CIL_KEY_ALL);
@@ -2076,7 +2076,7 @@ static int __cil_cond_expr_to_sepol_expr_helper(policydb_t *pdb, struct cil_list
if (item == NULL) {
goto exit;
} else if (item->flavor == CIL_OP) {
- enum cil_flavor cil_op = (enum cil_flavor)item->data;
+ enum cil_flavor cil_op = (enum cil_flavor)(uintptr_t)item->data;
op = cil_malloc(sizeof(*op));
op->bool = 0;
@@ -2562,7 +2562,7 @@ int __cil_constrain_expr_leaf_to_sepol_expr(policydb_t *pdb, const struct cil_db
struct cil_list_item *l_item = op_item->next;
struct cil_list_item *r_item = op_item->next->next;
- enum cil_flavor l_operand = (enum cil_flavor)l_item->data;
+ enum cil_flavor l_operand = (enum cil_flavor)(uintptr_t)l_item->data;
switch (l_operand) {
case CIL_CONS_U1:
@@ -2593,7 +2593,7 @@ int __cil_constrain_expr_leaf_to_sepol_expr(policydb_t *pdb, const struct cil_db
expr->attr = CEXPR_TYPE | CEXPR_XTARGET;
break;
case CIL_CONS_L1: {
- enum cil_flavor r_operand = (enum cil_flavor)r_item->data;
+ enum cil_flavor r_operand = (enum cil_flavor)(uintptr_t)r_item->data;
if (r_operand == CIL_CONS_L2) {
expr->attr = CEXPR_L1L2;
@@ -2608,7 +2608,7 @@ int __cil_constrain_expr_leaf_to_sepol_expr(policydb_t *pdb, const struct cil_db
expr->attr = CEXPR_L2H2;
break;
case CIL_CONS_H1: {
- enum cil_flavor r_operand = (enum cil_flavor)r_item->data;
+ enum cil_flavor r_operand = (enum cil_flavor)(uintptr_t)r_item->data;
if (r_operand == CIL_CONS_L2) {
expr->attr = CEXPR_H1L2;
} else {
@@ -2672,7 +2672,7 @@ int __cil_constrain_expr_to_sepol_expr_helper(policydb_t *pdb, const struct cil_
goto exit;
}
- enum cil_flavor cil_op = (enum cil_flavor)item->data;
+ enum cil_flavor cil_op = (enum cil_flavor)(uintptr_t)item->data;
switch (cil_op) {
case CIL_NOT:
op->expr_type = CEXPR_NOT;
diff --git a/libsepol/cil/src/cil_policy.c b/libsepol/cil/src/cil_policy.c
index 74edb34575ea..30d507f12626 100644
--- a/libsepol/cil/src/cil_policy.c
+++ b/libsepol/cil/src/cil_policy.c
@@ -285,7 +285,7 @@ static void cil_cond_expr_to_policy(FILE *out, struct cil_list *expr, int first)
struct cil_list_item *i1 = expr->head;
if (i1->flavor == CIL_OP) {
- enum cil_flavor op = (enum cil_flavor)i1->data;
+ enum cil_flavor op = (enum cil_flavor)(uintptr_t)i1->data;
fprintf(out, "(");
switch (op) {
case CIL_NOT:
@@ -385,7 +385,7 @@ static size_t __cil_cons_leaf_operand_len(struct cil_db *db, struct cil_list_ite
static size_t __cil_cons_leaf_op_len(struct cil_list_item *op)
{
- enum cil_flavor flavor = (enum cil_flavor)op->data;
+ enum cil_flavor flavor = (enum cil_flavor)(uintptr_t)op->data;
size_t len;
switch (flavor) {
@@ -420,7 +420,7 @@ static size_t cil_cons_expr_len(struct cil_db *db, struct cil_list *cons_expr)
i1 = cons_expr->head;
- op = (enum cil_flavor)i1->data;
+ op = (enum cil_flavor)(uintptr_t)i1->data;
switch (op) {
case CIL_NOT:
len = 6; /* "(not )" */
@@ -472,7 +472,7 @@ static char *__cil_cons_leaf_operand_to_string(struct cil_db *db, struct cil_lis
size_t o_len;
if (flavor == CIL_CONS_OPERAND) {
- enum cil_flavor o_flavor = (enum cil_flavor)operand->data;
+ enum cil_flavor o_flavor = (enum cil_flavor)(uintptr_t)operand->data;
switch (o_flavor) {
case CIL_CONS_U1:
o_str = "u1";
@@ -555,7 +555,7 @@ static char *__cil_cons_leaf_operand_to_string(struct cil_db *db, struct cil_lis
static char *__cil_cons_leaf_op_to_string(struct cil_list_item *op, char *new)
{
- enum cil_flavor flavor = (enum cil_flavor)op->data;
+ enum cil_flavor flavor = (enum cil_flavor)(uintptr_t)op->data;
const char *op_str;
size_t len;
@@ -599,7 +599,7 @@ static char *__cil_cons_expr_to_string(struct cil_db *db, struct cil_list *cons_
i1 = cons_expr->head;
- op = (enum cil_flavor)i1->data;
+ op = (enum cil_flavor)(uintptr_t)i1->data;
switch (op) {
case CIL_NOT:
*new++ = '(';
diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
index 37a444157853..a55df1ea5bb0 100644
--- a/libsepol/cil/src/cil_post.c
+++ b/libsepol/cil/src/cil_post.c
@@ -1301,7 +1301,7 @@ static int __cil_expr_to_bitmap(struct cil_list *expr, ebitmap_t *out, int max,
flavor = expr->flavor;
if (curr->flavor == CIL_OP) {
- enum cil_flavor op = (enum cil_flavor)curr->data;
+ enum cil_flavor op = (enum cil_flavor)(uintptr_t)curr->data;
if (op == CIL_ALL) {
ebitmap_init(&b1); /* all zeros */
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 9300cd2be9be..208bc01a7a2d 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -3355,7 +3355,7 @@ static int __cil_evaluate_tunable_expr(struct cil_list_item *curr)
return CIL_FALSE;
} else if (curr->flavor == CIL_OP) {
uint16_t v1, v2;
- enum cil_flavor op_flavor = (enum cil_flavor)curr->data;
+ enum cil_flavor op_flavor = (enum cil_flavor)(uintptr_t)curr->data;
v1 = __cil_evaluate_tunable_expr_helper(curr->next);
--
2.30.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] libsepol/cil: fix NULL pointer dereference with empty macro argument
2021-02-05 9:45 [PATCH 1/3] libsepol/cil: fix NULL pointer dereference with empty macro argument Nicolas Iooss
2021-02-05 9:45 ` [PATCH 2/3] libsepol/cil: be more robust when encountering <src_info> Nicolas Iooss
2021-02-05 9:45 ` [PATCH 3/3] libsepol/cil: introduce intermediate cast to silence -Wvoid-pointer-to-enum-cast Nicolas Iooss
@ 2021-02-05 21:59 ` James Carter
2021-02-16 14:36 ` James Carter
2 siblings, 1 reply; 9+ messages in thread
From: James Carter @ 2021-02-05 21:59 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
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:
>
> (macro m((name n))) (call m(()))
>
> When calling the macro, the name (in variable "pc") is NULL, which
> triggers a NULL pointer dereference when using it as a key in
> __cil_insert_name(). The stack trace is:
>
> #0 0x7f4662655a85 in __strlen_avx2 (/usr/lib/libc.so.6+0x162a85)
> #1 0x556d0b6d150c in __interceptor_strlen.part.0 (/selinux/libsepol/fuzz/fuzz-secilc+0x44850c)
> #2 0x556d0ba74ed6 in symhash /selinux/libsepol/src/symtab.c:22:9
> #3 0x556d0b9ef50d in hashtab_search /selinux/libsepol/src/hashtab.c:186:11
> #4 0x556d0b928e1f in cil_symtab_get_datum /selinux/libsepol/src/../cil/src/cil_symtab.c:121:37
> #5 0x556d0b8f28f4 in __cil_insert_name /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:96:2
> #6 0x556d0b908184 in cil_resolve_call1 /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:2835:12
> #7 0x556d0b91b404 in __cil_resolve_ast_node /selinux/libsepol/src/../cil/src/cil_resolve_ast.c
> #8 0x556d0b91380f in __cil_resolve_ast_node_helper /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:3773:7
> #9 0x556d0b932230 in cil_tree_walk_core /selinux/libsepol/src/../cil/src/cil_tree.c:263:9
> #10 0x556d0b932230 in cil_tree_walk /selinux/libsepol/src/../cil/src/cil_tree.c:307:7
> #11 0x556d0b932326 in cil_tree_walk_core /selinux/libsepol/src/../cil/src/cil_tree.c:275:9
> #12 0x556d0b932326 in cil_tree_walk /selinux/libsepol/src/../cil/src/cil_tree.c:307:7
> #13 0x556d0b911189 in cil_resolve_ast /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:3941:8
> #14 0x556d0b798729 in cil_compile /selinux/libsepol/src/../cil/src/cil.c:550:7
>
> Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28544
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Wow, that function is getting long and ugly. It probably needs to be
refactored at some point. At any rate, your patch is good.
Acked-by: James Carter <jwcart2@gmail.com>
> ---
> libsepol/cil/src/cil_resolve_ast.c | 79 ++++++++++++++++++++++++++++++
> 1 file changed, 79 insertions(+)
>
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index 0c85eabe5a81..9300cd2be9be 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -2828,6 +2828,12 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> switch (flavor) {
> case CIL_NAME: {
> struct cil_name *name;
> + if (pc->data == NULL) {
> + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> + cil_destroy_args(new_arg);
> + rc = SEPOL_ERR;
> + goto exit;
> + }
> name = __cil_insert_name(args->db, pc->data, current);
> if (name != NULL) {
> new_arg->arg = (struct cil_symtab_datum *)name;
> @@ -2837,21 +2843,57 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> }
> break;
> case CIL_TYPE:
> + if (pc->data == NULL) {
> + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> + cil_destroy_args(new_arg);
> + rc = SEPOL_ERR;
> + goto exit;
> + }
> new_arg->arg_str = pc->data;
> break;
> case CIL_ROLE:
> + if (pc->data == NULL) {
> + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> + cil_destroy_args(new_arg);
> + rc = SEPOL_ERR;
> + goto exit;
> + }
> new_arg->arg_str = pc->data;
> break;
> case CIL_USER:
> + if (pc->data == NULL) {
> + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> + cil_destroy_args(new_arg);
> + rc = SEPOL_ERR;
> + goto exit;
> + }
> new_arg->arg_str = pc->data;
> break;
> case CIL_SENS:
> + if (pc->data == NULL) {
> + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> + cil_destroy_args(new_arg);
> + rc = SEPOL_ERR;
> + goto exit;
> + }
> new_arg->arg_str = pc->data;
> break;
> case CIL_CAT:
> + if (pc->data == NULL) {
> + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> + cil_destroy_args(new_arg);
> + rc = SEPOL_ERR;
> + goto exit;
> + }
> new_arg->arg_str = pc->data;
> break;
> case CIL_BOOL:
> + if (pc->data == NULL) {
> + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> + cil_destroy_args(new_arg);
> + rc = SEPOL_ERR;
> + goto exit;
> + }
> new_arg->arg_str = pc->data;
> break;
> case CIL_CATSET: {
> @@ -2871,6 +2913,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> cil_list_append(((struct cil_symtab_datum*)catset)->nodes,
> CIL_LIST_ITEM, cat_node);
> new_arg->arg = (struct cil_symtab_datum*)catset;
> + } else if (pc->data == NULL) {
> + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> + cil_destroy_args(new_arg);
> + rc = SEPOL_ERR;
> + goto exit;
> } else {
> new_arg->arg_str = pc->data;
> }
> @@ -2896,6 +2943,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> cil_list_append(((struct cil_symtab_datum*)level)->nodes,
> CIL_LIST_ITEM, lvl_node);
> new_arg->arg = (struct cil_symtab_datum*)level;
> + } else if (pc->data == NULL) {
> + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> + cil_destroy_args(new_arg);
> + rc = SEPOL_ERR;
> + goto exit;
> } else {
> new_arg->arg_str = pc->data;
> }
> @@ -2921,6 +2973,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> cil_list_append(((struct cil_symtab_datum*)range)->nodes,
> CIL_LIST_ITEM, range_node);
> new_arg->arg = (struct cil_symtab_datum*)range;
> + } else if (pc->data == NULL) {
> + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> + cil_destroy_args(new_arg);
> + rc = SEPOL_ERR;
> + goto exit;
> } else {
> new_arg->arg_str = pc->data;
> }
> @@ -2946,6 +3003,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> cil_list_append(((struct cil_symtab_datum*)ipaddr)->nodes,
> CIL_LIST_ITEM, addr_node);
> new_arg->arg = (struct cil_symtab_datum*)ipaddr;
> + } else if (pc->data == NULL) {
> + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> + cil_destroy_args(new_arg);
> + rc = SEPOL_ERR;
> + goto exit;
> } else {
> new_arg->arg_str = pc->data;
> }
> @@ -2953,9 +3015,21 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> break;
> }
> case CIL_CLASS:
> + if (pc->data == NULL) {
> + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> + cil_destroy_args(new_arg);
> + rc = SEPOL_ERR;
> + goto exit;
> + }
> new_arg->arg_str = pc->data;
> break;
> case CIL_MAP_CLASS:
> + if (pc->data == NULL) {
> + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> + cil_destroy_args(new_arg);
> + rc = SEPOL_ERR;
> + goto exit;
> + }
> new_arg->arg_str = pc->data;
> break;
> case CIL_CLASSPERMISSION: {
> @@ -2976,6 +3050,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> cp_node->data = cp;
> cil_list_append(cp->datum.nodes, CIL_LIST_ITEM, cp_node);
> new_arg->arg = (struct cil_symtab_datum*)cp;
> + } else if (pc->data == NULL) {
> + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> + cil_destroy_args(new_arg);
> + rc = SEPOL_ERR;
> + goto exit;
> } else {
> new_arg->arg_str = pc->data;
> }
> --
> 2.30.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] libsepol/cil: be more robust when encountering <src_info>
2021-02-05 9:45 ` [PATCH 2/3] libsepol/cil: be more robust when encountering <src_info> Nicolas Iooss
@ 2021-02-05 22:02 ` James Carter
2021-02-16 14:39 ` James Carter
0 siblings, 1 reply; 9+ messages in thread
From: James Carter @ 2021-02-05 22:02 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] libsepol/cil: introduce intermediate cast to silence -Wvoid-pointer-to-enum-cast
2021-02-05 9:45 ` [PATCH 3/3] libsepol/cil: introduce intermediate cast to silence -Wvoid-pointer-to-enum-cast Nicolas Iooss
@ 2021-02-05 22:02 ` James Carter
2021-02-16 14:39 ` James Carter
0 siblings, 1 reply; 9+ messages in thread
From: James Carter @ 2021-02-05 22:02 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
On Fri, Feb 5, 2021 at 4:54 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> clang 11.0.0 reports the following warning several times (when building
> with "make CC=clang" from libsepol directory, in the default
> configuration of the git repository):
>
> ../cil/src/cil_binary.c:1980:8: error: cast to smaller integer type
> 'enum cil_flavor' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast]
> op = (enum cil_flavor)curr->data;
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Silence this warning by casting the pointer to an integer the cast to
> enum cil_flavor.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Acked-by: James Carter <jwcart2@gmail.com>
> ---
> libsepol/cil/src/cil_binary.c | 12 ++++++------
> libsepol/cil/src/cil_policy.c | 12 ++++++------
> libsepol/cil/src/cil_post.c | 2 +-
> libsepol/cil/src/cil_resolve_ast.c | 2 +-
> 4 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index 7ba2098b61ea..f80d84679f85 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
> @@ -1977,7 +1977,7 @@ static void __cil_expr_to_string(struct cil_list *expr, enum cil_flavor flavor,
> curr = expr->head;
>
> if (curr->flavor == CIL_OP) {
> - op = (enum cil_flavor)curr->data;
> + op = (enum cil_flavor)(uintptr_t)curr->data;
>
> if (op == CIL_ALL) {
> *out = cil_strdup(CIL_KEY_ALL);
> @@ -2076,7 +2076,7 @@ static int __cil_cond_expr_to_sepol_expr_helper(policydb_t *pdb, struct cil_list
> if (item == NULL) {
> goto exit;
> } else if (item->flavor == CIL_OP) {
> - enum cil_flavor cil_op = (enum cil_flavor)item->data;
> + enum cil_flavor cil_op = (enum cil_flavor)(uintptr_t)item->data;
>
> op = cil_malloc(sizeof(*op));
> op->bool = 0;
> @@ -2562,7 +2562,7 @@ int __cil_constrain_expr_leaf_to_sepol_expr(policydb_t *pdb, const struct cil_db
> struct cil_list_item *l_item = op_item->next;
> struct cil_list_item *r_item = op_item->next->next;
>
> - enum cil_flavor l_operand = (enum cil_flavor)l_item->data;
> + enum cil_flavor l_operand = (enum cil_flavor)(uintptr_t)l_item->data;
>
> switch (l_operand) {
> case CIL_CONS_U1:
> @@ -2593,7 +2593,7 @@ int __cil_constrain_expr_leaf_to_sepol_expr(policydb_t *pdb, const struct cil_db
> expr->attr = CEXPR_TYPE | CEXPR_XTARGET;
> break;
> case CIL_CONS_L1: {
> - enum cil_flavor r_operand = (enum cil_flavor)r_item->data;
> + enum cil_flavor r_operand = (enum cil_flavor)(uintptr_t)r_item->data;
>
> if (r_operand == CIL_CONS_L2) {
> expr->attr = CEXPR_L1L2;
> @@ -2608,7 +2608,7 @@ int __cil_constrain_expr_leaf_to_sepol_expr(policydb_t *pdb, const struct cil_db
> expr->attr = CEXPR_L2H2;
> break;
> case CIL_CONS_H1: {
> - enum cil_flavor r_operand = (enum cil_flavor)r_item->data;
> + enum cil_flavor r_operand = (enum cil_flavor)(uintptr_t)r_item->data;
> if (r_operand == CIL_CONS_L2) {
> expr->attr = CEXPR_H1L2;
> } else {
> @@ -2672,7 +2672,7 @@ int __cil_constrain_expr_to_sepol_expr_helper(policydb_t *pdb, const struct cil_
> goto exit;
> }
>
> - enum cil_flavor cil_op = (enum cil_flavor)item->data;
> + enum cil_flavor cil_op = (enum cil_flavor)(uintptr_t)item->data;
> switch (cil_op) {
> case CIL_NOT:
> op->expr_type = CEXPR_NOT;
> diff --git a/libsepol/cil/src/cil_policy.c b/libsepol/cil/src/cil_policy.c
> index 74edb34575ea..30d507f12626 100644
> --- a/libsepol/cil/src/cil_policy.c
> +++ b/libsepol/cil/src/cil_policy.c
> @@ -285,7 +285,7 @@ static void cil_cond_expr_to_policy(FILE *out, struct cil_list *expr, int first)
> struct cil_list_item *i1 = expr->head;
>
> if (i1->flavor == CIL_OP) {
> - enum cil_flavor op = (enum cil_flavor)i1->data;
> + enum cil_flavor op = (enum cil_flavor)(uintptr_t)i1->data;
> fprintf(out, "(");
> switch (op) {
> case CIL_NOT:
> @@ -385,7 +385,7 @@ static size_t __cil_cons_leaf_operand_len(struct cil_db *db, struct cil_list_ite
>
> static size_t __cil_cons_leaf_op_len(struct cil_list_item *op)
> {
> - enum cil_flavor flavor = (enum cil_flavor)op->data;
> + enum cil_flavor flavor = (enum cil_flavor)(uintptr_t)op->data;
> size_t len;
>
> switch (flavor) {
> @@ -420,7 +420,7 @@ static size_t cil_cons_expr_len(struct cil_db *db, struct cil_list *cons_expr)
>
> i1 = cons_expr->head;
>
> - op = (enum cil_flavor)i1->data;
> + op = (enum cil_flavor)(uintptr_t)i1->data;
> switch (op) {
> case CIL_NOT:
> len = 6; /* "(not )" */
> @@ -472,7 +472,7 @@ static char *__cil_cons_leaf_operand_to_string(struct cil_db *db, struct cil_lis
> size_t o_len;
>
> if (flavor == CIL_CONS_OPERAND) {
> - enum cil_flavor o_flavor = (enum cil_flavor)operand->data;
> + enum cil_flavor o_flavor = (enum cil_flavor)(uintptr_t)operand->data;
> switch (o_flavor) {
> case CIL_CONS_U1:
> o_str = "u1";
> @@ -555,7 +555,7 @@ static char *__cil_cons_leaf_operand_to_string(struct cil_db *db, struct cil_lis
>
> static char *__cil_cons_leaf_op_to_string(struct cil_list_item *op, char *new)
> {
> - enum cil_flavor flavor = (enum cil_flavor)op->data;
> + enum cil_flavor flavor = (enum cil_flavor)(uintptr_t)op->data;
> const char *op_str;
> size_t len;
>
> @@ -599,7 +599,7 @@ static char *__cil_cons_expr_to_string(struct cil_db *db, struct cil_list *cons_
>
> i1 = cons_expr->head;
>
> - op = (enum cil_flavor)i1->data;
> + op = (enum cil_flavor)(uintptr_t)i1->data;
> switch (op) {
> case CIL_NOT:
> *new++ = '(';
> diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
> index 37a444157853..a55df1ea5bb0 100644
> --- a/libsepol/cil/src/cil_post.c
> +++ b/libsepol/cil/src/cil_post.c
> @@ -1301,7 +1301,7 @@ static int __cil_expr_to_bitmap(struct cil_list *expr, ebitmap_t *out, int max,
> flavor = expr->flavor;
>
> if (curr->flavor == CIL_OP) {
> - enum cil_flavor op = (enum cil_flavor)curr->data;
> + enum cil_flavor op = (enum cil_flavor)(uintptr_t)curr->data;
>
> if (op == CIL_ALL) {
> ebitmap_init(&b1); /* all zeros */
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index 9300cd2be9be..208bc01a7a2d 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -3355,7 +3355,7 @@ static int __cil_evaluate_tunable_expr(struct cil_list_item *curr)
> return CIL_FALSE;
> } else if (curr->flavor == CIL_OP) {
> uint16_t v1, v2;
> - enum cil_flavor op_flavor = (enum cil_flavor)curr->data;
> + enum cil_flavor op_flavor = (enum cil_flavor)(uintptr_t)curr->data;
>
> v1 = __cil_evaluate_tunable_expr_helper(curr->next);
>
> --
> 2.30.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] libsepol/cil: fix NULL pointer dereference with empty macro argument
2021-02-05 21:59 ` [PATCH 1/3] libsepol/cil: fix NULL pointer dereference with empty macro argument James Carter
@ 2021-02-16 14:36 ` James Carter
0 siblings, 0 replies; 9+ messages in thread
From: James Carter @ 2021-02-16 14:36 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
On Fri, Feb 5, 2021 at 4:59 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:
> >
> > (macro m((name n))) (call m(()))
> >
> > When calling the macro, the name (in variable "pc") is NULL, which
> > triggers a NULL pointer dereference when using it as a key in
> > __cil_insert_name(). The stack trace is:
> >
> > #0 0x7f4662655a85 in __strlen_avx2 (/usr/lib/libc.so.6+0x162a85)
> > #1 0x556d0b6d150c in __interceptor_strlen.part.0 (/selinux/libsepol/fuzz/fuzz-secilc+0x44850c)
> > #2 0x556d0ba74ed6 in symhash /selinux/libsepol/src/symtab.c:22:9
> > #3 0x556d0b9ef50d in hashtab_search /selinux/libsepol/src/hashtab.c:186:11
> > #4 0x556d0b928e1f in cil_symtab_get_datum /selinux/libsepol/src/../cil/src/cil_symtab.c:121:37
> > #5 0x556d0b8f28f4 in __cil_insert_name /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:96:2
> > #6 0x556d0b908184 in cil_resolve_call1 /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:2835:12
> > #7 0x556d0b91b404 in __cil_resolve_ast_node /selinux/libsepol/src/../cil/src/cil_resolve_ast.c
> > #8 0x556d0b91380f in __cil_resolve_ast_node_helper /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:3773:7
> > #9 0x556d0b932230 in cil_tree_walk_core /selinux/libsepol/src/../cil/src/cil_tree.c:263:9
> > #10 0x556d0b932230 in cil_tree_walk /selinux/libsepol/src/../cil/src/cil_tree.c:307:7
> > #11 0x556d0b932326 in cil_tree_walk_core /selinux/libsepol/src/../cil/src/cil_tree.c:275:9
> > #12 0x556d0b932326 in cil_tree_walk /selinux/libsepol/src/../cil/src/cil_tree.c:307:7
> > #13 0x556d0b911189 in cil_resolve_ast /selinux/libsepol/src/../cil/src/cil_resolve_ast.c:3941:8
> > #14 0x556d0b798729 in cil_compile /selinux/libsepol/src/../cil/src/cil.c:550:7
> >
> > Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28544
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
> Wow, that function is getting long and ugly. It probably needs to be
> refactored at some point. At any rate, your patch is good.
>
> Acked-by: James Carter <jwcart2@gmail.com>
>
Applied.
Thanks,
Jim
> > ---
> > libsepol/cil/src/cil_resolve_ast.c | 79 ++++++++++++++++++++++++++++++
> > 1 file changed, 79 insertions(+)
> >
> > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> > index 0c85eabe5a81..9300cd2be9be 100644
> > --- a/libsepol/cil/src/cil_resolve_ast.c
> > +++ b/libsepol/cil/src/cil_resolve_ast.c
> > @@ -2828,6 +2828,12 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> > switch (flavor) {
> > case CIL_NAME: {
> > struct cil_name *name;
> > + if (pc->data == NULL) {
> > + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> > + cil_destroy_args(new_arg);
> > + rc = SEPOL_ERR;
> > + goto exit;
> > + }
> > name = __cil_insert_name(args->db, pc->data, current);
> > if (name != NULL) {
> > new_arg->arg = (struct cil_symtab_datum *)name;
> > @@ -2837,21 +2843,57 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> > }
> > break;
> > case CIL_TYPE:
> > + if (pc->data == NULL) {
> > + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> > + cil_destroy_args(new_arg);
> > + rc = SEPOL_ERR;
> > + goto exit;
> > + }
> > new_arg->arg_str = pc->data;
> > break;
> > case CIL_ROLE:
> > + if (pc->data == NULL) {
> > + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> > + cil_destroy_args(new_arg);
> > + rc = SEPOL_ERR;
> > + goto exit;
> > + }
> > new_arg->arg_str = pc->data;
> > break;
> > case CIL_USER:
> > + if (pc->data == NULL) {
> > + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> > + cil_destroy_args(new_arg);
> > + rc = SEPOL_ERR;
> > + goto exit;
> > + }
> > new_arg->arg_str = pc->data;
> > break;
> > case CIL_SENS:
> > + if (pc->data == NULL) {
> > + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> > + cil_destroy_args(new_arg);
> > + rc = SEPOL_ERR;
> > + goto exit;
> > + }
> > new_arg->arg_str = pc->data;
> > break;
> > case CIL_CAT:
> > + if (pc->data == NULL) {
> > + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> > + cil_destroy_args(new_arg);
> > + rc = SEPOL_ERR;
> > + goto exit;
> > + }
> > new_arg->arg_str = pc->data;
> > break;
> > case CIL_BOOL:
> > + if (pc->data == NULL) {
> > + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> > + cil_destroy_args(new_arg);
> > + rc = SEPOL_ERR;
> > + goto exit;
> > + }
> > new_arg->arg_str = pc->data;
> > break;
> > case CIL_CATSET: {
> > @@ -2871,6 +2913,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> > cil_list_append(((struct cil_symtab_datum*)catset)->nodes,
> > CIL_LIST_ITEM, cat_node);
> > new_arg->arg = (struct cil_symtab_datum*)catset;
> > + } else if (pc->data == NULL) {
> > + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> > + cil_destroy_args(new_arg);
> > + rc = SEPOL_ERR;
> > + goto exit;
> > } else {
> > new_arg->arg_str = pc->data;
> > }
> > @@ -2896,6 +2943,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> > cil_list_append(((struct cil_symtab_datum*)level)->nodes,
> > CIL_LIST_ITEM, lvl_node);
> > new_arg->arg = (struct cil_symtab_datum*)level;
> > + } else if (pc->data == NULL) {
> > + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> > + cil_destroy_args(new_arg);
> > + rc = SEPOL_ERR;
> > + goto exit;
> > } else {
> > new_arg->arg_str = pc->data;
> > }
> > @@ -2921,6 +2973,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> > cil_list_append(((struct cil_symtab_datum*)range)->nodes,
> > CIL_LIST_ITEM, range_node);
> > new_arg->arg = (struct cil_symtab_datum*)range;
> > + } else if (pc->data == NULL) {
> > + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> > + cil_destroy_args(new_arg);
> > + rc = SEPOL_ERR;
> > + goto exit;
> > } else {
> > new_arg->arg_str = pc->data;
> > }
> > @@ -2946,6 +3003,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> > cil_list_append(((struct cil_symtab_datum*)ipaddr)->nodes,
> > CIL_LIST_ITEM, addr_node);
> > new_arg->arg = (struct cil_symtab_datum*)ipaddr;
> > + } else if (pc->data == NULL) {
> > + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> > + cil_destroy_args(new_arg);
> > + rc = SEPOL_ERR;
> > + goto exit;
> > } else {
> > new_arg->arg_str = pc->data;
> > }
> > @@ -2953,9 +3015,21 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> > break;
> > }
> > case CIL_CLASS:
> > + if (pc->data == NULL) {
> > + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> > + cil_destroy_args(new_arg);
> > + rc = SEPOL_ERR;
> > + goto exit;
> > + }
> > new_arg->arg_str = pc->data;
> > break;
> > case CIL_MAP_CLASS:
> > + if (pc->data == NULL) {
> > + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> > + cil_destroy_args(new_arg);
> > + rc = SEPOL_ERR;
> > + goto exit;
> > + }
> > new_arg->arg_str = pc->data;
> > break;
> > case CIL_CLASSPERMISSION: {
> > @@ -2976,6 +3050,11 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
> > cp_node->data = cp;
> > cil_list_append(cp->datum.nodes, CIL_LIST_ITEM, cp_node);
> > new_arg->arg = (struct cil_symtab_datum*)cp;
> > + } else if (pc->data == NULL) {
> > + cil_tree_log(current, CIL_ERR, "Invalid macro parameter");
> > + cil_destroy_args(new_arg);
> > + rc = SEPOL_ERR;
> > + goto exit;
> > } else {
> > new_arg->arg_str = pc->data;
> > }
> > --
> > 2.30.0
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] libsepol/cil: be more robust when encountering <src_info>
2021-02-05 22:02 ` James Carter
@ 2021-02-16 14:39 ` James Carter
0 siblings, 0 replies; 9+ messages in thread
From: James Carter @ 2021-02-16 14:39 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
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
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] libsepol/cil: introduce intermediate cast to silence -Wvoid-pointer-to-enum-cast
2021-02-05 22:02 ` James Carter
@ 2021-02-16 14:39 ` James Carter
0 siblings, 0 replies; 9+ messages in thread
From: James Carter @ 2021-02-16 14:39 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
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:
> >
> > clang 11.0.0 reports the following warning several times (when building
> > with "make CC=clang" from libsepol directory, in the default
> > configuration of the git repository):
> >
> > ../cil/src/cil_binary.c:1980:8: error: cast to smaller integer type
> > 'enum cil_flavor' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast]
> > op = (enum cil_flavor)curr->data;
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Silence this warning by casting the pointer to an integer the cast to
> > enum cil_flavor.
> >
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
> Acked-by: James Carter <jwcart2@gmail.com>
Applied.
Thanks,
Jim
>
> > ---
> > libsepol/cil/src/cil_binary.c | 12 ++++++------
> > libsepol/cil/src/cil_policy.c | 12 ++++++------
> > libsepol/cil/src/cil_post.c | 2 +-
> > libsepol/cil/src/cil_resolve_ast.c | 2 +-
> > 4 files changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> > index 7ba2098b61ea..f80d84679f85 100644
> > --- a/libsepol/cil/src/cil_binary.c
> > +++ b/libsepol/cil/src/cil_binary.c
> > @@ -1977,7 +1977,7 @@ static void __cil_expr_to_string(struct cil_list *expr, enum cil_flavor flavor,
> > curr = expr->head;
> >
> > if (curr->flavor == CIL_OP) {
> > - op = (enum cil_flavor)curr->data;
> > + op = (enum cil_flavor)(uintptr_t)curr->data;
> >
> > if (op == CIL_ALL) {
> > *out = cil_strdup(CIL_KEY_ALL);
> > @@ -2076,7 +2076,7 @@ static int __cil_cond_expr_to_sepol_expr_helper(policydb_t *pdb, struct cil_list
> > if (item == NULL) {
> > goto exit;
> > } else if (item->flavor == CIL_OP) {
> > - enum cil_flavor cil_op = (enum cil_flavor)item->data;
> > + enum cil_flavor cil_op = (enum cil_flavor)(uintptr_t)item->data;
> >
> > op = cil_malloc(sizeof(*op));
> > op->bool = 0;
> > @@ -2562,7 +2562,7 @@ int __cil_constrain_expr_leaf_to_sepol_expr(policydb_t *pdb, const struct cil_db
> > struct cil_list_item *l_item = op_item->next;
> > struct cil_list_item *r_item = op_item->next->next;
> >
> > - enum cil_flavor l_operand = (enum cil_flavor)l_item->data;
> > + enum cil_flavor l_operand = (enum cil_flavor)(uintptr_t)l_item->data;
> >
> > switch (l_operand) {
> > case CIL_CONS_U1:
> > @@ -2593,7 +2593,7 @@ int __cil_constrain_expr_leaf_to_sepol_expr(policydb_t *pdb, const struct cil_db
> > expr->attr = CEXPR_TYPE | CEXPR_XTARGET;
> > break;
> > case CIL_CONS_L1: {
> > - enum cil_flavor r_operand = (enum cil_flavor)r_item->data;
> > + enum cil_flavor r_operand = (enum cil_flavor)(uintptr_t)r_item->data;
> >
> > if (r_operand == CIL_CONS_L2) {
> > expr->attr = CEXPR_L1L2;
> > @@ -2608,7 +2608,7 @@ int __cil_constrain_expr_leaf_to_sepol_expr(policydb_t *pdb, const struct cil_db
> > expr->attr = CEXPR_L2H2;
> > break;
> > case CIL_CONS_H1: {
> > - enum cil_flavor r_operand = (enum cil_flavor)r_item->data;
> > + enum cil_flavor r_operand = (enum cil_flavor)(uintptr_t)r_item->data;
> > if (r_operand == CIL_CONS_L2) {
> > expr->attr = CEXPR_H1L2;
> > } else {
> > @@ -2672,7 +2672,7 @@ int __cil_constrain_expr_to_sepol_expr_helper(policydb_t *pdb, const struct cil_
> > goto exit;
> > }
> >
> > - enum cil_flavor cil_op = (enum cil_flavor)item->data;
> > + enum cil_flavor cil_op = (enum cil_flavor)(uintptr_t)item->data;
> > switch (cil_op) {
> > case CIL_NOT:
> > op->expr_type = CEXPR_NOT;
> > diff --git a/libsepol/cil/src/cil_policy.c b/libsepol/cil/src/cil_policy.c
> > index 74edb34575ea..30d507f12626 100644
> > --- a/libsepol/cil/src/cil_policy.c
> > +++ b/libsepol/cil/src/cil_policy.c
> > @@ -285,7 +285,7 @@ static void cil_cond_expr_to_policy(FILE *out, struct cil_list *expr, int first)
> > struct cil_list_item *i1 = expr->head;
> >
> > if (i1->flavor == CIL_OP) {
> > - enum cil_flavor op = (enum cil_flavor)i1->data;
> > + enum cil_flavor op = (enum cil_flavor)(uintptr_t)i1->data;
> > fprintf(out, "(");
> > switch (op) {
> > case CIL_NOT:
> > @@ -385,7 +385,7 @@ static size_t __cil_cons_leaf_operand_len(struct cil_db *db, struct cil_list_ite
> >
> > static size_t __cil_cons_leaf_op_len(struct cil_list_item *op)
> > {
> > - enum cil_flavor flavor = (enum cil_flavor)op->data;
> > + enum cil_flavor flavor = (enum cil_flavor)(uintptr_t)op->data;
> > size_t len;
> >
> > switch (flavor) {
> > @@ -420,7 +420,7 @@ static size_t cil_cons_expr_len(struct cil_db *db, struct cil_list *cons_expr)
> >
> > i1 = cons_expr->head;
> >
> > - op = (enum cil_flavor)i1->data;
> > + op = (enum cil_flavor)(uintptr_t)i1->data;
> > switch (op) {
> > case CIL_NOT:
> > len = 6; /* "(not )" */
> > @@ -472,7 +472,7 @@ static char *__cil_cons_leaf_operand_to_string(struct cil_db *db, struct cil_lis
> > size_t o_len;
> >
> > if (flavor == CIL_CONS_OPERAND) {
> > - enum cil_flavor o_flavor = (enum cil_flavor)operand->data;
> > + enum cil_flavor o_flavor = (enum cil_flavor)(uintptr_t)operand->data;
> > switch (o_flavor) {
> > case CIL_CONS_U1:
> > o_str = "u1";
> > @@ -555,7 +555,7 @@ static char *__cil_cons_leaf_operand_to_string(struct cil_db *db, struct cil_lis
> >
> > static char *__cil_cons_leaf_op_to_string(struct cil_list_item *op, char *new)
> > {
> > - enum cil_flavor flavor = (enum cil_flavor)op->data;
> > + enum cil_flavor flavor = (enum cil_flavor)(uintptr_t)op->data;
> > const char *op_str;
> > size_t len;
> >
> > @@ -599,7 +599,7 @@ static char *__cil_cons_expr_to_string(struct cil_db *db, struct cil_list *cons_
> >
> > i1 = cons_expr->head;
> >
> > - op = (enum cil_flavor)i1->data;
> > + op = (enum cil_flavor)(uintptr_t)i1->data;
> > switch (op) {
> > case CIL_NOT:
> > *new++ = '(';
> > diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
> > index 37a444157853..a55df1ea5bb0 100644
> > --- a/libsepol/cil/src/cil_post.c
> > +++ b/libsepol/cil/src/cil_post.c
> > @@ -1301,7 +1301,7 @@ static int __cil_expr_to_bitmap(struct cil_list *expr, ebitmap_t *out, int max,
> > flavor = expr->flavor;
> >
> > if (curr->flavor == CIL_OP) {
> > - enum cil_flavor op = (enum cil_flavor)curr->data;
> > + enum cil_flavor op = (enum cil_flavor)(uintptr_t)curr->data;
> >
> > if (op == CIL_ALL) {
> > ebitmap_init(&b1); /* all zeros */
> > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> > index 9300cd2be9be..208bc01a7a2d 100644
> > --- a/libsepol/cil/src/cil_resolve_ast.c
> > +++ b/libsepol/cil/src/cil_resolve_ast.c
> > @@ -3355,7 +3355,7 @@ static int __cil_evaluate_tunable_expr(struct cil_list_item *curr)
> > return CIL_FALSE;
> > } else if (curr->flavor == CIL_OP) {
> > uint16_t v1, v2;
> > - enum cil_flavor op_flavor = (enum cil_flavor)curr->data;
> > + enum cil_flavor op_flavor = (enum cil_flavor)(uintptr_t)curr->data;
> >
> > v1 = __cil_evaluate_tunable_expr_helper(curr->next);
> >
> > --
> > 2.30.0
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-02-16 14:41 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-05 9:45 [PATCH 1/3] libsepol/cil: fix NULL pointer dereference with empty macro argument Nicolas Iooss
2021-02-05 9:45 ` [PATCH 2/3] libsepol/cil: be more robust when encountering <src_info> Nicolas Iooss
2021-02-05 22:02 ` James Carter
2021-02-16 14:39 ` James Carter
2021-02-05 9:45 ` [PATCH 3/3] libsepol/cil: introduce intermediate cast to silence -Wvoid-pointer-to-enum-cast Nicolas Iooss
2021-02-05 22:02 ` James Carter
2021-02-16 14:39 ` James Carter
2021-02-05 21:59 ` [PATCH 1/3] libsepol/cil: fix NULL pointer dereference with empty macro argument James Carter
2021-02-16 14:36 ` James Carter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.