| Message ID | 20230420125801.999381-1-jwcart2@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e81c466bca9a |
| Delegated to: | Petr Lautrbach |
| Headers | show |
| Series | libsepol/cil: Fix class permission verification in CIL | expand |
On 4/20/23 14:58, James Carter wrote: > Before the CIL post processing phase (where expressions are evaluated, > various ebitmaps are set, etc) there is a pre-verification where > checks are made to find self references or loops in bounds, attribute > sets, and class permissions. The class permission checking is faulty > in two ways. > > First, it does not check for the use of "all" in a permission expression > for a class that has no permissions. An error will still be generated > later and secilc will exit cleanly, but without an error message that > explains the problem. > > Second, it does not properly handle lists in permission expressions. > For example, "(C ((P)))" is a legitimate class permission. The > permissions expression contains one item that is a list containing > one permission. This permission expression will be properly evaluated. > Unfortunately, the class permission verification assumes that each > item in the permission expression is either an operator or a > permission datum and a segmenation fault will occur. > > Refactor the class permission checking to give a proper error when > "all" is used in a permission expression for a class that has no > permissions and so that it can handle lists in permission > expressions. Also, check for the actual flavor of each item in > the permission expression and return an error if an unexpected > flavor is found. > > The failure to properly handle lists in permission expressions was > found by oss-fuzz (#58085). For what it's worth: Verified the fuzzer no longer crashes and no new issues arose after running for ~1h. Successfully build DSSP5. Tested-by: Christian Göttsche <cgzones@googlemail.com> > > Signed-off-by: James Carter <jwcart2@gmail.com> > --- > libsepol/cil/src/cil_verify.c | 167 +++++++++++++++++++++++----------- > 1 file changed, 114 insertions(+), 53 deletions(-) > > diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c > index 4640dc59..3f58969d 100644 > --- a/libsepol/cil/src/cil_verify.c > +++ b/libsepol/cil/src/cil_verify.c > @@ -1700,31 +1700,109 @@ static int __add_perm_to_list(__attribute__((unused)) hashtab_key_t k, hashtab_d > return SEPOL_OK; > } > > -static int __cil_verify_classperms(struct cil_list *classperms, > - struct cil_symtab_datum *orig, > - struct cil_symtab_datum *parent, > - struct cil_symtab_datum *cur, > - enum cil_flavor flavor, > - unsigned steps, unsigned limit) > +static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symtab_datum *orig, struct cil_symtab_datum *cur, unsigned steps, unsigned limit); > + > +static int __cil_verify_map_perm(struct cil_class *class, struct cil_perm *perm, struct cil_symtab_datum *orig, unsigned steps, unsigned limit) > +{ > + int rc; > + > + if (!perm->classperms) { > + cil_tree_log(NODE(class), CIL_ERR, "No class permissions for map class %s, permission %s", DATUM(class)->name, DATUM(perm)->name); > + goto exit; > + } > + > + rc = __cil_verify_classperms(perm->classperms, orig, &perm->datum, steps, limit); > + if (rc != SEPOL_OK) { > + cil_tree_log(NODE(class), CIL_ERR, "There was an error verifying class permissions for map class %s, permission %s", DATUM(class)->name, DATUM(perm)->name); > + goto exit; > + } > + > + return SEPOL_OK; > + > +exit: > + return SEPOL_ERR; > +} > + > + > +static int __cil_verify_perms(struct cil_class *class, struct cil_list *perms, struct cil_symtab_datum *orig, unsigned steps, unsigned limit) > { > int rc = SEPOL_ERR; > - struct cil_list_item *curr; > + int count = 0; > + struct cil_list_item *i = NULL; > > - if (classperms == NULL) { > - if (flavor == CIL_MAP_PERM) { > - cil_tree_log(NODE(cur), CIL_ERR, "Map class %s does not have a classmapping for %s", parent->name, cur->name); > + if (!perms) { > + cil_tree_log(NODE(class), CIL_ERR, "No permissions for class %s in class permissions", DATUM(class)->name); > + goto exit; > + } > + > + cil_list_for_each(i, perms) { > + count++; > + if (i->flavor == CIL_LIST) { > + rc = __cil_verify_perms(class, i->data, orig, steps, limit); > + if (rc != SEPOL_OK) { > + goto exit; > + } > + } else if (i->flavor == CIL_DATUM) { > + struct cil_perm *perm = i->data; > + if (FLAVOR(perm) == CIL_MAP_PERM) { > + rc = __cil_verify_map_perm(class, perm, orig, steps, limit); > + if (rc != SEPOL_OK) { > + goto exit; > + } > + } > + } else if (i->flavor == CIL_OP) { > + enum cil_flavor op = (enum cil_flavor)(uintptr_t)i->data; > + if (op == CIL_ALL) { > + struct cil_list *perm_list; > + struct cil_list_item *j = NULL; > + int count2 = 0; > + cil_list_init(&perm_list, CIL_MAP_PERM); > + cil_symtab_map(&class->perms, __add_perm_to_list, perm_list); > + cil_list_for_each(j, perm_list) { > + count2++; > + struct cil_perm *perm = j->data; > + if (FLAVOR(perm) == CIL_MAP_PERM) { > + rc = __cil_verify_map_perm(class, perm, orig, steps, limit); > + if (rc != SEPOL_OK) { > + cil_list_destroy(&perm_list, CIL_FALSE); > + goto exit; > + } > + } > + } > + cil_list_destroy(&perm_list, CIL_FALSE); > + if (count2 == 0) { > + cil_tree_log(NODE(class), CIL_ERR, "Operator \"all\" used for %s which has no permissions associated with it", DATUM(class)->name); > + goto exit; > + } > + } > } else { > - cil_tree_log(NODE(cur), CIL_ERR, "Classpermission %s does not have a classpermissionset", cur->name); > + cil_tree_log(NODE(class), CIL_ERR, "Permission list for %s has an unexpected flavor: %d", DATUM(class)->name, i->flavor); > + goto exit; > } > + } > + > + if (count == 0) { > + cil_tree_log(NODE(class), CIL_ERR, "Empty permissions list for class %s in class permissions", DATUM(class)->name); > + goto exit; > + } > + > + return SEPOL_OK; > + > +exit: > + return SEPOL_ERR; > +} > + > +static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symtab_datum *orig, struct cil_symtab_datum *cur, unsigned steps, unsigned limit) > +{ > + int rc; > + struct cil_list_item *i; > + > + if (classperms == NULL) { > goto exit; > } > > if (steps > 0 && orig == cur) { > - if (flavor == CIL_MAP_PERM) { > - cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving the map class %s and permission %s", parent->name, cur->name); > - } else { > - cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving the set %s", cur->name); > - } > + cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving %s", cur->name); > goto exit; > } else { > steps++; > @@ -1735,44 +1813,20 @@ static int __cil_verify_classperms(struct cil_list *classperms, > } > } > > - cil_list_for_each(curr, classperms) { > - if (curr->flavor == CIL_CLASSPERMS) { > - struct cil_classperms *cp = curr->data; > - if (FLAVOR(cp->class) != CIL_CLASS) { /* MAP */ > - struct cil_list_item *i = NULL; > - cil_list_for_each(i, cp->perms) { > - if (i->flavor != CIL_OP) { > - struct cil_perm *cmp = i->data; > - rc = __cil_verify_classperms(cmp->classperms, orig, &cp->class->datum, &cmp->datum, CIL_MAP_PERM, steps, limit); > - if (rc != SEPOL_OK) { > - goto exit; > - } > - } else { > - enum cil_flavor op = (enum cil_flavor)(uintptr_t)i->data; > - if (op == CIL_ALL) { > - struct cil_class *mc = cp->class; > - struct cil_list *perm_list; > - struct cil_list_item *j = NULL; > - > - cil_list_init(&perm_list, CIL_MAP_PERM); > - cil_symtab_map(&mc->perms, __add_perm_to_list, perm_list); > - cil_list_for_each(j, perm_list) { > - struct cil_perm *cmp = j->data; > - rc = __cil_verify_classperms(cmp->classperms, orig, &cp->class->datum, &cmp->datum, CIL_MAP_PERM, steps, limit); > - if (rc != SEPOL_OK) { > - cil_list_destroy(&perm_list, CIL_FALSE); > - goto exit; > - } > - } > - cil_list_destroy(&perm_list, CIL_FALSE); > - } > - } > - } > + cil_list_for_each(i, classperms) { > + if (i->flavor == CIL_CLASSPERMS) { > + struct cil_classperms *cp = i->data; > + rc = __cil_verify_perms(cp->class, cp->perms, orig, steps, limit); > + if (rc != SEPOL_OK) { > + goto exit; > } > } else { /* SET */ > - struct cil_classperms_set *cp_set = curr->data; > + struct cil_classperms_set *cp_set = i->data; > struct cil_classpermission *cp = cp_set->set; > - rc = __cil_verify_classperms(cp->classperms, orig, NULL, &cp->datum, CIL_CLASSPERMISSION, steps, limit); > + if (!cp->classperms) { > + cil_tree_log(NODE(cur), CIL_ERR, "Classpermission %s does not have a classpermissionset", DATUM(cp)->name); > + } > + rc = __cil_verify_classperms(cp->classperms, orig, &cp->datum, steps, limit); > if (rc != SEPOL_OK) { > goto exit; > } > @@ -1787,9 +1841,15 @@ exit: > > static int __cil_verify_classpermission(struct cil_tree_node *node) > { > + int rc; > struct cil_classpermission *cp = node->data; > > - return __cil_verify_classperms(cp->classperms, &cp->datum, NULL, &cp->datum, CIL_CLASSPERMISSION, 0, 2); > + rc = __cil_verify_classperms(cp->classperms, &cp->datum, &cp->datum, 0, 2); > + if (rc != SEPOL_OK) { > + cil_tree_log(node, CIL_ERR, "Error verifying class permissions for classpermission %s", DATUM(cp)->name); > + } > + > + return rc; > } > > struct cil_verify_map_args { > @@ -1804,8 +1864,9 @@ static int __verify_map_perm_classperms(__attribute__((unused)) hashtab_key_t k, > struct cil_perm *cmp = (struct cil_perm *)d; > int rc; > > - rc = __cil_verify_classperms(cmp->classperms, &cmp->datum, &map_args->class->datum, &cmp->datum, CIL_MAP_PERM, 0, 2); > + rc = __cil_verify_classperms(cmp->classperms, &cmp->datum, &cmp->datum, 0, 2); > if (rc != SEPOL_OK) { > + cil_tree_log(NODE(cmp), CIL_ERR, "Error verifying class permissions for map class %s, permission %s", DATUM(map_args->class)->name, DATUM(cmp)->name); > map_args->rc = rc; > } >
On Wed, Jul 12, 2023 at 12:01 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > On 4/20/23 14:58, James Carter wrote: > > Before the CIL post processing phase (where expressions are evaluated, > > various ebitmaps are set, etc) there is a pre-verification where > > checks are made to find self references or loops in bounds, attribute > > sets, and class permissions. The class permission checking is faulty > > in two ways. > > > > First, it does not check for the use of "all" in a permission expression > > for a class that has no permissions. An error will still be generated > > later and secilc will exit cleanly, but without an error message that > > explains the problem. > > > > Second, it does not properly handle lists in permission expressions. > > For example, "(C ((P)))" is a legitimate class permission. The > > permissions expression contains one item that is a list containing > > one permission. This permission expression will be properly evaluated. > > Unfortunately, the class permission verification assumes that each > > item in the permission expression is either an operator or a > > permission datum and a segmenation fault will occur. > > > > Refactor the class permission checking to give a proper error when > > "all" is used in a permission expression for a class that has no > > permissions and so that it can handle lists in permission > > expressions. Also, check for the actual flavor of each item in > > the permission expression and return an error if an unexpected > > flavor is found. > > > > The failure to properly handle lists in permission expressions was > > found by oss-fuzz (#58085). > > > For what it's worth: > > Verified the fuzzer no longer crashes and no new issues arose after > running for ~1h. > > Successfully build DSSP5. > > > Tested-by: Christian Göttsche <cgzones@googlemail.com> > I plan on merging this soon, unless I hear any objections. Jim > > > > > Signed-off-by: James Carter <jwcart2@gmail.com> > > --- > > libsepol/cil/src/cil_verify.c | 167 +++++++++++++++++++++++----------- > > 1 file changed, 114 insertions(+), 53 deletions(-) > > > > diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c > > index 4640dc59..3f58969d 100644 > > --- a/libsepol/cil/src/cil_verify.c > > +++ b/libsepol/cil/src/cil_verify.c > > @@ -1700,31 +1700,109 @@ static int __add_perm_to_list(__attribute__((unused)) hashtab_key_t k, hashtab_d > > return SEPOL_OK; > > } > > > > -static int __cil_verify_classperms(struct cil_list *classperms, > > - struct cil_symtab_datum *orig, > > - struct cil_symtab_datum *parent, > > - struct cil_symtab_datum *cur, > > - enum cil_flavor flavor, > > - unsigned steps, unsigned limit) > > +static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symtab_datum *orig, struct cil_symtab_datum *cur, unsigned steps, unsigned limit); > > + > > +static int __cil_verify_map_perm(struct cil_class *class, struct cil_perm *perm, struct cil_symtab_datum *orig, unsigned steps, unsigned limit) > > +{ > > + int rc; > > + > > + if (!perm->classperms) { > > + cil_tree_log(NODE(class), CIL_ERR, "No class permissions for map class %s, permission %s", DATUM(class)->name, DATUM(perm)->name); > > + goto exit; > > + } > > + > > + rc = __cil_verify_classperms(perm->classperms, orig, &perm->datum, steps, limit); > > + if (rc != SEPOL_OK) { > > + cil_tree_log(NODE(class), CIL_ERR, "There was an error verifying class permissions for map class %s, permission %s", DATUM(class)->name, DATUM(perm)->name); > > + goto exit; > > + } > > + > > + return SEPOL_OK; > > + > > +exit: > > + return SEPOL_ERR; > > +} > > + > > + > > +static int __cil_verify_perms(struct cil_class *class, struct cil_list *perms, struct cil_symtab_datum *orig, unsigned steps, unsigned limit) > > { > > int rc = SEPOL_ERR; > > - struct cil_list_item *curr; > > + int count = 0; > > + struct cil_list_item *i = NULL; > > > > - if (classperms == NULL) { > > - if (flavor == CIL_MAP_PERM) { > > - cil_tree_log(NODE(cur), CIL_ERR, "Map class %s does not have a classmapping for %s", parent->name, cur->name); > > + if (!perms) { > > + cil_tree_log(NODE(class), CIL_ERR, "No permissions for class %s in class permissions", DATUM(class)->name); > > + goto exit; > > + } > > + > > + cil_list_for_each(i, perms) { > > + count++; > > + if (i->flavor == CIL_LIST) { > > + rc = __cil_verify_perms(class, i->data, orig, steps, limit); > > + if (rc != SEPOL_OK) { > > + goto exit; > > + } > > + } else if (i->flavor == CIL_DATUM) { > > + struct cil_perm *perm = i->data; > > + if (FLAVOR(perm) == CIL_MAP_PERM) { > > + rc = __cil_verify_map_perm(class, perm, orig, steps, limit); > > + if (rc != SEPOL_OK) { > > + goto exit; > > + } > > + } > > + } else if (i->flavor == CIL_OP) { > > + enum cil_flavor op = (enum cil_flavor)(uintptr_t)i->data; > > + if (op == CIL_ALL) { > > + struct cil_list *perm_list; > > + struct cil_list_item *j = NULL; > > + int count2 = 0; > > + cil_list_init(&perm_list, CIL_MAP_PERM); > > + cil_symtab_map(&class->perms, __add_perm_to_list, perm_list); > > + cil_list_for_each(j, perm_list) { > > + count2++; > > + struct cil_perm *perm = j->data; > > + if (FLAVOR(perm) == CIL_MAP_PERM) { > > + rc = __cil_verify_map_perm(class, perm, orig, steps, limit); > > + if (rc != SEPOL_OK) { > > + cil_list_destroy(&perm_list, CIL_FALSE); > > + goto exit; > > + } > > + } > > + } > > + cil_list_destroy(&perm_list, CIL_FALSE); > > + if (count2 == 0) { > > + cil_tree_log(NODE(class), CIL_ERR, "Operator \"all\" used for %s which has no permissions associated with it", DATUM(class)->name); > > + goto exit; > > + } > > + } > > } else { > > - cil_tree_log(NODE(cur), CIL_ERR, "Classpermission %s does not have a classpermissionset", cur->name); > > + cil_tree_log(NODE(class), CIL_ERR, "Permission list for %s has an unexpected flavor: %d", DATUM(class)->name, i->flavor); > > + goto exit; > > } > > + } > > + > > + if (count == 0) { > > + cil_tree_log(NODE(class), CIL_ERR, "Empty permissions list for class %s in class permissions", DATUM(class)->name); > > + goto exit; > > + } > > + > > + return SEPOL_OK; > > + > > +exit: > > + return SEPOL_ERR; > > +} > > + > > +static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symtab_datum *orig, struct cil_symtab_datum *cur, unsigned steps, unsigned limit) > > +{ > > + int rc; > > + struct cil_list_item *i; > > + > > + if (classperms == NULL) { > > goto exit; > > } > > > > if (steps > 0 && orig == cur) { > > - if (flavor == CIL_MAP_PERM) { > > - cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving the map class %s and permission %s", parent->name, cur->name); > > - } else { > > - cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving the set %s", cur->name); > > - } > > + cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving %s", cur->name); > > goto exit; > > } else { > > steps++; > > @@ -1735,44 +1813,20 @@ static int __cil_verify_classperms(struct cil_list *classperms, > > } > > } > > > > - cil_list_for_each(curr, classperms) { > > - if (curr->flavor == CIL_CLASSPERMS) { > > - struct cil_classperms *cp = curr->data; > > - if (FLAVOR(cp->class) != CIL_CLASS) { /* MAP */ > > - struct cil_list_item *i = NULL; > > - cil_list_for_each(i, cp->perms) { > > - if (i->flavor != CIL_OP) { > > - struct cil_perm *cmp = i->data; > > - rc = __cil_verify_classperms(cmp->classperms, orig, &cp->class->datum, &cmp->datum, CIL_MAP_PERM, steps, limit); > > - if (rc != SEPOL_OK) { > > - goto exit; > > - } > > - } else { > > - enum cil_flavor op = (enum cil_flavor)(uintptr_t)i->data; > > - if (op == CIL_ALL) { > > - struct cil_class *mc = cp->class; > > - struct cil_list *perm_list; > > - struct cil_list_item *j = NULL; > > - > > - cil_list_init(&perm_list, CIL_MAP_PERM); > > - cil_symtab_map(&mc->perms, __add_perm_to_list, perm_list); > > - cil_list_for_each(j, perm_list) { > > - struct cil_perm *cmp = j->data; > > - rc = __cil_verify_classperms(cmp->classperms, orig, &cp->class->datum, &cmp->datum, CIL_MAP_PERM, steps, limit); > > - if (rc != SEPOL_OK) { > > - cil_list_destroy(&perm_list, CIL_FALSE); > > - goto exit; > > - } > > - } > > - cil_list_destroy(&perm_list, CIL_FALSE); > > - } > > - } > > - } > > + cil_list_for_each(i, classperms) { > > + if (i->flavor == CIL_CLASSPERMS) { > > + struct cil_classperms *cp = i->data; > > + rc = __cil_verify_perms(cp->class, cp->perms, orig, steps, limit); > > + if (rc != SEPOL_OK) { > > + goto exit; > > } > > } else { /* SET */ > > - struct cil_classperms_set *cp_set = curr->data; > > + struct cil_classperms_set *cp_set = i->data; > > struct cil_classpermission *cp = cp_set->set; > > - rc = __cil_verify_classperms(cp->classperms, orig, NULL, &cp->datum, CIL_CLASSPERMISSION, steps, limit); > > + if (!cp->classperms) { > > + cil_tree_log(NODE(cur), CIL_ERR, "Classpermission %s does not have a classpermissionset", DATUM(cp)->name); > > + } > > + rc = __cil_verify_classperms(cp->classperms, orig, &cp->datum, steps, limit); > > if (rc != SEPOL_OK) { > > goto exit; > > } > > @@ -1787,9 +1841,15 @@ exit: > > > > static int __cil_verify_classpermission(struct cil_tree_node *node) > > { > > + int rc; > > struct cil_classpermission *cp = node->data; > > > > - return __cil_verify_classperms(cp->classperms, &cp->datum, NULL, &cp->datum, CIL_CLASSPERMISSION, 0, 2); > > + rc = __cil_verify_classperms(cp->classperms, &cp->datum, &cp->datum, 0, 2); > > + if (rc != SEPOL_OK) { > > + cil_tree_log(node, CIL_ERR, "Error verifying class permissions for classpermission %s", DATUM(cp)->name); > > + } > > + > > + return rc; > > } > > > > struct cil_verify_map_args { > > @@ -1804,8 +1864,9 @@ static int __verify_map_perm_classperms(__attribute__((unused)) hashtab_key_t k, > > struct cil_perm *cmp = (struct cil_perm *)d; > > int rc; > > > > - rc = __cil_verify_classperms(cmp->classperms, &cmp->datum, &map_args->class->datum, &cmp->datum, CIL_MAP_PERM, 0, 2); > > + rc = __cil_verify_classperms(cmp->classperms, &cmp->datum, &cmp->datum, 0, 2); > > if (rc != SEPOL_OK) { > > + cil_tree_log(NODE(cmp), CIL_ERR, "Error verifying class permissions for map class %s, permission %s", DATUM(map_args->class)->name, DATUM(cmp)->name); > > map_args->rc = rc; > > } > >
On Thu, Aug 3, 2023 at 4:34 PM James Carter <jwcart2@gmail.com> wrote: > > On Wed, Jul 12, 2023 at 12:01 PM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > On 4/20/23 14:58, James Carter wrote: > > > Before the CIL post processing phase (where expressions are evaluated, > > > various ebitmaps are set, etc) there is a pre-verification where > > > checks are made to find self references or loops in bounds, attribute > > > sets, and class permissions. The class permission checking is faulty > > > in two ways. > > > > > > First, it does not check for the use of "all" in a permission expression > > > for a class that has no permissions. An error will still be generated > > > later and secilc will exit cleanly, but without an error message that > > > explains the problem. > > > > > > Second, it does not properly handle lists in permission expressions. > > > For example, "(C ((P)))" is a legitimate class permission. The > > > permissions expression contains one item that is a list containing > > > one permission. This permission expression will be properly evaluated. > > > Unfortunately, the class permission verification assumes that each > > > item in the permission expression is either an operator or a > > > permission datum and a segmenation fault will occur. > > > > > > Refactor the class permission checking to give a proper error when > > > "all" is used in a permission expression for a class that has no > > > permissions and so that it can handle lists in permission > > > expressions. Also, check for the actual flavor of each item in > > > the permission expression and return an error if an unexpected > > > flavor is found. > > > > > > The failure to properly handle lists in permission expressions was > > > found by oss-fuzz (#58085). > > > > > > For what it's worth: > > > > Verified the fuzzer no longer crashes and no new issues arose after > > running for ~1h. > > > > Successfully build DSSP5. > > > > > > Tested-by: Christian Göttsche <cgzones@googlemail.com> > > > > I plan on merging this soon, unless I hear any objections. > Jim > This patch has been merged. Jim > > > > > > > > > Signed-off-by: James Carter <jwcart2@gmail.com> > > > --- > > > libsepol/cil/src/cil_verify.c | 167 +++++++++++++++++++++++----------- > > > 1 file changed, 114 insertions(+), 53 deletions(-) > > > > > > diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c > > > index 4640dc59..3f58969d 100644 > > > --- a/libsepol/cil/src/cil_verify.c > > > +++ b/libsepol/cil/src/cil_verify.c > > > @@ -1700,31 +1700,109 @@ static int __add_perm_to_list(__attribute__((unused)) hashtab_key_t k, hashtab_d > > > return SEPOL_OK; > > > } > > > > > > -static int __cil_verify_classperms(struct cil_list *classperms, > > > - struct cil_symtab_datum *orig, > > > - struct cil_symtab_datum *parent, > > > - struct cil_symtab_datum *cur, > > > - enum cil_flavor flavor, > > > - unsigned steps, unsigned limit) > > > +static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symtab_datum *orig, struct cil_symtab_datum *cur, unsigned steps, unsigned limit); > > > + > > > +static int __cil_verify_map_perm(struct cil_class *class, struct cil_perm *perm, struct cil_symtab_datum *orig, unsigned steps, unsigned limit) > > > +{ > > > + int rc; > > > + > > > + if (!perm->classperms) { > > > + cil_tree_log(NODE(class), CIL_ERR, "No class permissions for map class %s, permission %s", DATUM(class)->name, DATUM(perm)->name); > > > + goto exit; > > > + } > > > + > > > + rc = __cil_verify_classperms(perm->classperms, orig, &perm->datum, steps, limit); > > > + if (rc != SEPOL_OK) { > > > + cil_tree_log(NODE(class), CIL_ERR, "There was an error verifying class permissions for map class %s, permission %s", DATUM(class)->name, DATUM(perm)->name); > > > + goto exit; > > > + } > > > + > > > + return SEPOL_OK; > > > + > > > +exit: > > > + return SEPOL_ERR; > > > +} > > > + > > > + > > > +static int __cil_verify_perms(struct cil_class *class, struct cil_list *perms, struct cil_symtab_datum *orig, unsigned steps, unsigned limit) > > > { > > > int rc = SEPOL_ERR; > > > - struct cil_list_item *curr; > > > + int count = 0; > > > + struct cil_list_item *i = NULL; > > > > > > - if (classperms == NULL) { > > > - if (flavor == CIL_MAP_PERM) { > > > - cil_tree_log(NODE(cur), CIL_ERR, "Map class %s does not have a classmapping for %s", parent->name, cur->name); > > > + if (!perms) { > > > + cil_tree_log(NODE(class), CIL_ERR, "No permissions for class %s in class permissions", DATUM(class)->name); > > > + goto exit; > > > + } > > > + > > > + cil_list_for_each(i, perms) { > > > + count++; > > > + if (i->flavor == CIL_LIST) { > > > + rc = __cil_verify_perms(class, i->data, orig, steps, limit); > > > + if (rc != SEPOL_OK) { > > > + goto exit; > > > + } > > > + } else if (i->flavor == CIL_DATUM) { > > > + struct cil_perm *perm = i->data; > > > + if (FLAVOR(perm) == CIL_MAP_PERM) { > > > + rc = __cil_verify_map_perm(class, perm, orig, steps, limit); > > > + if (rc != SEPOL_OK) { > > > + goto exit; > > > + } > > > + } > > > + } else if (i->flavor == CIL_OP) { > > > + enum cil_flavor op = (enum cil_flavor)(uintptr_t)i->data; > > > + if (op == CIL_ALL) { > > > + struct cil_list *perm_list; > > > + struct cil_list_item *j = NULL; > > > + int count2 = 0; > > > + cil_list_init(&perm_list, CIL_MAP_PERM); > > > + cil_symtab_map(&class->perms, __add_perm_to_list, perm_list); > > > + cil_list_for_each(j, perm_list) { > > > + count2++; > > > + struct cil_perm *perm = j->data; > > > + if (FLAVOR(perm) == CIL_MAP_PERM) { > > > + rc = __cil_verify_map_perm(class, perm, orig, steps, limit); > > > + if (rc != SEPOL_OK) { > > > + cil_list_destroy(&perm_list, CIL_FALSE); > > > + goto exit; > > > + } > > > + } > > > + } > > > + cil_list_destroy(&perm_list, CIL_FALSE); > > > + if (count2 == 0) { > > > + cil_tree_log(NODE(class), CIL_ERR, "Operator \"all\" used for %s which has no permissions associated with it", DATUM(class)->name); > > > + goto exit; > > > + } > > > + } > > > } else { > > > - cil_tree_log(NODE(cur), CIL_ERR, "Classpermission %s does not have a classpermissionset", cur->name); > > > + cil_tree_log(NODE(class), CIL_ERR, "Permission list for %s has an unexpected flavor: %d", DATUM(class)->name, i->flavor); > > > + goto exit; > > > } > > > + } > > > + > > > + if (count == 0) { > > > + cil_tree_log(NODE(class), CIL_ERR, "Empty permissions list for class %s in class permissions", DATUM(class)->name); > > > + goto exit; > > > + } > > > + > > > + return SEPOL_OK; > > > + > > > +exit: > > > + return SEPOL_ERR; > > > +} > > > + > > > +static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symtab_datum *orig, struct cil_symtab_datum *cur, unsigned steps, unsigned limit) > > > +{ > > > + int rc; > > > + struct cil_list_item *i; > > > + > > > + if (classperms == NULL) { > > > goto exit; > > > } > > > > > > if (steps > 0 && orig == cur) { > > > - if (flavor == CIL_MAP_PERM) { > > > - cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving the map class %s and permission %s", parent->name, cur->name); > > > - } else { > > > - cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving the set %s", cur->name); > > > - } > > > + cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving %s", cur->name); > > > goto exit; > > > } else { > > > steps++; > > > @@ -1735,44 +1813,20 @@ static int __cil_verify_classperms(struct cil_list *classperms, > > > } > > > } > > > > > > - cil_list_for_each(curr, classperms) { > > > - if (curr->flavor == CIL_CLASSPERMS) { > > > - struct cil_classperms *cp = curr->data; > > > - if (FLAVOR(cp->class) != CIL_CLASS) { /* MAP */ > > > - struct cil_list_item *i = NULL; > > > - cil_list_for_each(i, cp->perms) { > > > - if (i->flavor != CIL_OP) { > > > - struct cil_perm *cmp = i->data; > > > - rc = __cil_verify_classperms(cmp->classperms, orig, &cp->class->datum, &cmp->datum, CIL_MAP_PERM, steps, limit); > > > - if (rc != SEPOL_OK) { > > > - goto exit; > > > - } > > > - } else { > > > - enum cil_flavor op = (enum cil_flavor)(uintptr_t)i->data; > > > - if (op == CIL_ALL) { > > > - struct cil_class *mc = cp->class; > > > - struct cil_list *perm_list; > > > - struct cil_list_item *j = NULL; > > > - > > > - cil_list_init(&perm_list, CIL_MAP_PERM); > > > - cil_symtab_map(&mc->perms, __add_perm_to_list, perm_list); > > > - cil_list_for_each(j, perm_list) { > > > - struct cil_perm *cmp = j->data; > > > - rc = __cil_verify_classperms(cmp->classperms, orig, &cp->class->datum, &cmp->datum, CIL_MAP_PERM, steps, limit); > > > - if (rc != SEPOL_OK) { > > > - cil_list_destroy(&perm_list, CIL_FALSE); > > > - goto exit; > > > - } > > > - } > > > - cil_list_destroy(&perm_list, CIL_FALSE); > > > - } > > > - } > > > - } > > > + cil_list_for_each(i, classperms) { > > > + if (i->flavor == CIL_CLASSPERMS) { > > > + struct cil_classperms *cp = i->data; > > > + rc = __cil_verify_perms(cp->class, cp->perms, orig, steps, limit); > > > + if (rc != SEPOL_OK) { > > > + goto exit; > > > } > > > } else { /* SET */ > > > - struct cil_classperms_set *cp_set = curr->data; > > > + struct cil_classperms_set *cp_set = i->data; > > > struct cil_classpermission *cp = cp_set->set; > > > - rc = __cil_verify_classperms(cp->classperms, orig, NULL, &cp->datum, CIL_CLASSPERMISSION, steps, limit); > > > + if (!cp->classperms) { > > > + cil_tree_log(NODE(cur), CIL_ERR, "Classpermission %s does not have a classpermissionset", DATUM(cp)->name); > > > + } > > > + rc = __cil_verify_classperms(cp->classperms, orig, &cp->datum, steps, limit); > > > if (rc != SEPOL_OK) { > > > goto exit; > > > } > > > @@ -1787,9 +1841,15 @@ exit: > > > > > > static int __cil_verify_classpermission(struct cil_tree_node *node) > > > { > > > + int rc; > > > struct cil_classpermission *cp = node->data; > > > > > > - return __cil_verify_classperms(cp->classperms, &cp->datum, NULL, &cp->datum, CIL_CLASSPERMISSION, 0, 2); > > > + rc = __cil_verify_classperms(cp->classperms, &cp->datum, &cp->datum, 0, 2); > > > + if (rc != SEPOL_OK) { > > > + cil_tree_log(node, CIL_ERR, "Error verifying class permissions for classpermission %s", DATUM(cp)->name); > > > + } > > > + > > > + return rc; > > > } > > > > > > struct cil_verify_map_args { > > > @@ -1804,8 +1864,9 @@ static int __verify_map_perm_classperms(__attribute__((unused)) hashtab_key_t k, > > > struct cil_perm *cmp = (struct cil_perm *)d; > > > int rc; > > > > > > - rc = __cil_verify_classperms(cmp->classperms, &cmp->datum, &map_args->class->datum, &cmp->datum, CIL_MAP_PERM, 0, 2); > > > + rc = __cil_verify_classperms(cmp->classperms, &cmp->datum, &cmp->datum, 0, 2); > > > if (rc != SEPOL_OK) { > > > + cil_tree_log(NODE(cmp), CIL_ERR, "Error verifying class permissions for map class %s, permission %s", DATUM(map_args->class)->name, DATUM(cmp)->name); > > > map_args->rc = rc; > > > } > > >
diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c index 4640dc59..3f58969d 100644 --- a/libsepol/cil/src/cil_verify.c +++ b/libsepol/cil/src/cil_verify.c @@ -1700,31 +1700,109 @@ static int __add_perm_to_list(__attribute__((unused)) hashtab_key_t k, hashtab_d return SEPOL_OK; } -static int __cil_verify_classperms(struct cil_list *classperms, - struct cil_symtab_datum *orig, - struct cil_symtab_datum *parent, - struct cil_symtab_datum *cur, - enum cil_flavor flavor, - unsigned steps, unsigned limit) +static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symtab_datum *orig, struct cil_symtab_datum *cur, unsigned steps, unsigned limit); + +static int __cil_verify_map_perm(struct cil_class *class, struct cil_perm *perm, struct cil_symtab_datum *orig, unsigned steps, unsigned limit) +{ + int rc; + + if (!perm->classperms) { + cil_tree_log(NODE(class), CIL_ERR, "No class permissions for map class %s, permission %s", DATUM(class)->name, DATUM(perm)->name); + goto exit; + } + + rc = __cil_verify_classperms(perm->classperms, orig, &perm->datum, steps, limit); + if (rc != SEPOL_OK) { + cil_tree_log(NODE(class), CIL_ERR, "There was an error verifying class permissions for map class %s, permission %s", DATUM(class)->name, DATUM(perm)->name); + goto exit; + } + + return SEPOL_OK; + +exit: + return SEPOL_ERR; +} + + +static int __cil_verify_perms(struct cil_class *class, struct cil_list *perms, struct cil_symtab_datum *orig, unsigned steps, unsigned limit) { int rc = SEPOL_ERR; - struct cil_list_item *curr; + int count = 0; + struct cil_list_item *i = NULL; - if (classperms == NULL) { - if (flavor == CIL_MAP_PERM) { - cil_tree_log(NODE(cur), CIL_ERR, "Map class %s does not have a classmapping for %s", parent->name, cur->name); + if (!perms) { + cil_tree_log(NODE(class), CIL_ERR, "No permissions for class %s in class permissions", DATUM(class)->name); + goto exit; + } + + cil_list_for_each(i, perms) { + count++; + if (i->flavor == CIL_LIST) { + rc = __cil_verify_perms(class, i->data, orig, steps, limit); + if (rc != SEPOL_OK) { + goto exit; + } + } else if (i->flavor == CIL_DATUM) { + struct cil_perm *perm = i->data; + if (FLAVOR(perm) == CIL_MAP_PERM) { + rc = __cil_verify_map_perm(class, perm, orig, steps, limit); + if (rc != SEPOL_OK) { + goto exit; + } + } + } else if (i->flavor == CIL_OP) { + enum cil_flavor op = (enum cil_flavor)(uintptr_t)i->data; + if (op == CIL_ALL) { + struct cil_list *perm_list; + struct cil_list_item *j = NULL; + int count2 = 0; + cil_list_init(&perm_list, CIL_MAP_PERM); + cil_symtab_map(&class->perms, __add_perm_to_list, perm_list); + cil_list_for_each(j, perm_list) { + count2++; + struct cil_perm *perm = j->data; + if (FLAVOR(perm) == CIL_MAP_PERM) { + rc = __cil_verify_map_perm(class, perm, orig, steps, limit); + if (rc != SEPOL_OK) { + cil_list_destroy(&perm_list, CIL_FALSE); + goto exit; + } + } + } + cil_list_destroy(&perm_list, CIL_FALSE); + if (count2 == 0) { + cil_tree_log(NODE(class), CIL_ERR, "Operator \"all\" used for %s which has no permissions associated with it", DATUM(class)->name); + goto exit; + } + } } else { - cil_tree_log(NODE(cur), CIL_ERR, "Classpermission %s does not have a classpermissionset", cur->name); + cil_tree_log(NODE(class), CIL_ERR, "Permission list for %s has an unexpected flavor: %d", DATUM(class)->name, i->flavor); + goto exit; } + } + + if (count == 0) { + cil_tree_log(NODE(class), CIL_ERR, "Empty permissions list for class %s in class permissions", DATUM(class)->name); + goto exit; + } + + return SEPOL_OK; + +exit: + return SEPOL_ERR; +} + +static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symtab_datum *orig, struct cil_symtab_datum *cur, unsigned steps, unsigned limit) +{ + int rc; + struct cil_list_item *i; + + if (classperms == NULL) { goto exit; } if (steps > 0 && orig == cur) { - if (flavor == CIL_MAP_PERM) { - cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving the map class %s and permission %s", parent->name, cur->name); - } else { - cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving the set %s", cur->name); - } + cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving %s", cur->name); goto exit; } else { steps++; @@ -1735,44 +1813,20 @@ static int __cil_verify_classperms(struct cil_list *classperms, } } - cil_list_for_each(curr, classperms) { - if (curr->flavor == CIL_CLASSPERMS) { - struct cil_classperms *cp = curr->data; - if (FLAVOR(cp->class) != CIL_CLASS) { /* MAP */ - struct cil_list_item *i = NULL; - cil_list_for_each(i, cp->perms) { - if (i->flavor != CIL_OP) { - struct cil_perm *cmp = i->data; - rc = __cil_verify_classperms(cmp->classperms, orig, &cp->class->datum, &cmp->datum, CIL_MAP_PERM, steps, limit); - if (rc != SEPOL_OK) { - goto exit; - } - } else { - enum cil_flavor op = (enum cil_flavor)(uintptr_t)i->data; - if (op == CIL_ALL) { - struct cil_class *mc = cp->class; - struct cil_list *perm_list; - struct cil_list_item *j = NULL; - - cil_list_init(&perm_list, CIL_MAP_PERM); - cil_symtab_map(&mc->perms, __add_perm_to_list, perm_list); - cil_list_for_each(j, perm_list) { - struct cil_perm *cmp = j->data; - rc = __cil_verify_classperms(cmp->classperms, orig, &cp->class->datum, &cmp->datum, CIL_MAP_PERM, steps, limit); - if (rc != SEPOL_OK) { - cil_list_destroy(&perm_list, CIL_FALSE); - goto exit; - } - } - cil_list_destroy(&perm_list, CIL_FALSE); - } - } - } + cil_list_for_each(i, classperms) { + if (i->flavor == CIL_CLASSPERMS) { + struct cil_classperms *cp = i->data; + rc = __cil_verify_perms(cp->class, cp->perms, orig, steps, limit); + if (rc != SEPOL_OK) { + goto exit; } } else { /* SET */ - struct cil_classperms_set *cp_set = curr->data; + struct cil_classperms_set *cp_set = i->data; struct cil_classpermission *cp = cp_set->set; - rc = __cil_verify_classperms(cp->classperms, orig, NULL, &cp->datum, CIL_CLASSPERMISSION, steps, limit); + if (!cp->classperms) { + cil_tree_log(NODE(cur), CIL_ERR, "Classpermission %s does not have a classpermissionset", DATUM(cp)->name); + } + rc = __cil_verify_classperms(cp->classperms, orig, &cp->datum, steps, limit); if (rc != SEPOL_OK) { goto exit; } @@ -1787,9 +1841,15 @@ exit: static int __cil_verify_classpermission(struct cil_tree_node *node) { + int rc; struct cil_classpermission *cp = node->data; - return __cil_verify_classperms(cp->classperms, &cp->datum, NULL, &cp->datum, CIL_CLASSPERMISSION, 0, 2); + rc = __cil_verify_classperms(cp->classperms, &cp->datum, &cp->datum, 0, 2); + if (rc != SEPOL_OK) { + cil_tree_log(node, CIL_ERR, "Error verifying class permissions for classpermission %s", DATUM(cp)->name); + } + + return rc; } struct cil_verify_map_args { @@ -1804,8 +1864,9 @@ static int __verify_map_perm_classperms(__attribute__((unused)) hashtab_key_t k, struct cil_perm *cmp = (struct cil_perm *)d; int rc; - rc = __cil_verify_classperms(cmp->classperms, &cmp->datum, &map_args->class->datum, &cmp->datum, CIL_MAP_PERM, 0, 2); + rc = __cil_verify_classperms(cmp->classperms, &cmp->datum, &cmp->datum, 0, 2); if (rc != SEPOL_OK) { + cil_tree_log(NODE(cmp), CIL_ERR, "Error verifying class permissions for map class %s, permission %s", DATUM(map_args->class)->name, DATUM(cmp)->name); map_args->rc = rc; }
Before the CIL post processing phase (where expressions are evaluated, various ebitmaps are set, etc) there is a pre-verification where checks are made to find self references or loops in bounds, attribute sets, and class permissions. The class permission checking is faulty in two ways. First, it does not check for the use of "all" in a permission expression for a class that has no permissions. An error will still be generated later and secilc will exit cleanly, but without an error message that explains the problem. Second, it does not properly handle lists in permission expressions. For example, "(C ((P)))" is a legitimate class permission. The permissions expression contains one item that is a list containing one permission. This permission expression will be properly evaluated. Unfortunately, the class permission verification assumes that each item in the permission expression is either an operator or a permission datum and a segmenation fault will occur. Refactor the class permission checking to give a proper error when "all" is used in a permission expression for a class that has no permissions and so that it can handle lists in permission expressions. Also, check for the actual flavor of each item in the permission expression and return an error if an unexpected flavor is found. The failure to properly handle lists in permission expressions was found by oss-fuzz (#58085). Signed-off-by: James Carter <jwcart2@gmail.com> --- libsepol/cil/src/cil_verify.c | 167 +++++++++++++++++++++++----------- 1 file changed, 114 insertions(+), 53 deletions(-)