| Message ID | 1466457580-7331-1-git-send-email-aatteka@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, Jun 20, 2016 at 02:19:40PM -0700, Ansis Atteka wrote: > Currently Open vSwitch is unable to create or connect to Unix Domain > Sockets outside designated 'run' directory, because of fear of potential > remote exploits where a hacked remote OVSDB manager would tell Open vSwitch > to connect to a unix domain sockets owned by other daemons on the same > hypervisor. > > This patch allows to disable this behavior by changing > /etc/default/openvswitch file to: > > ... > OVS_CTL_OPTS=--no-self-confinement > ... > > Note, that it is better to stick with default behavior, unless: > 1. You have Open vSwitch running under SELinux or AppArmor > that would prevent OVS from messing with sockets owned by other > daemons; OR > 2. You are sure that relying on OpenFlow handshake is enough to > prevent OVS to adversely interact with those other daemons > running on the same hypervisor; OR > 3. You don't have much worries of remote exploits in the first > place, because perhaps OVSDB manager is running on the same host > as OVS. > > Signed-off-by: Ansis Atteka <aatteka@ovn.org> > VMware-BZ: #1525857 I'm comfortable with this idea but I have some comments on the implementation. I am surprised to see this implemented in daemon.[ch]. Usually options implemented there are ones that every daemon can use, but so far at least only ovs-vswitchd supports this feature. Do you expect to extend this to other daemons soon? The description of the default behavior, above, is detailed and useful, but it is not easily accessible to users, who would need it to appear in the documentation. I guess that this should be documented in the ovs-vswitchd manpage, or if you really intend for it to be general, in daemon.man and daemon-syn.man, as well as in the ovs-ctl manpage. I'd add a NEWS item. Thanks, Ben.
On 22 June 2016 at 15:44, Ben Pfaff <blp@ovn.org> wrote: > On Mon, Jun 20, 2016 at 02:19:40PM -0700, Ansis Atteka wrote: > > Currently Open vSwitch is unable to create or connect to Unix Domain > > Sockets outside designated 'run' directory, because of fear of potential > > remote exploits where a hacked remote OVSDB manager would tell Open > vSwitch > > to connect to a unix domain sockets owned by other daemons on the same > > hypervisor. > > > > This patch allows to disable this behavior by changing > > /etc/default/openvswitch file to: > > > > ... > > OVS_CTL_OPTS=--no-self-confinement > > ... > > > > Note, that it is better to stick with default behavior, unless: > > 1. You have Open vSwitch running under SELinux or AppArmor > > that would prevent OVS from messing with sockets owned by other > > daemons; OR > > 2. You are sure that relying on OpenFlow handshake is enough to > > prevent OVS to adversely interact with those other daemons > > running on the same hypervisor; OR > > 3. You don't have much worries of remote exploits in the first > > place, because perhaps OVSDB manager is running on the same host > > as OVS. > > > > Signed-off-by: Ansis Atteka <aatteka@ovn.org> > > VMware-BZ: #1525857 > > I'm comfortable with this idea but I have some comments on the > implementation. > Thanks for review, I sent PATCHv2 that should address your concerns regarding documentation - https://patchwork.ozlabs.org/patch/640601/. > I am surprised to see this implemented in daemon.[ch]. Usually options > implemented there are ones that every daemon can use, but so far at > least only ovs-vswitchd supports this feature. Do you expect to extend > this to other daemons soon? > Yes, I think --no-self-confinement flag should extend over other daemons. > > The description of the default behavior, above, is detailed and useful, > but it is not easily accessible to users, who would need it to appear in > the documentation. I guess that this should be documented in the > ovs-vswitchd manpage, or if you really intend for it to be general, in > daemon.man and daemon-syn.man, as well as in the ovs-ctl manpage. > > I'd add a NEWS item. > > Thanks, > > Ben. >
diff --git a/lib/daemon.c b/lib/daemon.c index b8313d4..6273ebd 100644 --- a/lib/daemon.c +++ b/lib/daemon.c @@ -29,6 +29,13 @@ VLOG_DEFINE_THIS_MODULE(daemon); * /dev/null (if false) or keep it for the daemon to use (if true). */ static bool save_fds[3]; +/* Self Confinement is a security feature that introduces additional + * layer of defense where OVS in self-denying manner would refuse to connect + * to or create unix domain sockets outside designated 'run' directory even + * if remote (or local) OVSDB manager asked it to do so. This feature may + * be disabled if Mandatory Access Control is used. */ +bool self_confine = true; + /* Will daemonize() really detach? */ bool get_detach(void) @@ -59,6 +66,13 @@ set_pidfile(const char *name) pidfile = make_pidfile_name(name); } +/* Disables self confinement. */ +void +daemon_disable_self_confinement(void) +{ + self_confine = false; +} + /* A daemon doesn't normally have any use for the file descriptors for stdin, * stdout, and stderr after it detaches. To keep these file descriptors from * e.g. holding an SSH session open, by default detaching replaces each of diff --git a/lib/daemon.h b/lib/daemon.h index 4990415..742f382 100644 --- a/lib/daemon.h +++ b/lib/daemon.h @@ -39,6 +39,7 @@ #ifndef _WIN32 #define DAEMON_OPTION_ENUMS \ OPT_DETACH, \ + OPT_NO_SELF_CONFINEMENT, \ OPT_NO_CHDIR, \ OPT_OVERWRITE_PIDFILE, \ OPT_PIDFILE, \ @@ -47,6 +48,7 @@ #define DAEMON_LONG_OPTIONS \ {"detach", no_argument, NULL, OPT_DETACH}, \ + {"no-self-confinement", no_argument, NULL, OPT_NO_SELF_CONFINEMENT}, \ {"no-chdir", no_argument, NULL, OPT_NO_CHDIR}, \ {"pidfile", optional_argument, NULL, OPT_PIDFILE}, \ {"overwrite-pidfile", no_argument, NULL, OPT_OVERWRITE_PIDFILE}, \ @@ -58,6 +60,10 @@ set_detach(); \ break; \ \ + case OPT_NO_SELF_CONFINEMENT: \ + daemon_disable_self_confinement(); \ + break; \ + \ case OPT_NO_CHDIR: \ set_no_chdir(); \ break; \ @@ -86,6 +92,7 @@ pid_t read_pidfile(const char *name); #else #define DAEMON_OPTION_ENUMS \ OPT_DETACH, \ + OPT_NO_SELF_CONFINEMENT, \ OPT_NO_CHDIR, \ OPT_PIDFILE, \ OPT_PIPE_HANDLE, \ @@ -95,6 +102,7 @@ pid_t read_pidfile(const char *name); #define DAEMON_LONG_OPTIONS \ {"detach", no_argument, NULL, OPT_DETACH}, \ + {"no-self-confinement" no_argument, NULL, OPT_NO_SELF_CONFINEMENT}, \ {"no-chdir", no_argument, NULL, OPT_NO_CHDIR}, \ {"pidfile", optional_argument, NULL, OPT_PIDFILE}, \ {"pipe-handle", required_argument, NULL, OPT_PIPE_HANDLE}, \ @@ -106,6 +114,10 @@ pid_t read_pidfile(const char *name); case OPT_DETACH: \ break; \ \ + case OPT_NO_SELF_CONFINEMENT: \ + daemon_disable_self_confinement(); \ + break; \ + \ case OPT_NO_CHDIR: \ break; \ \ @@ -138,10 +150,12 @@ void daemonize_complete(void); void daemon_set_new_user(const char * user_spec); void daemon_become_new_user(bool access_datapath); void daemon_usage(void); +void daemon_disable_self_confinement(void); void service_start(int *argcp, char **argvp[]); void service_stop(void); bool should_service_stop(void); void set_pidfile(const char *name); void close_standard_fds(void); +extern bool self_confine; #endif /* daemon.h */ diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in index 6bc7ced..1f03f96 100755 --- a/utilities/ovs-ctl.in +++ b/utilities/ovs-ctl.in @@ -226,13 +226,16 @@ do_start_forwarding () { ulimit -n $MAXFD fi - # Start ovs-vswitchd. - set ovs-vswitchd unix:"$DB_SOCK" - set "$@" -vconsole:emer -vsyslog:err -vfile:info - if test X"$MLOCKALL" != Xno; then - set "$@" --mlockall - fi - start_daemon "$OVS_VSWITCHD_PRIORITY" "$OVS_VSWITCHD_WRAPPER" "$@" + # Start ovs-vswitchd. + set ovs-vswitchd unix:"$DB_SOCK" + set "$@" -vconsole:emer -vsyslog:err -vfile:info + if test X"$MLOCKALL" != Xno; then + set "$@" --mlockall + fi + if test X"$SELF_CONFINEMENT" = Xno; then + set "$@" --no-self-confinement + fi + start_daemon "$OVS_VSWITCHD_PRIORITY" "$OVS_VSWITCHD_WRAPPER" "$@" fi } @@ -492,6 +495,7 @@ set_defaults () { DAEMON_CWD=/ FORCE_COREFILES=yes MLOCKALL=yes + SELF_CONFINEMENT=yes OVSDB_SERVER=yes OVS_VSWITCHD=yes OVSDB_SERVER_PRIORITY=-10 diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 4273552..9b97d9a 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -3561,8 +3561,9 @@ bridge_configure_remotes(struct bridge *br, for (i = 0; i < n_controllers; i++) { struct ovsrec_controller *c = controllers[i]; - if (!strncmp(c->target, "punix:", 6) - || !strncmp(c->target, "unix:", 5)) { + if (self_confine + && (!strncmp(c->target, "punix:", 6) + || !strncmp(c->target, "unix:", 5))) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); char *whitelist;
Currently Open vSwitch is unable to create or connect to Unix Domain Sockets outside designated 'run' directory, because of fear of potential remote exploits where a hacked remote OVSDB manager would tell Open vSwitch to connect to a unix domain sockets owned by other daemons on the same hypervisor. This patch allows to disable this behavior by changing /etc/default/openvswitch file to: ... OVS_CTL_OPTS=--no-self-confinement ... Note, that it is better to stick with default behavior, unless: 1. You have Open vSwitch running under SELinux or AppArmor that would prevent OVS from messing with sockets owned by other daemons; OR 2. You are sure that relying on OpenFlow handshake is enough to prevent OVS to adversely interact with those other daemons running on the same hypervisor; OR 3. You don't have much worries of remote exploits in the first place, because perhaps OVSDB manager is running on the same host as OVS. Signed-off-by: Ansis Atteka <aatteka@ovn.org> VMware-BZ: #1525857 --- lib/daemon.c | 14 ++++++++++++++ lib/daemon.h | 14 ++++++++++++++ utilities/ovs-ctl.in | 18 +++++++++++------- vswitchd/bridge.c | 5 +++-- 4 files changed, 42 insertions(+), 9 deletions(-)