On Thu, Aug 3, 2023 at 4:34 PM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> 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
>
This patch has been merged.
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;
> > > }
> > >
