| Message ID | 20210308204923.71940-1-jwcart2@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | libsepol/checkpolicy: Set user roles using role value instead of dominance | expand |
On Mon, Mar 8, 2021 at 9:50 PM James Carter <jwcart2@gmail.com> 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@gmail.com> > --- > 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@m4x.org> Thanks, Nicolas
On Sun, Mar 14, 2021 at 8:59 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > On Mon, Mar 8, 2021 at 9:50 PM James Carter <jwcart2@gmail.com> 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@gmail.com> > > --- > > 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@m4x.org> Merged, with added spaces. Thanks! Nicolas
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");
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@gmail.com> --- checkpolicy/policy_define.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)