On Wed, Aug 14, 2024 at 12:06 PM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> On Mon, 12 Aug 2024 at 21:41, James Carter <jwcart2@xxxxxxxxx> wrote:
> >
> > When a user is created in an optional block, a user datum is added
> > to both the avrule_decl's symtab and the policydb's symtab, but
> > the semantic MLS information is only added to the avrule_decl's
> > user datum. This causes an error to occur during policy expansion
> > when user_copy_callback() is called. If this error did not occur
> > then the policydb's user datum would be written without any MLS
> > info and the policy would fail validation when read later.
> >
> > When creating a user datum, search for a user datum with the same
> > key in the policydb's symtab. If that datum has no MLS information,
> > then copy the MLS information from the avrule_decl's datum. If it
> > does, then compare the default level, low level, and high level
> > sensitivities and give an error if they do not match. There is not
> > enough information to expand the categories for the high and low
> > levels, so merge the semantic categories. If the two category sets
> > are not equal an error will occur during the expansion phase.
> >
> > A minimum policy to demonstrate the bug:
> > class CLASS1
> > sid kernel
> > class CLASS1 { PERM1 }
> > sensitivity SENS1;
> > dominance { SENS1 }
> > level SENS1;
> > mlsconstrain CLASS1 { PERM1 } ((h1 dom h2) and (l1 domby h1));
> > type TYPE1;
> > allow TYPE1 self : CLASS1 PERM1;
> > role ROLE1;
> > role ROLE1 types TYPE1;
> > optional {
> > require {
> > role ROLE1;
> > }
> > user USER2 roles ROLE1 level SENS1 range SENS1;
> > }
>
> Maybe add this optional block to checkpolicy/tests/policy_allonce_mls.conf?
>
Yes, eventually. I've been working on testing all the valid syntax
options for each rule and block (which is how I found this bug).
> > user USER1 roles ROLE1 level SENS1 range SENS1;
> > sid kernel USER1:ROLE1:TYPE1:SENS1
> >
> > Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> > ---
> > checkpolicy/policy_define.c | 76 ++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 75 insertions(+), 1 deletion(-)
> >
> > diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
> > index bfeda86b..93a1397e 100644
> > --- a/checkpolicy/policy_define.c
> > +++ b/checkpolicy/policy_define.c
> > @@ -4175,6 +4175,55 @@ static int parse_categories(char *id, level_datum_t * levdatum, ebitmap_t * cats
> > return 0;
> > }
> >
> > +static int mls_semantic_cats_merge(mls_semantic_cat_t ** dst,
> > + const mls_semantic_cat_t * src)
> > +{
> > + mls_semantic_cat_t *new, *dcat;
> > +
> > + dcat = *dst;
>
> Does this always work?
> *dst is the pointer to the first category (which might be NULL).
> What if the list is not empty, so dcat->next is not NULL, then we
> would override it later and lose the existing tail of the list.
No, it doesn't work. I wasn't really thinking and just used the syntax
from mls_semantic_level_cpy(), which, of course, is not used when the
level already has values.
>
> > + while (src) {
> > + new = (mls_semantic_cat_t *) malloc(sizeof(mls_semantic_cat_t));
> > + if (!new)
> > + return -1;
> > +
> > + mls_semantic_cat_init(new);
> > + new->low = src->low;
> > + new->high = src->high;
> > +
> > + if (dcat)
> > + dcat->next = new;
> > + else
> > + *dst = new;
> > + dcat = new;
> > +
> > + src = src->next;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mls_add_or_check_level(mls_semantic_level_t *dst, mls_semantic_level_t *src)
>
> src can be declared const
>
That is true.
> > +{
> > + if (!dst->sens) {
> > + if (mls_semantic_level_cpy(dst, src) < 0) {
> > + yyerror("out of memory");
> > + return -1;
> > + }
> > + } else {
> > + if (dst->sens != src->sens) {
> > + return -1;
> > + }
> > + /* Duplicate cats won't cause problems, but different cats will
> > + * result in an error during expansion */
> > + if (mls_semantic_cats_merge(&dst->cat, src->cat) < 0) {
> > + yyerror("out of memory");
> > + return -1;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int parse_semantic_categories(char *id, level_datum_t * levdatum __attribute__ ((unused)),
> > mls_semantic_cat_t ** cats)
> > {
> > @@ -4233,7 +4282,7 @@ static int parse_semantic_categories(char *id, level_datum_t * levdatum __attrib
> > int define_user(void)
> > {
> > char *id;
> > - user_datum_t *usrdatum;
> > + user_datum_t *usrdatum, *usr_global;
> > level_datum_t *levdatum;
> > int l;
> >
> > @@ -4258,10 +4307,15 @@ int define_user(void)
> > return 0;
> > }
> >
> > + id = strdup(queue_head(id_queue));
> > +
> > if ((usrdatum = declare_user()) == NULL) {
>
> free(id);
>
I actually free it below after the hashtab_search(). I have to copy it
here because declare_user() pulls things from the queue.
Thanks for the review.
Jim
> > return -1;
> > }
> >
> > + usr_global = hashtab_search(policydbp->p_users.table, (hashtab_key_t) id);
> > + free(id);
> > +
> > while ((id = queue_remove(id_queue))) {
> > if (set_user_roles(&usrdatum->roles, id))
> > return -1;
> > @@ -4288,6 +4342,7 @@ int define_user(void)
> > usrdatum->dfltlevel.sens = levdatum->level->sens;
> >
> > while ((id = queue_remove(id_queue))) {
> > + /* This will add to any already existing categories */
> > if (parse_semantic_categories(id, levdatum,
> > &usrdatum->dfltlevel.cat)) {
> > free(id);
> > @@ -4313,6 +4368,7 @@ int define_user(void)
> > usrdatum->range.level[l].sens = levdatum->level->sens;
> >
> > while ((id = queue_remove(id_queue))) {
> > + /* This will add to any already existing categories */
> > if (parse_semantic_categories(id, levdatum,
> > &usrdatum->range.level[l].cat)) {
> > free(id);
> > @@ -4333,6 +4389,24 @@ int define_user(void)
> > return -1;
> > }
> > }
> > +
> > + if (usr_global && usr_global != usrdatum) {
> > + if (mls_add_or_check_level(&usr_global->dfltlevel,
> > + &usrdatum->dfltlevel)) {
> > + yyerror("Problem with user default level");
> > + return -1;
> > + }
> > + if (mls_add_or_check_level(&usr_global->range.level[0],
> > + &usrdatum->range.level[0])) {
> > + yyerror("Problem with user low level");
> > + return -1;
> > + }
> > + if (mls_add_or_check_level(&usr_global->range.level[1],
> > + &usrdatum->range.level[1])) {
> > + yyerror("Problem with user high level");
> > + return -1;
> > + }
> > + }
> > }
> > return 0;
> > }
> > --
> > 2.46.0
> >
> >
