* [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2
@ 2020-03-05 13:53 Ondrej Mosnacek
2020-03-09 13:17 ` James Carter
2020-05-11 12:27 ` Petr Lautrbach
0 siblings, 2 replies; 13+ messages in thread
From: Ondrej Mosnacek @ 2020-03-05 13:53 UTC (permalink / raw)
To: selinux; +Cc: Stephen Smalley, James Carter
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2
2020-03-05 13:53 [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2 Ondrej Mosnacek
@ 2020-03-09 13:17 ` James Carter
2020-03-11 18:29 ` James Carter
2020-05-11 12:27 ` Petr Lautrbach
1 sibling, 1 reply; 13+ messages in thread
From: James Carter @ 2020-03-09 13:17 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Stephen Smalley
On Thu, Mar 5, 2020 at 8:53 AM Ondrej Mosnacek <omosnace@redhat.com> 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>
Acked-by: James Carter <jwcart2@gmail.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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2
2020-03-09 13:17 ` James Carter
@ 2020-03-11 18:29 ` James Carter
0 siblings, 0 replies; 13+ messages in thread
From: James Carter @ 2020-03-11 18:29 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Stephen Smalley
On Mon, Mar 9, 2020 at 9:17 AM James Carter <jwcart2@gmail.com> wrote:
>
> On Thu, Mar 5, 2020 at 8:53 AM Ondrej Mosnacek <omosnace@redhat.com> 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>
>
> Acked-by: James Carter <jwcart2@gmail.com>
>
Thanks, applied.
Jim
> > ---
> >
> > 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
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2
2020-03-05 13:53 [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2 Ondrej Mosnacek
2020-03-09 13:17 ` James Carter
@ 2020-05-11 12:27 ` Petr Lautrbach
2020-05-11 13:25 ` James Carter
1 sibling, 1 reply; 13+ messages in thread
From: Petr Lautrbach @ 2020-05-11 12:27 UTC (permalink / raw)
To: selinux; +Cc: Ondrej Mosnacek, Stephen Smalley, James Carter
[-- Attachment #1: Type: text/plain, Size: 2460 bytes --]
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
Petr
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2
2020-05-11 12:27 ` Petr Lautrbach
@ 2020-05-11 13:25 ` James Carter
2020-05-11 18:03 ` James Carter
0 siblings, 1 reply; 13+ messages in thread
From: James Carter @ 2020-05-11 13:25 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: SElinux list, Ondrej Mosnacek, Stephen Smalley
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
>
> Petr
This suggests that there is a problem with how type attributes are
being expanded. This patch is just causing that problem to be exposed.
I'll have to investigate.
Thanks for the report.
Jim
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2
2020-05-11 13:25 ` James Carter
@ 2020-05-11 18:03 ` James Carter
2020-05-11 18:08 ` Stephen Smalley
0 siblings, 1 reply; 13+ messages in thread
From: James Carter @ 2020-05-11 18:03 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: SElinux list, Ondrej Mosnacek, Stephen Smalley
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.
Jim
> > Petr
>
> This suggests that there is a problem with how type attributes are
> being expanded. This patch is just causing that problem to be exposed.
> I'll have to investigate.
>
> Thanks for the report.
>
> Jim
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2
2020-05-11 18:03 ` James Carter
@ 2020-05-11 18:08 ` Stephen Smalley
2020-05-11 19:01 ` James Carter
0 siblings, 1 reply; 13+ messages in thread
From: Stephen Smalley @ 2020-05-11 18:08 UTC (permalink / raw)
To: James Carter; +Cc: Petr Lautrbach, SElinux list, Ondrej Mosnacek
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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2
2020-05-11 18:08 ` Stephen Smalley
@ 2020-05-11 19:01 ` James Carter
2020-05-12 17:16 ` Petr Lautrbach
0 siblings, 1 reply; 13+ messages in thread
From: James Carter @ 2020-05-11 19:01 UTC (permalink / raw)
To: Stephen Smalley; +Cc: Petr Lautrbach, SElinux list, Ondrej Mosnacek
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2
2020-05-11 19:01 ` James Carter
@ 2020-05-12 17:16 ` Petr Lautrbach
2020-05-12 17:27 ` Stephen Smalley
0 siblings, 1 reply; 13+ messages in thread
From: Petr Lautrbach @ 2020-05-12 17:16 UTC (permalink / raw)
To: SElinux list; +Cc: Stephen Smalley, Ondrej Mosnacek, James Carter
[-- Attachment #1: Type: text/plain, Size: 5971 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2
2020-05-12 17:16 ` Petr Lautrbach
@ 2020-05-12 17:27 ` Stephen Smalley
2020-05-12 19:33 ` James Carter
2020-05-12 19:57 ` James Carter
0 siblings, 2 replies; 13+ messages in thread
From: Stephen Smalley @ 2020-05-12 17:27 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: SElinux list, Ondrej Mosnacek, James Carter
On Tue, May 12, 2020 at 1:16 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> 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.
I think we should revert the change prior to release since it breaks
selinux userspace.
We'll need to fix the separate bug regarding pp to cil conversion that
is losing the expandattribute statement that you mentioned above.
And we should start annotating all attributes in policy on which
selinux userspace depends with
explicit expandattribute false statements to ensure that they are not
optimized away in the future.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2
2020-05-12 17:27 ` Stephen Smalley
@ 2020-05-12 19:33 ` James Carter
2020-05-12 19:48 ` James Carter
2020-05-12 19:57 ` James Carter
1 sibling, 1 reply; 13+ messages in thread
From: James Carter @ 2020-05-12 19:33 UTC (permalink / raw)
To: Stephen Smalley; +Cc: Petr Lautrbach, SElinux list, Ondrej Mosnacek
On Tue, May 12, 2020 at 1:27 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Tue, May 12, 2020 at 1:16 PM Petr Lautrbach <plautrba@redhat.com> wrote:
> >
> > 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.
>
> I think we should revert the change prior to release since it breaks
> selinux userspace.
> We'll need to fix the separate bug regarding pp to cil conversion that
> is losing the expandattribute statement that you mentioned above.
> And we should start annotating all attributes in policy on which
> selinux userspace depends with
> explicit expandattribute false statements to ensure that they are not
> optimized away in the future.
Looking at libsepol, the issue is that the expand attribute
information is not saved in any of the binary formats. Converting a
policy.conf to a CIL file preserves the expand attribute statement.
So we need to revert the change, update binary formats, annotate
attributes that always need to be kept, and then we can reapply the
change.
Jim
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2
2020-05-12 19:33 ` James Carter
@ 2020-05-12 19:48 ` James Carter
0 siblings, 0 replies; 13+ messages in thread
From: James Carter @ 2020-05-12 19:48 UTC (permalink / raw)
To: Stephen Smalley; +Cc: Petr Lautrbach, SElinux list, Ondrej Mosnacek
On Tue, May 12, 2020 at 3:33 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Tue, May 12, 2020 at 1:27 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Tue, May 12, 2020 at 1:16 PM Petr Lautrbach <plautrba@redhat.com> wrote:
> > >
> > > 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.
> >
> > I think we should revert the change prior to release since it breaks
> > selinux userspace.
> > We'll need to fix the separate bug regarding pp to cil conversion that
> > is losing the expandattribute statement that you mentioned above.
> > And we should start annotating all attributes in policy on which
> > selinux userspace depends with
> > explicit expandattribute false statements to ensure that they are not
> > optimized away in the future.
>
> Looking at libsepol, the issue is that the expand attribute
> information is not saved in any of the binary formats. Converting a
> policy.conf to a CIL file preserves the expand attribute statement.
>
> So we need to revert the change, update binary formats, annotate
> attributes that always need to be kept, and then we can reapply the
> change.
>
I guess we can just add a CIL module with the needed
expandtypeattribute statements. That would be a whole lot easier.
Jim
> Jim
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2
2020-05-12 17:27 ` Stephen Smalley
2020-05-12 19:33 ` James Carter
@ 2020-05-12 19:57 ` James Carter
1 sibling, 0 replies; 13+ messages in thread
From: James Carter @ 2020-05-12 19:57 UTC (permalink / raw)
To: Stephen Smalley; +Cc: Petr Lautrbach, SElinux list, Ondrej Mosnacek
On Tue, May 12, 2020 at 1:27 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> I think we should revert the change prior to release since it breaks
> selinux userspace.
This has been reverted.
Jim
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-05-12 19:58 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-05 13:53 [RFC PATCH v2] libsepol/cil: raise default attrs_expand_size to 2 Ondrej Mosnacek
2020-03-09 13:17 ` James Carter
2020-03-11 18:29 ` James Carter
2020-05-11 12:27 ` Petr Lautrbach
2020-05-11 13:25 ` James Carter
2020-05-11 18:03 ` James Carter
2020-05-11 18:08 ` Stephen Smalley
2020-05-11 19:01 ` James Carter
2020-05-12 17:16 ` Petr Lautrbach
2020-05-12 17:27 ` Stephen Smalley
2020-05-12 19:33 ` James Carter
2020-05-12 19:48 ` James Carter
2020-05-12 19:57 ` James Carter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).