| Message ID | 20231013135207.1462729-1-jwcart2@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 903e8cf26e2a |
| Delegated to: | Petr Lautrbach |
| Headers | show |
| Series | libsepol/cil: Do not allow classpermissionset to use anonymous classpermission | expand |
On Fri, Oct 13, 2023 at 9:52 AM James Carter <jwcart2@gmail.com> wrote: > > Macros can use classpermission arguments. These are used in two > different ways. Either a named classpermission is passed (which is > declared using a classpermisison rule) or an anonymous classpermission > is passed (something like "(CLASS (PERM))"). > > Usually this will look like either of the following: > Ex1/ > (classpermission cp1) > (classpermisisonset cp1 (CLASS (PERM))) > (macro m1 ((classpermisison ARG1)) > (allow t1 self ARG1) > ) > (call m1 (cp1)) > or > Ex2/ > (macro m2 ((classpermission ARG2)) > (allow t2 self ARG2) > ) > (call m2 ((CLASS (PERM)))) > > The following would also be valid: > Ex3/ > (classpermission cp3) > (macro m3 ((classpermission ARG3)) > (classpermissionset ARG3 (CLASS (PERM))) > (allow t3 self ARG3) > ) > (call m3 (cp3)) > > The oss-fuzzer did the equivalent of the following: > > (classpermission cp4) > (macro m4 ((classpermission ARG4)) > (classpermissionset ARG4 (CLASS (PERM1))) > (allow t4 self ARG4) > ) > (call m4 (CLASS (PERM2))) > > It passed an anonymous classpermission into a macro where there > was a classpermissionset rule. Suprisingly, everything worked well > until it was time to destroy the AST. There is no way to distinguish > between the anonymous classpermission being passed in which needs > to be destroyed and the classpermission in the classpermissionset > rule which is destroyed when the classpermissionset rule is > destroyed. This led to CIL trying to destroy the classpermission > in the classpermissionset rule twice. > > To fix this, when resolving the classpermission name in the > classpermissionset rule, check if the datum returned is for > an anonymous classpermission (it has no name) and return an > error if it is. > > This fixes oss-fuzz issue 60670. > > Signed-off-by: James Carter <jwcart2@gmail.com> This patch could use a review. I would like to get it in the upcoming release. Jim > --- > libsepol/cil/src/cil_resolve_ast.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c > index 33b9d321..49de8618 100644 > --- a/libsepol/cil/src/cil_resolve_ast.c > +++ b/libsepol/cil/src/cil_resolve_ast.c > @@ -254,6 +254,12 @@ int cil_resolve_classpermissionset(struct cil_tree_node *current, struct cil_cla > goto exit; > } > > + if (!datum->fqn) { > + cil_tree_log(current, CIL_ERR, "Anonymous classpermission used in a classpermissionset"); > + rc = SEPOL_ERR; > + goto exit; > + } > + > rc = cil_resolve_classperms_list(current, cps->classperms, extra_args); > if (rc != SEPOL_OK) { > goto exit; > -- > 2.41.0 >
On Fri, Nov 17, 2023 at 10:31 AM James Carter <jwcart2@gmail.com> wrote: > > On Fri, Oct 13, 2023 at 9:52 AM James Carter <jwcart2@gmail.com> wrote: > > > > Macros can use classpermission arguments. These are used in two > > different ways. Either a named classpermission is passed (which is > > declared using a classpermisison rule) or an anonymous classpermission > > is passed (something like "(CLASS (PERM))"). > > > > Usually this will look like either of the following: > > Ex1/ > > (classpermission cp1) > > (classpermisisonset cp1 (CLASS (PERM))) > > (macro m1 ((classpermisison ARG1)) > > (allow t1 self ARG1) > > ) > > (call m1 (cp1)) > > or > > Ex2/ > > (macro m2 ((classpermission ARG2)) > > (allow t2 self ARG2) > > ) > > (call m2 ((CLASS (PERM)))) > > > > The following would also be valid: > > Ex3/ > > (classpermission cp3) > > (macro m3 ((classpermission ARG3)) > > (classpermissionset ARG3 (CLASS (PERM))) > > (allow t3 self ARG3) > > ) > > (call m3 (cp3)) > > > > The oss-fuzzer did the equivalent of the following: > > > > (classpermission cp4) > > (macro m4 ((classpermission ARG4)) > > (classpermissionset ARG4 (CLASS (PERM1))) > > (allow t4 self ARG4) > > ) > > (call m4 (CLASS (PERM2))) > > > > It passed an anonymous classpermission into a macro where there > > was a classpermissionset rule. Suprisingly, everything worked well > > until it was time to destroy the AST. There is no way to distinguish > > between the anonymous classpermission being passed in which needs > > to be destroyed and the classpermission in the classpermissionset > > rule which is destroyed when the classpermissionset rule is > > destroyed. This led to CIL trying to destroy the classpermission > > in the classpermissionset rule twice. > > > > To fix this, when resolving the classpermission name in the > > classpermissionset rule, check if the datum returned is for > > an anonymous classpermission (it has no name) and return an > > error if it is. > > > > This fixes oss-fuzz issue 60670. > > > > Signed-off-by: James Carter <jwcart2@gmail.com> > > This patch could use a review. I would like to get it in the upcoming release. > Jim > This patch has been merged. Jim > > --- > > libsepol/cil/src/cil_resolve_ast.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c > > index 33b9d321..49de8618 100644 > > --- a/libsepol/cil/src/cil_resolve_ast.c > > +++ b/libsepol/cil/src/cil_resolve_ast.c > > @@ -254,6 +254,12 @@ int cil_resolve_classpermissionset(struct cil_tree_node *current, struct cil_cla > > goto exit; > > } > > > > + if (!datum->fqn) { > > + cil_tree_log(current, CIL_ERR, "Anonymous classpermission used in a classpermissionset"); > > + rc = SEPOL_ERR; > > + goto exit; > > + } > > + > > rc = cil_resolve_classperms_list(current, cps->classperms, extra_args); > > if (rc != SEPOL_OK) { > > goto exit; > > -- > > 2.41.0 > >
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c index 33b9d321..49de8618 100644 --- a/libsepol/cil/src/cil_resolve_ast.c +++ b/libsepol/cil/src/cil_resolve_ast.c @@ -254,6 +254,12 @@ int cil_resolve_classpermissionset(struct cil_tree_node *current, struct cil_cla goto exit; } + if (!datum->fqn) { + cil_tree_log(current, CIL_ERR, "Anonymous classpermission used in a classpermissionset"); + rc = SEPOL_ERR; + goto exit; + } + rc = cil_resolve_classperms_list(current, cps->classperms, extra_args); if (rc != SEPOL_OK) { goto exit;
Macros can use classpermission arguments. These are used in two different ways. Either a named classpermission is passed (which is declared using a classpermisison rule) or an anonymous classpermission is passed (something like "(CLASS (PERM))"). Usually this will look like either of the following: Ex1/ (classpermission cp1) (classpermisisonset cp1 (CLASS (PERM))) (macro m1 ((classpermisison ARG1)) (allow t1 self ARG1) ) (call m1 (cp1)) or Ex2/ (macro m2 ((classpermission ARG2)) (allow t2 self ARG2) ) (call m2 ((CLASS (PERM)))) The following would also be valid: Ex3/ (classpermission cp3) (macro m3 ((classpermission ARG3)) (classpermissionset ARG3 (CLASS (PERM))) (allow t3 self ARG3) ) (call m3 (cp3)) The oss-fuzzer did the equivalent of the following: (classpermission cp4) (macro m4 ((classpermission ARG4)) (classpermissionset ARG4 (CLASS (PERM1))) (allow t4 self ARG4) ) (call m4 (CLASS (PERM2))) It passed an anonymous classpermission into a macro where there was a classpermissionset rule. Suprisingly, everything worked well until it was time to destroy the AST. There is no way to distinguish between the anonymous classpermission being passed in which needs to be destroyed and the classpermission in the classpermissionset rule which is destroyed when the classpermissionset rule is destroyed. This led to CIL trying to destroy the classpermission in the classpermissionset rule twice. To fix this, when resolving the classpermission name in the classpermissionset rule, check if the datum returned is for an anonymous classpermission (it has no name) and return an error if it is. This fixes oss-fuzz issue 60670. Signed-off-by: James Carter <jwcart2@gmail.com> --- libsepol/cil/src/cil_resolve_ast.c | 6 ++++++ 1 file changed, 6 insertions(+)