| Message ID | 20240812194051.66157-1-jwcart2@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Petr Lautrbach |
| Headers | show |
| Series | checkpolicy: Fix MLS users in optional blocks | expand |
On Mon, 12 Aug 2024 at 21:41, James Carter <jwcart2@gmail.com> 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? > user USER1 roles ROLE1 level SENS1 range SENS1; > sid kernel USER1:ROLE1:TYPE1:SENS1 > > Signed-off-by: James Carter <jwcart2@gmail.com> > --- > 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. > + 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 > +{ > + 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); > 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 > >
On Wed, Aug 14, 2024 at 12:06 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > On Mon, 12 Aug 2024 at 21:41, James Carter <jwcart2@gmail.com> 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@gmail.com> > > --- > > 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 > > > >
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; + 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) +{ + 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) { 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; }
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; } user USER1 roles ROLE1 level SENS1 range SENS1; sid kernel USER1:ROLE1:TYPE1:SENS1 Signed-off-by: James Carter <jwcart2@gmail.com> --- checkpolicy/policy_define.c | 76 ++++++++++++++++++++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-)