On Wed, Jul 12, 2023 at 12:01 PM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>
I plan on merging this soon, unless I hear any objections.
Jim
>
> >
> > Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> > ---
> > 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;
> > }
> >
