On Mon, Mar 8, 2021 at 9:50 PM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> Roles in an optional block have two datums, one in the global block
> and one in the avrule_decl where it is declared. The datum in the
> global block does not have its dominace set. This is a problem because
> the function set_user_role() sets the user's roles based on the global
> datum's dominance ebitmap. If a user is declared with an associated role
> that was declared in an optional block, then it will not have any roles
> set for it because the dominance ebitmap is empty.
>
> Example/
> # handle_unknown deny
> class CLASS1
> sid kernel
> class CLASS1 { PERM1 }
> type TYPE1;
> allow TYPE1 self:CLASS1 PERM1;
> role ROLE1;
> role ROLE1 types { TYPE1 };
> optional {
> require {
> class CLASS1 { PERM1 };
> }
> role ROLE1A;
> user USER1A roles ROLE1A;
> }
> user USER1 roles ROLE1;
> sid kernel USER1:ROLE1:TYPE1
>
> In this example, USER1A would not have ROLE1A associated with it.
>
> Instead of using dominance, which has been deprecated anyway, just
> set the bit corresponding to the role's value in the user's roles
> ebitmap in set_user_role().
>
> Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> ---
> checkpolicy/policy_define.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
> index c9286f77..6c035716 100644
> --- a/checkpolicy/policy_define.c
> +++ b/checkpolicy/policy_define.c
> @@ -4088,8 +4088,6 @@ cond_expr_t *define_cond_expr(uint32_t expr_type, void *arg1, void *arg2)
> static int set_user_roles(role_set_t * set, char *id)
> {
> role_datum_t *r;
> - unsigned int i;
> - ebitmap_node_t *node;
>
> if (strcmp(id, "*") == 0) {
> free(id);
> @@ -4115,12 +4113,9 @@ static int set_user_roles(role_set_t * set, char *id)
> return -1;
> }
>
> - /* set the role and every role it dominates */
> - ebitmap_for_each_positive_bit(&r->dominates, node, i) {
> - if (ebitmap_set_bit(&set->roles, i, TRUE))
> - goto oom;
> - }
> free(id);
> + if (ebitmap_set_bit(&set->roles, r->s.value-1, TRUE))
> + goto oom;
> return 0;
> oom:
> yyerror("out of memory");
In other places there are spaces between the subtraction operator
("r->s.value - 1") and it would be nicer if this patch also used
spaces.
This is a minor comment anyway, and this patch looks good to me.
Acked-by: Nicolas Iooss <nicolas.iooss@xxxxxxx>
Thanks,
Nicolas
