On Mon, May 11, 2020 at 03:01:06PM -0400, James Carter wrote:
> 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.
> 


It doesn't seem to be propagated to cil:

# cat mypolicy.te 
policy_module(mypolicy,1.0)

type myapp_t;
type myapp_log_t;

attribute myattribute;
expandattribute myattribute false;
typeattribute myapp_t myattribute;

allow myattribute myapp_log_t:file read;

# make -f /usr/share/selinux/devel/Makefile mypolicy.pp
make: 'mypolicy.pp' is up to date.

# /usr/libexec/selinux/hll/pp mypolicy.pp
(type myapp_t)
(roletype object_r myapp_t)
(type myapp_log_t)
(roletype object_r myapp_log_t)
(typeattribute myattribute)
(typeattributeset myattribute (myapp_t ))
(roleattributeset cil_gen_require system_r)
(allow myattribute myapp_log_t (file (read)))


But it works with mypolicy.cil which contains:
(expandtypeattribute myattribute false)


I'm not really experienced in writing policy but I still find the behaviour
confusing.
It's not only about `semanage`, you can hit this using `sesearch` as well, e.g.
before policy is rebuilt with new libsepol, `sesearch -A -t node_type` finds
about 535 rules with node_type, then you rebuild policy and it's 0. But if
assign node_type to another type, it's again more than 536.

For this particular attribute we can/should use `expandtypeattribute` but should
be this expression used every time you have an attribute assigned to only 1 type?


If it stays as it is, it definitely needs to be part of release notes.