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?
> 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.
> + 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
>
>
