On Tue, Nov 26, 2019 at 4:59 PM Richard Haines
<richard_c_haines@xxxxxxxxxxxxxx> wrote:
> On Tue, 2019-11-26 at 16:02 +0100, Ondrej Mosnacek wrote:
> > On Tue, Nov 26, 2019 at 3:54 PM Stephen Smalley <sds@xxxxxxxxxxxxx>
> > wrote:
> > > On 11/26/19 9:51 AM, Richard Haines wrote:
> > > > On Tue, 2019-11-26 at 13:06 +0100, Ondrej Mosnacek wrote:
> > > > > ...instead of open-coding the rules. Also define a fallback to
> > > > > allow
> > > > > the
> > > > > policy to build even if the interface is not defined.
> > > > >
> > > > > Fixes: f5e5a0b8d005 ("selinux-testsuite: Add key_socket tests")
> > > > > Cc: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx>
> > > > > Suggested-by: Stephen Smalley <sds@xxxxxxxxxxxxx>
> > > > > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > > > > ---
> > > > >
> > > > > Change in v3: use different approach as suggested by Stephen
> > > > > Change in v2: update also tests/Makefile for consistency
> > > > >
> > > > > policy/test_key_socket.te | 8 ++++----
> > > > > policy/test_policy.if | 6 ++++++
> > > > > 2 files changed, 10 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/policy/test_key_socket.te
> > > > > b/policy/test_key_socket.te
> > > > > index cde426b..f1c1606 100644
> > > > > --- a/policy/test_key_socket.te
> > > > > +++ b/policy/test_key_socket.te
> > > > > @@ -13,7 +13,7 @@ typeattribute test_key_sock_t keysockdomain;
> > > > > allow test_key_sock_t self:capability { net_admin };
> > > > > allow test_key_sock_t self:key_socket { create write read
> > > > > setopt };
> > > > > # For CONFIG_NET_KEY=m
> > > > > -allow test_key_sock_t kernel_t:system { module_request };
> > > > > +kernel_request_load_module(test_key_sock_t)
> > > > >
> > > > > ################## Deny capability { net_admin }
> > > > > ##########################
> > > > > #
> > > > > @@ -29,7 +29,7 @@ typeattribute test_key_sock_no_net_admin_t
> > > > > testdomain;
> > > > > typeattribute test_key_sock_no_net_admin_t keysockdomain;
> > > > >
> > > > > allow test_key_sock_no_net_admin_t self:key_socket { create
> > > > > write
> > > > > read setopt };
> > > > > -allow test_key_sock_no_net_admin_t kernel_t:system {
> > > > > module_request
> > > > > };
> > > > > +kernel_request_load_module(test_key_sock_t)
> > > >
> > > > All the new entries have (test_key_sock_t) ??
> > > > Anyway if you run the tests in the order they appear in 'test'
> > > > script,
> > > > then it just so happens that the module will be loaded for
> > > > test_key_sock_t as it's first. I added the others just to cover
> > > > the
> > > > cases where I sometimes run out of sequence, so you could remove
> > > > these
> > > > if required.
> >
> > *facepalm*
> >
> > Yes, sorry, I messed up the copy-paste...
> >
> > > Good catch! Somehow I missed that. Could probably just have a
> > > single
> > > kernel_request_load_module(keysockdomain) line?
> >
> > There is one domain (test_key_sock_no_create_t) that doesn't have the
> > rule, so probably not.
>
> That is probably one I didn't test out of sequence.
>
> I'm currently writing the TUN/TAP tests and had the same problem as
> I've left the default Fedora CONFIG_TUN=m.
> I've added 'kernel_request_load_module(tuntapdomain)' to the test
> module that allows tests in any order (as I usually run rmmod just as a
> check).
>
> I'll leave it to you guys to decide what is finally best for you.
Aha, I see, so actually all the types might potentially need the rule
(if someone rmmods the module during test, for example), so it would
make more sense to allow it to the whole domain as Stephen suggested.
I didn't stop to think about it and I assumed that the one domain
didn't have the rule intentionally... I think I'll repost the patch
once more, calling the macro on the whole keysockdomain.
>
> >
> > > > > ####################### Deny key_socket { create }
> > > > > ##########################
> > > > > type test_key_sock_no_create_t;
> > > > > @@ -50,7 +50,7 @@ typeattribute test_key_sock_no_write_t
> > > > > keysockdomain;
> > > > >
> > > > > allow test_key_sock_no_write_t self:capability { net_admin };
> > > > > allow test_key_sock_no_write_t self:key_socket { create read
> > > > > setopt
> > > > > };
> > > > > -allow test_key_sock_no_write_t kernel_t:system {
> > > > > module_request };
> > > > > +kernel_request_load_module(test_key_sock_t)
> > > > >
> > > > > ####################### Deny key_socket { read }
> > > > > ##########################
> > > > > type test_key_sock_no_read_t;
> > > > > @@ -61,7 +61,7 @@ typeattribute test_key_sock_no_read_t
> > > > > keysockdomain;
> > > > >
> > > > > allow test_key_sock_no_read_t self:capability { net_admin };
> > > > > allow test_key_sock_no_read_t self:key_socket { create write
> > > > > setopt
> > > > > };
> > > > > -allow test_key_sock_no_read_t kernel_t:system { module_request
> > > > > };
> > > > > +kernel_request_load_module(test_key_sock_t)
> > > > >
> > > > > #
> > > > > ########### Allow these domains to be entered from sysadm
> > > > > domain
> > > > > ############
> > > > > diff --git a/policy/test_policy.if b/policy/test_policy.if
> > > > > index e1175e8..3f163cb 100644
> > > > > --- a/policy/test_policy.if
> > > > > +++ b/policy/test_policy.if
> > > > > @@ -82,3 +82,9 @@ interface(`userdom_search_admin_dir', `
> > > > > userdom_search_user_home_content($1)
> > > > > ')
> > > > > ')
> > > > > +
> > > > > +# If the macro isn't defined, then most probably
> > > > > module_request
> > > > > permission
> > > > > +# is just not supported (and relevant operations should be
> > > > > just
> > > > > allowed).
> > > > > +ifdef(`kernel_request_load_module', `', ` dnl
> > > > > +interface(`kernel_request_load_module', `')
> > > > > +')
> >
> >
>
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
