Re: [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2

[James Carter <jwcart2@gmail.com>]

On Mon, May 11, 2020 at 2:09 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Mon, May 11, 2020 at 2:03 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > On Mon, May 11, 2020 at 9:25 AM James Carter <jwcart2@gmail.com> wrote:
> > >
> > > On Mon, May 11, 2020 at 8:27 AM Petr Lautrbach <plautrba@redhat.com> wrote:
> > > >
> > > > On Thu, Mar 05, 2020 at 02:53:37PM +0100, Ondrej Mosnacek wrote:
> > > > > The value attrs_expand_size == 1 removes all empty attributes, but it
> > > > > also makes sense to expand all attributes that have only one type. This
> > > > > removes some redundant rules (there is sometimes the same rule for the
> > > > > type and the attribute) and reduces the number of attributes that the
> > > > > kernel has to go through when looking up rules.
> > > > >
> > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > > > ---
> > > > >
> > > > > v2: fix typos (Tne -> The; cointains -> contains)
> > > > >
> > > > >  libsepol/cil/src/cil.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
> > > > > index d222ad3a..c010ca2a 100644
> > > > > --- a/libsepol/cil/src/cil.c
> > > > > +++ b/libsepol/cil/src/cil.c
> > > > > @@ -452,7 +452,8 @@ void cil_db_init(struct cil_db **db)
> > > > >       (*db)->disable_dontaudit = CIL_FALSE;
> > > > >       (*db)->disable_neverallow = CIL_FALSE;
> > > > >       (*db)->attrs_expand_generated = CIL_FALSE;
> > > > > -     (*db)->attrs_expand_size = 1;
> > > > > +     /* 2 == remove attributes that contain none or just 1 type */
> > > > > +     (*db)->attrs_expand_size = 2;
> > > > >       (*db)->preserve_tunables = CIL_FALSE;
> > > > >       (*db)->handle_unknown = -1;
> > > > >       (*db)->mls = -1;
> > > > > --
> > > > > 2.24.1
> > > > >
> > > >
> > > >
> > > > This patch broke `semanage node -l` on Fedora [1]
> > > >
> > > > :: [ 21:25:25 ] :: [  BEGIN   ] :: Running 'make LIBDIR=/usr/lib64 SHLIBDIR=/lib64 test'
> > > > ...
> > > > test_list (__main__.SemanageTests) ... Traceback (most recent call last):
> > > >   File "/usr/sbin/semanage", line 967, in <module>
> > > >     do_parser()
> > > >   File "/usr/sbin/semanage", line 946, in do_parser
> > > >     args.func(args)
> > > >   File "/usr/sbin/semanage", line 649, in handleNode
> > > >     OBJECT = object_dict['node'](args)
> > > >   File "/usr/lib/python3.8/site-packages/seobject.py", line 1849, in __init__
> > > >     self.valid_types = list(list(sepolicy.info(sepolicy.ATTRIBUTE, "node_type"))[0]["types"])
> > > > IndexError: list index out of range
> > > >
> > > > While the `IndexError: list index out of range` error can be simply fixed, it
> > > > uncovered the problem that semanage uses attibutes to list certain records -
> > > > node_type, port_type, file_type, device_node, ... and these attributes can disappear when
> > > > there's only 1 type assigned.
> > > >
> > > > I guess it should be reverted as there's no other way how to find out that a
> > > > type node_t is node_type.
> > > >
> > > > [1] https://jenkins-continuous-infra.apps.ci.centos.org/job/fedora-rawhide-pr-pipeline/3462/artifact/package-tests/logs/FAIL-upstream-err.log
> > > >
> >
> > I see now.  python/semanage/seobject.py and
> > python/semanage/semanage-bash-completion.sh both assume that node_type
> > is always defined as an attribute in a policy. There seems to be quite
> > a lot that is assumed about policy in the python directory.
> >
> > This is not a bug in CIL or libsepol. Ideally the tests should have
> > their own policy and the python code should gracefully handle the
> > situation when its assumptions are wrong. If we need to revert this
> > patch in the short-term than I am ok with that.
>
> We can mark these attributes explicitly in policy to prevent their
> expansion, right?  So while we cannot make this change right now
> without breaking compatibility with selinux userspace tools (not just
> tests), we could start marking these attributes on which the tools
> depend in policy and then later we can re-apply this?

Yes, we can add the rule "expandattribute node_type false;" in policy
for node_type and any other attributes that are required to exist.

Jim