Re: [ovs-dev] [PATCH 2/2] rhel: Allow openvswitch to get parent information

[Joe Stringer]

On 28 July 2016 at 06:43, Flavio Leitner  wrote:
>
> Adding William since he is the author of commit 484371776e
>
> On Wed, Jul 27, 2016 at 01:54:45PM -0700, Joe Stringer wrote:
>> On 26 July 2016 at 14:10, Flavio Leitner  wrote:
>> > On Tue, Jul 26, 2016 at 12:57:07PM -0700, Joe Stringer wrote:
>> >> As far as I can follow, this "domain" type is not just for accessing
>> >> OVS directories and files (like openvswitch_t), but ifor a much wider
>> >> range of paths:
>> >> https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/4/html/SELinux_Guide/rhlcommon-section-0048.html
>> >>
>> >> "# The domain attribute identifies every type that can be
>> >> # assigned to a process.  This attribute is used in TE rules
>> >> # that should be applied to all domains, e.g. permitting
>> >> # init to kill all processes."
>> >>
>> >> Is my understanding (+documentation) correct here? Is there an similar
>> >
>> > Your understanding is correct.  Turns out that we don't know which
>> > process will be the parent, so it could bash unconfined or initrc_t
>> > or in any other context (neutron?).
>> >
>> >> but more restrictive policy that allows ovs-vsctl to access, for
>> >> example, /var/run/openvswitch/* (with var_run_openvswitch_t or
>> >> similar)? Alternatively is there an example of another daemon that has
>> >> a similar policy that set a precedence for writing the policy like
>> >> this?
>> >
>> > I spent few hours on this and I couldn't find a way to restrict it
>> > more that I proposed with selinux.  Basically the above is an expansion
>> > of the interface domain_read_all_domains_state()[1] which other
>> > applications are using to read other processes states.  However, that
>> > seemed relatively new and probably not available on older distros, so
>> > I have expanded to the relevant actions removing what we don't need.
>> >
>> > [1] http://danwalsh.livejournal.com/51435.html
>> >
>> >> Would you also be able to provide the full ovs-vsctl commandline? It
>> >> was a little difficult to understand exactly what was going on during
>> >> this event, or try to reproduce.
>> >
>> > utilities/ovs-vsctl.c:2473
>> >
>> > 2472 static char *
>> > 2473 vsctl_parent_process_info(void)
>> > 2474 {
>> > 2475 #ifdef __linux__
>> > 2476 pid_t parent_pid;
>> > 2477 char *procfile;
>> > 2478 struct ds s;
>> > 2479 FILE *f;
>> > 2480
>> > 2481 parent_pid = getppid();
>> > 2482 procfile = xasprintf("/proc/%d/cmdline", parent_pid);
>> > 2483
>> > 2484 f = fopen(procfile, "r");
>> >
>> > That is called from do_vsctl() to find the parent info.  If you run as
>> > root, then it's unconfined and it works, but it doesn't work during
>> > boot time (initrc_t) for instance.
>> >
>> > To reproduce you just need to configure an OVS interface using ifcfg
>> > with ONBOOT=yes and reboot.
>>
>> Hmm. So all we want to do in this case is to get the parent's name to
>> log it. I'm of two minds:
>
> Yes, funny how that can be a pain :-)
>
>> 1) Be more restrictive. If this were the only place in OVS that we
>> read /proc then maybe it should just print the pid on failure to read
>> /proc, then we just accept that SELinux denies this access. It seems
>> like it's just a niceity for prettyprinting which shouldn't affect
>> actual setup/operation of OVS. While a pid is less than a name, it's
>> still just as much information for cases like this where it's being
>> invoked from initrc_t.
>
> It generates AVC messages which triggers notifications, so we actually
> need to consider three options:
> 1) Allow it (your option 2 below)
> 2) Fix the code  (proposed in your option 1)
> 3) Use dontaudit to deny but not log.
>
> The problem is that PIDs numbers don't mean anything useful for us when
> you are looking at logs from another system.  Even if you are on the
> system, PIDs are ephemeral and probably gone or worse, recycled.

I was primarily thinking of something like it being called from
initrc_t and it printed "pid 1" then that would actually be
informative. But I guess this doesn't address your "various
scripts/programs are changing configuration" use case.

> I think the feature has an interesting value when you consider a OSP
> compute node where agents and scripts are running and configuring OVS.
> I got reports like "we added an port but there is no traffic", the
> next step is to make sense out of ovsdb logs and there you find
> an application adding and deleting the same port.  Without that, we
> have the actions, but we can't tell who did it.  How to find that out
> then?
>
> If we can reproduce the issue and start to record pids and names with
> another script, then it's fine, but most probably it's one of those
> "happened once", "it's in production", "can't enable this or that"
> kind of situation.

I spoke briefly with Ben about this offline, and the way I understand
it is that there were two pieces of debugging information provided
around the time this 

Re: [ovs-dev] [PATCH 2/2] rhel: Allow openvswitch to get parent information

[Flavio Leitner]

Adding William since he is the author of commit 484371776e

On Wed, Jul 27, 2016 at 01:54:45PM -0700, Joe Stringer wrote:
> On 26 July 2016 at 14:10, Flavio Leitner  wrote:
> > On Tue, Jul 26, 2016 at 12:57:07PM -0700, Joe Stringer wrote:
> >> As far as I can follow, this "domain" type is not just for accessing
> >> OVS directories and files (like openvswitch_t), but ifor a much wider
> >> range of paths:
> >> https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/4/html/SELinux_Guide/rhlcommon-section-0048.html
> >>
> >> "# The domain attribute identifies every type that can be
> >> # assigned to a process.  This attribute is used in TE rules
> >> # that should be applied to all domains, e.g. permitting
> >> # init to kill all processes."
> >>
> >> Is my understanding (+documentation) correct here? Is there an similar
> >
> > Your understanding is correct.  Turns out that we don't know which
> > process will be the parent, so it could bash unconfined or initrc_t
> > or in any other context (neutron?).
> >
> >> but more restrictive policy that allows ovs-vsctl to access, for
> >> example, /var/run/openvswitch/* (with var_run_openvswitch_t or
> >> similar)? Alternatively is there an example of another daemon that has
> >> a similar policy that set a precedence for writing the policy like
> >> this?
> >
> > I spent few hours on this and I couldn't find a way to restrict it
> > more that I proposed with selinux.  Basically the above is an expansion
> > of the interface domain_read_all_domains_state()[1] which other
> > applications are using to read other processes states.  However, that
> > seemed relatively new and probably not available on older distros, so
> > I have expanded to the relevant actions removing what we don't need.
> >
> > [1] http://danwalsh.livejournal.com/51435.html
> >
> >> Would you also be able to provide the full ovs-vsctl commandline? It
> >> was a little difficult to understand exactly what was going on during
> >> this event, or try to reproduce.
> >
> > utilities/ovs-vsctl.c:2473
> >
> > 2472 static char *
> > 2473 vsctl_parent_process_info(void)
> > 2474 {
> > 2475 #ifdef __linux__
> > 2476 pid_t parent_pid;
> > 2477 char *procfile;
> > 2478 struct ds s;
> > 2479 FILE *f;
> > 2480
> > 2481 parent_pid = getppid();
> > 2482 procfile = xasprintf("/proc/%d/cmdline", parent_pid);
> > 2483
> > 2484 f = fopen(procfile, "r");
> >
> > That is called from do_vsctl() to find the parent info.  If you run as
> > root, then it's unconfined and it works, but it doesn't work during
> > boot time (initrc_t) for instance.
> >
> > To reproduce you just need to configure an OVS interface using ifcfg
> > with ONBOOT=yes and reboot.
> 
> Hmm. So all we want to do in this case is to get the parent's name to
> log it. I'm of two minds:

Yes, funny how that can be a pain :-)

> 1) Be more restrictive. If this were the only place in OVS that we
> read /proc then maybe it should just print the pid on failure to read
> /proc, then we just accept that SELinux denies this access. It seems
> like it's just a niceity for prettyprinting which shouldn't affect
> actual setup/operation of OVS. While a pid is less than a name, it's
> still just as much information for cases like this where it's being
> invoked from initrc_t.

It generates AVC messages which triggers notifications, so we actually
need to consider three options:
1) Allow it (your option 2 below)
2) Fix the code  (proposed in your option 1)
3) Use dontaudit to deny but not log.

The problem is that PIDs numbers don't mean anything useful for us when
you are looking at logs from another system.  Even if you are on the
system, PIDs are ephemeral and probably gone or worse, recycled.

I think the feature has an interesting value when you consider a OSP
compute node where agents and scripts are running and configuring OVS.
I got reports like "we added an port but there is no traffic", the
next step is to make sense out of ovsdb logs and there you find
an application adding and deleting the same port.  Without that, we
have the actions, but we can't tell who did it.  How to find that out
then?

If we can reproduce the issue and start to record pids and names with
another script, then it's fine, but most probably it's one of those
"happened once", "it's in production", "can't enable this or that"
kind of situation.

> 2) Be more lenient. It seems there are a few other /proc reads,
> particularly in netdev-linux.c, which I'm guessing will just need the
> same policy that you're proposing above, so we can just accept this
> policy. If someone's concerned about tightening this policy, they can
> choose not to install this package.

IIRC, the other reads on /proc are system_u:object_r:proc_net_t:s0 so
it can be more restricted by using that and not 'domain'.

> I suppose that if SELinux isn't granular enough at this point then we
> could go with #2 and tighten the policy later when there are better

Re: [ovs-dev] [PATCH 2/2] rhel: Allow openvswitch to get parent information

[Joe Stringer]

On 26 July 2016 at 14:10, Flavio Leitner  wrote:
> On Tue, Jul 26, 2016 at 12:57:07PM -0700, Joe Stringer wrote:
>> On 25 July 2016 at 18:16, Flavio Leitner  wrote:
>> > Updates SELinux to allow ovs-vsctl to get parent process
>> > information and log that to the database:
>> >
>> > record 241: 2016-07-26 00:59:47.418 "ovs-vsctl (invoked by /bin/bash
>> > (pid 1589)): ovs-vsctl -t 10 -- --if-exist ...
>> >
>> > Jul 25 12:57:35 localhost.localdomain audit[830]: AVC avc:  denied  {
>> > search } for  pid=830 comm="ovs-vsctl" name="731" dev="proc" ino=14140
>> > scontext=system_u:system_r:openvswitch_t:s0
>> > tcontext=system_u:system_r:initrc_t:s0 tclass=dir permissive=0
>> >
>> > Signed-off-by: Flavio Leitner 
>> > ---
>> >  selinux/openvswitch-custom.te | 5 +
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/selinux/openvswitch-custom.te b/selinux/openvswitch-custom.te
>> > index fc32b97..5739595 100644
>> > --- a/selinux/openvswitch-custom.te
>> > +++ b/selinux/openvswitch-custom.te
>> > @@ -2,8 +2,13 @@ module openvswitch-custom 1.0;
>> >
>> >  require {
>> >  type openvswitch_t;
>> > +attribute domain;
>> >  class netlink_socket { setopt getopt create connect getattr write 
>> > read };
>> > +class dir { search };
>> > +class file { open getattr read };
>> >  }
>> >
>> >  #= openvswitch_t ==
>> >  allow openvswitch_t self:netlink_socket { setopt getopt create connect 
>> > getattr write read };
>> > +allow openvswitch_t domain:dir { search };
>> > +allow openvswitch_t domain:file { open getattr read };
>>
>> Hi Flavio,
>>
>> Thanks for spending some time to get OVS in better shape with SELinux.
>> I figure that once this settles down a bit we should take the policy
>> file here and work towards upstreaming all of the policy changes.
>
> Yeah, we can try to do both in parallel.  Once this gets in, I will
> open the bz requesting to fix Fedora which would fix upstream too.

Sounds good.

>> As far as I can follow, this "domain" type is not just for accessing
>> OVS directories and files (like openvswitch_t), but ifor a much wider
>> range of paths:
>> https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/4/html/SELinux_Guide/rhlcommon-section-0048.html
>>
>> "# The domain attribute identifies every type that can be
>> # assigned to a process.  This attribute is used in TE rules
>> # that should be applied to all domains, e.g. permitting
>> # init to kill all processes."
>>
>> Is my understanding (+documentation) correct here? Is there an similar
>
> Your understanding is correct.  Turns out that we don't know which
> process will be the parent, so it could bash unconfined or initrc_t
> or in any other context (neutron?).
>
>> but more restrictive policy that allows ovs-vsctl to access, for
>> example, /var/run/openvswitch/* (with var_run_openvswitch_t or
>> similar)? Alternatively is there an example of another daemon that has
>> a similar policy that set a precedence for writing the policy like
>> this?
>
> I spent few hours on this and I couldn't find a way to restrict it
> more that I proposed with selinux.  Basically the above is an expansion
> of the interface domain_read_all_domains_state()[1] which other
> applications are using to read other processes states.  However, that
> seemed relatively new and probably not available on older distros, so
> I have expanded to the relevant actions removing what we don't need.
>
> [1] http://danwalsh.livejournal.com/51435.html
>
>> Would you also be able to provide the full ovs-vsctl commandline? It
>> was a little difficult to understand exactly what was going on during
>> this event, or try to reproduce.
>
> utilities/ovs-vsctl.c:2473
>
> 2472 static char *
> 2473 vsctl_parent_process_info(void)
> 2474 {
> 2475 #ifdef __linux__
> 2476 pid_t parent_pid;
> 2477 char *procfile;
> 2478 struct ds s;
> 2479 FILE *f;
> 2480
> 2481 parent_pid = getppid();
> 2482 procfile = xasprintf("/proc/%d/cmdline", parent_pid);
> 2483
> 2484 f = fopen(procfile, "r");
>
> That is called from do_vsctl() to find the parent info.  If you run as
> root, then it's unconfined and it works, but it doesn't work during
> boot time (initrc_t) for instance.
>
> To reproduce you just need to configure an OVS interface using ifcfg
> with ONBOOT=yes and reboot.

Hmm. So all we want to do in this case is to get the parent's name to
log it. I'm of two minds:

1) Be more restrictive. If this were the only place in OVS that we
read /proc then maybe it should just print the pid on failure to read
/proc, then we just accept that SELinux denies this access. It seems
like it's just a niceity for prettyprinting which shouldn't affect
actual setup/operation of OVS. While a pid is less than a name, it's
still just as much information for cases like this where it's being
invoked from initrc_t.

2) Be more lenient. It seems there 

Re: [ovs-dev] [PATCH 2/2] rhel: Allow openvswitch to get parent information

[Flavio Leitner]

On Tue, Jul 26, 2016 at 12:57:07PM -0700, Joe Stringer wrote:
> On 25 July 2016 at 18:16, Flavio Leitner  wrote:
> > Updates SELinux to allow ovs-vsctl to get parent process
> > information and log that to the database:
> >
> > record 241: 2016-07-26 00:59:47.418 "ovs-vsctl (invoked by /bin/bash
> > (pid 1589)): ovs-vsctl -t 10 -- --if-exist ...
> >
> > Jul 25 12:57:35 localhost.localdomain audit[830]: AVC avc:  denied  {
> > search } for  pid=830 comm="ovs-vsctl" name="731" dev="proc" ino=14140
> > scontext=system_u:system_r:openvswitch_t:s0
> > tcontext=system_u:system_r:initrc_t:s0 tclass=dir permissive=0
> >
> > Signed-off-by: Flavio Leitner 
> > ---
> >  selinux/openvswitch-custom.te | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/selinux/openvswitch-custom.te b/selinux/openvswitch-custom.te
> > index fc32b97..5739595 100644
> > --- a/selinux/openvswitch-custom.te
> > +++ b/selinux/openvswitch-custom.te
> > @@ -2,8 +2,13 @@ module openvswitch-custom 1.0;
> >
> >  require {
> >  type openvswitch_t;
> > +attribute domain;
> >  class netlink_socket { setopt getopt create connect getattr write 
> > read };
> > +class dir { search };
> > +class file { open getattr read };
> >  }
> >
> >  #= openvswitch_t ==
> >  allow openvswitch_t self:netlink_socket { setopt getopt create connect 
> > getattr write read };
> > +allow openvswitch_t domain:dir { search };
> > +allow openvswitch_t domain:file { open getattr read };
> 
> Hi Flavio,
> 
> Thanks for spending some time to get OVS in better shape with SELinux.
> I figure that once this settles down a bit we should take the policy
> file here and work towards upstreaming all of the policy changes.

Yeah, we can try to do both in parallel.  Once this gets in, I will
open the bz requesting to fix Fedora which would fix upstream too.

> As far as I can follow, this "domain" type is not just for accessing
> OVS directories and files (like openvswitch_t), but ifor a much wider
> range of paths:
> https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/4/html/SELinux_Guide/rhlcommon-section-0048.html
> 
> "# The domain attribute identifies every type that can be
> # assigned to a process.  This attribute is used in TE rules
> # that should be applied to all domains, e.g. permitting
> # init to kill all processes."
> 
> Is my understanding (+documentation) correct here? Is there an similar

Your understanding is correct.  Turns out that we don't know which
process will be the parent, so it could bash unconfined or initrc_t
or in any other context (neutron?).

> but more restrictive policy that allows ovs-vsctl to access, for
> example, /var/run/openvswitch/* (with var_run_openvswitch_t or
> similar)? Alternatively is there an example of another daemon that has
> a similar policy that set a precedence for writing the policy like
> this?

I spent few hours on this and I couldn't find a way to restrict it
more that I proposed with selinux.  Basically the above is an expansion
of the interface domain_read_all_domains_state()[1] which other
applications are using to read other processes states.  However, that
seemed relatively new and probably not available on older distros, so
I have expanded to the relevant actions removing what we don't need.

[1] http://danwalsh.livejournal.com/51435.html

> Would you also be able to provide the full ovs-vsctl commandline? It
> was a little difficult to understand exactly what was going on during
> this event, or try to reproduce.

utilities/ovs-vsctl.c:2473

2472 static char *
2473 vsctl_parent_process_info(void)
2474 {
2475 #ifdef __linux__
2476 pid_t parent_pid;
2477 char *procfile;
2478 struct ds s;
2479 FILE *f;
2480 
2481 parent_pid = getppid();
2482 procfile = xasprintf("/proc/%d/cmdline", parent_pid);
2483 
2484 f = fopen(procfile, "r");

That is called from do_vsctl() to find the parent info.  If you run as
root, then it's unconfined and it works, but it doesn't work during
boot time (initrc_t) for instance.

To reproduce you just need to configure an OVS interface using ifcfg
with ONBOOT=yes and reboot.

> Lastly, I've just applied the other SELinux patch so you'll need to
> rebase this one.

Sure, not a problem.

-- 
fbl

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] rhel: Allow openvswitch to get parent information

[Joe Stringer]

On 25 July 2016 at 18:16, Flavio Leitner  wrote:
> Updates SELinux to allow ovs-vsctl to get parent process
> information and log that to the database:
>
> record 241: 2016-07-26 00:59:47.418 "ovs-vsctl (invoked by /bin/bash
> (pid 1589)): ovs-vsctl -t 10 -- --if-exist ...
>
> Jul 25 12:57:35 localhost.localdomain audit[830]: AVC avc:  denied  {
> search } for  pid=830 comm="ovs-vsctl" name="731" dev="proc" ino=14140
> scontext=system_u:system_r:openvswitch_t:s0
> tcontext=system_u:system_r:initrc_t:s0 tclass=dir permissive=0
>
> Signed-off-by: Flavio Leitner 
> ---
>  selinux/openvswitch-custom.te | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/selinux/openvswitch-custom.te b/selinux/openvswitch-custom.te
> index fc32b97..5739595 100644
> --- a/selinux/openvswitch-custom.te
> +++ b/selinux/openvswitch-custom.te
> @@ -2,8 +2,13 @@ module openvswitch-custom 1.0;
>
>  require {
>  type openvswitch_t;
> +attribute domain;
>  class netlink_socket { setopt getopt create connect getattr write 
> read };
> +class dir { search };
> +class file { open getattr read };
>  }
>
>  #= openvswitch_t ==
>  allow openvswitch_t self:netlink_socket { setopt getopt create connect 
> getattr write read };
> +allow openvswitch_t domain:dir { search };
> +allow openvswitch_t domain:file { open getattr read };

Hi Flavio,

Thanks for spending some time to get OVS in better shape with SELinux.
I figure that once this settles down a bit we should take the policy
file here and work towards upstreaming all of the policy changes.

As far as I can follow, this "domain" type is not just for accessing
OVS directories and files (like openvswitch_t), but ifor a much wider
range of paths:
https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/4/html/SELinux_Guide/rhlcommon-section-0048.html

"# The domain attribute identifies every type that can be
# assigned to a process.  This attribute is used in TE rules
# that should be applied to all domains, e.g. permitting
# init to kill all processes."

Is my understanding (+documentation) correct here? Is there an similar
but more restrictive policy that allows ovs-vsctl to access, for
example, /var/run/openvswitch/* (with var_run_openvswitch_t or
similar)? Alternatively is there an example of another daemon that has
a similar policy that set a precedence for writing the policy like
this?

Would you also be able to provide the full ovs-vsctl commandline? It
was a little difficult to understand exactly what was going on during
this event, or try to reproduce.

Lastly, I've just applied the other SELinux patch so you'll need to
rebase this one.

Cheers,
Joe
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 2/2] rhel: Allow openvswitch to get parent information

[Flavio Leitner]

Updates SELinux to allow ovs-vsctl to get parent process
information and log that to the database:

record 241: 2016-07-26 00:59:47.418 "ovs-vsctl (invoked by /bin/bash
(pid 1589)): ovs-vsctl -t 10 -- --if-exist ...

Jul 25 12:57:35 localhost.localdomain audit[830]: AVC avc:  denied  {
search } for  pid=830 comm="ovs-vsctl" name="731" dev="proc" ino=14140
scontext=system_u:system_r:openvswitch_t:s0
tcontext=system_u:system_r:initrc_t:s0 tclass=dir permissive=0

Signed-off-by: Flavio Leitner 
---
 selinux/openvswitch-custom.te | 5 +
 1 file changed, 5 insertions(+)

diff --git a/selinux/openvswitch-custom.te b/selinux/openvswitch-custom.te
index fc32b97..5739595 100644
--- a/selinux/openvswitch-custom.te
+++ b/selinux/openvswitch-custom.te
@@ -2,8 +2,13 @@ module openvswitch-custom 1.0;
 
 require {
 type openvswitch_t;
+attribute domain;
 class netlink_socket { setopt getopt create connect getattr write read 
};
+class dir { search };
+class file { open getattr read };
 }
 
 #= openvswitch_t ==
 allow openvswitch_t self:netlink_socket { setopt getopt create connect getattr 
write read };
+allow openvswitch_t domain:dir { search };
+allow openvswitch_t domain:file { open getattr read };
-- 
2.7.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev