| Message ID | 20200513211508.4477-1-stephen.smalley.work@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | libsepol: drop broken warning on duplicate filename transitions | expand |
On Wed, May 13, 2020 at 11:16 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > As per the issue below, libsepol segfaults on loading old kernel policies > that contain duplicate filename transition rules. The segfault is due to > the fact that the val_to_name arrays have not yet been populated at this > point in the policydb_read() processing. Since this warning apparently > never worked since it was first introduced, drop it and just silently > discard the duplicate like the kernel does. I was not able to produce a > policy with such duplicates using the current policy toolchain, either > via CIL or via binary modules with manual semodule_link/expand. > > Fixes: https://github.com/SELinuxProject/selinux/issues/239 > Fixes: 8fdb2255215a1f14 ("libsepol,checkpolicy: convert rangetrans and filenametrans to hashtabs") > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > --- > libsepol/src/policydb.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > index 5b289a52..3992ea56 100644 > --- a/libsepol/src/policydb.c > +++ b/libsepol/src/policydb.c > @@ -2655,15 +2655,8 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp) > * Some old policies were wrongly generated with > * duplicate filename transition rules. For backward > * compatibility, do not reject such policies, just > - * issue a warning and ignore the duplicate. > + * ignore the duplicate. > */ > - WARN(fp->handle, > - "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring", > - p->p_type_val_to_name[ft->stype - 1], > - p->p_type_val_to_name[ft->ttype - 1], > - p->p_class_val_to_name[ft->tclass - 1], > - ft->name, > - p->p_type_val_to_name[otype->otype - 1]); Not sure if it's the same situation, but should we also do something about a similar pattern in checkpolicy/policy_define.c? https://github.com/SELinuxProject/selinux/blob/63bf6afe5ed20e1d62f966de65882dc327fb2915/checkpolicy/policy_define.c#L3408 > free(ft); > free(name); > free(otype); > -- > 2.23.3 >
On Thu, May 14, 2020 at 4:19 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Wed, May 13, 2020 at 11:16 PM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > As per the issue below, libsepol segfaults on loading old kernel policies > > that contain duplicate filename transition rules. The segfault is due to > > the fact that the val_to_name arrays have not yet been populated at this > > point in the policydb_read() processing. Since this warning apparently > > never worked since it was first introduced, drop it and just silently > > discard the duplicate like the kernel does. I was not able to produce a > > policy with such duplicates using the current policy toolchain, either > > via CIL or via binary modules with manual semodule_link/expand. > > > > Fixes: https://github.com/SELinuxProject/selinux/issues/239 > > Fixes: 8fdb2255215a1f14 ("libsepol,checkpolicy: convert rangetrans and filenametrans to hashtabs") > > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > --- > > libsepol/src/policydb.c | 9 +-------- > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > > index 5b289a52..3992ea56 100644 > > --- a/libsepol/src/policydb.c > > +++ b/libsepol/src/policydb.c > > @@ -2655,15 +2655,8 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp) > > * Some old policies were wrongly generated with > > * duplicate filename transition rules. For backward > > * compatibility, do not reject such policies, just > > - * issue a warning and ignore the duplicate. > > + * ignore the duplicate. > > */ > > - WARN(fp->handle, > > - "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring", > > - p->p_type_val_to_name[ft->stype - 1], > > - p->p_type_val_to_name[ft->ttype - 1], > > - p->p_class_val_to_name[ft->tclass - 1], > > - ft->name, > > - p->p_type_val_to_name[otype->otype - 1]); > > Not sure if it's the same situation, but should we also do something > about a similar pattern in checkpolicy/policy_define.c? > > https://github.com/SELinuxProject/selinux/blob/63bf6afe5ed20e1d62f966de65882dc327fb2915/checkpolicy/policy_define.c#L3408 No, in that case we are compiling source policy and we want to warn on it to encourage removal of duplicates (and we have populated the val_to_name arrays there so the warning works).
On Thu, May 14, 2020 at 2:03 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Thu, May 14, 2020 at 4:19 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > On Wed, May 13, 2020 at 11:16 PM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > > > As per the issue below, libsepol segfaults on loading old kernel policies > > > that contain duplicate filename transition rules. The segfault is due to > > > the fact that the val_to_name arrays have not yet been populated at this > > > point in the policydb_read() processing. Since this warning apparently > > > never worked since it was first introduced, drop it and just silently > > > discard the duplicate like the kernel does. I was not able to produce a > > > policy with such duplicates using the current policy toolchain, either > > > via CIL or via binary modules with manual semodule_link/expand. > > > > > > Fixes: https://github.com/SELinuxProject/selinux/issues/239 > > > Fixes: 8fdb2255215a1f14 ("libsepol,checkpolicy: convert rangetrans and filenametrans to hashtabs") > > > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > > --- > > > libsepol/src/policydb.c | 9 +-------- > > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > > > index 5b289a52..3992ea56 100644 > > > --- a/libsepol/src/policydb.c > > > +++ b/libsepol/src/policydb.c > > > @@ -2655,15 +2655,8 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp) > > > * Some old policies were wrongly generated with > > > * duplicate filename transition rules. For backward > > > * compatibility, do not reject such policies, just > > > - * issue a warning and ignore the duplicate. > > > + * ignore the duplicate. > > > */ > > > - WARN(fp->handle, > > > - "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring", > > > - p->p_type_val_to_name[ft->stype - 1], > > > - p->p_type_val_to_name[ft->ttype - 1], > > > - p->p_class_val_to_name[ft->tclass - 1], > > > - ft->name, > > > - p->p_type_val_to_name[otype->otype - 1]); > > > > Not sure if it's the same situation, but should we also do something > > about a similar pattern in checkpolicy/policy_define.c? > > > > https://github.com/SELinuxProject/selinux/blob/63bf6afe5ed20e1d62f966de65882dc327fb2915/checkpolicy/policy_define.c#L3408 > > No, in that case we are compiling source policy and we want to warn on > it to encourage removal of duplicates (and we have populated the > val_to_name arrays there so the warning works). Ok, makes sense. In that case: Acked-by: Ondrej Mosnacek <omosnace@redhat.com>
On Thu, May 14, 2020 at 02:19:05PM +0200, Ondrej Mosnacek wrote: > On Thu, May 14, 2020 at 2:03 PM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > On Thu, May 14, 2020 at 4:19 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > > On Wed, May 13, 2020 at 11:16 PM Stephen Smalley > > > <stephen.smalley.work@gmail.com> wrote: > > > > As per the issue below, libsepol segfaults on loading old kernel policies > > > > that contain duplicate filename transition rules. The segfault is due to > > > > the fact that the val_to_name arrays have not yet been populated at this > > > > point in the policydb_read() processing. Since this warning apparently > > > > never worked since it was first introduced, drop it and just silently > > > > discard the duplicate like the kernel does. I was not able to produce a > > > > policy with such duplicates using the current policy toolchain, either > > > > via CIL or via binary modules with manual semodule_link/expand. > > > > > > > > Fixes: https://github.com/SELinuxProject/selinux/issues/239 > > > > Fixes: 8fdb2255215a1f14 ("libsepol,checkpolicy: convert rangetrans and filenametrans to hashtabs") > > > > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > > > --- > > > > libsepol/src/policydb.c | 9 +-------- > > > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > > > > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > > > > index 5b289a52..3992ea56 100644 > > > > --- a/libsepol/src/policydb.c > > > > +++ b/libsepol/src/policydb.c > > > > @@ -2655,15 +2655,8 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp) > > > > * Some old policies were wrongly generated with > > > > * duplicate filename transition rules. For backward > > > > * compatibility, do not reject such policies, just > > > > - * issue a warning and ignore the duplicate. > > > > + * ignore the duplicate. > > > > */ > > > > - WARN(fp->handle, > > > > - "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring", > > > > - p->p_type_val_to_name[ft->stype - 1], > > > > - p->p_type_val_to_name[ft->ttype - 1], > > > > - p->p_class_val_to_name[ft->tclass - 1], > > > > - ft->name, > > > > - p->p_type_val_to_name[otype->otype - 1]); > > > > > > Not sure if it's the same situation, but should we also do something > > > about a similar pattern in checkpolicy/policy_define.c? > > > > > > https://github.com/SELinuxProject/selinux/blob/63bf6afe5ed20e1d62f966de65882dc327fb2915/checkpolicy/policy_define.c#L3408 > > > > No, in that case we are compiling source policy and we want to warn on > > it to encourage removal of duplicates (and we have populated the > > val_to_name arrays there so the warning works). > > Ok, makes sense. In that case: > > Acked-by: Ondrej Mosnacek <omosnace@redhat.com> > Applied.
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c index 5b289a52..3992ea56 100644 --- a/libsepol/src/policydb.c +++ b/libsepol/src/policydb.c @@ -2655,15 +2655,8 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp) * Some old policies were wrongly generated with * duplicate filename transition rules. For backward * compatibility, do not reject such policies, just - * issue a warning and ignore the duplicate. + * ignore the duplicate. */ - WARN(fp->handle, - "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring", - p->p_type_val_to_name[ft->stype - 1], - p->p_type_val_to_name[ft->ttype - 1], - p->p_class_val_to_name[ft->tclass - 1], - ft->name, - p->p_type_val_to_name[otype->otype - 1]); free(ft); free(name); free(otype);
As per the issue below, libsepol segfaults on loading old kernel policies that contain duplicate filename transition rules. The segfault is due to the fact that the val_to_name arrays have not yet been populated at this point in the policydb_read() processing. Since this warning apparently never worked since it was first introduced, drop it and just silently discard the duplicate like the kernel does. I was not able to produce a policy with such duplicates using the current policy toolchain, either via CIL or via binary modules with manual semodule_link/expand. Fixes: https://github.com/SELinuxProject/selinux/issues/239 Fixes: 8fdb2255215a1f14 ("libsepol,checkpolicy: convert rangetrans and filenametrans to hashtabs") Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> --- libsepol/src/policydb.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)