diff mbox

[ovs-dev,PATCHv2] bridge: allow OVS to interact with controller through sockets outside run dir

Message ID 1466897930-16306-1-git-send-email-aatteka@ovn.org
State Superseded
Headers show

Commit Message

Ansis Atteka June 25, 2016, 11:38 p.m. UTC 
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 socket owned by other daemon on the same
hypervisor.

This patch allows to disable this behavior by changing
/etc/default/openvswitch (Ubuntu) or /etc/sysconfig/openvswitch (RHEL)
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.

The initial use-case for this patch is to allow to connect to OpenFlow
controller that has its socket outside OVS run directory.  However,
in the future it could be generalized to allow to disable self-confinement
for other things like DPDK vhost-user sockets or anything else
that is specifiable in OVSDB with full path.

Signed-off-by: Ansis Atteka <aatteka@ovn.org>
VMware-BZ: #1525857
---
 NEWS                 |  2 ++
 lib/daemon-syn.man   |  1 +
 lib/daemon.c         | 14 ++++++++++++++
 lib/daemon.h         | 14 ++++++++++++++
 lib/daemon.man       | 12 ++++++++++++
 utilities/ovs-ctl.8  |  5 +++++
 utilities/ovs-ctl.in | 21 ++++++++++++++-------
 vswitchd/bridge.c    |  5 +++--
 8 files changed, 65 insertions(+), 9 deletions(-)

Comments

Jesse Gross June 27, 2016, 6:37 p.m. UTC | #1 
On Sat, Jun 25, 2016 at 4:38 PM, Ansis Atteka <aatteka@ovn.org> wrote:
> diff --git a/lib/daemon.h b/lib/daemon.h
> index 4990415..742f382 100644
> --- a/lib/daemon.h
> +++ b/lib/daemon.h
[...]
> +extern bool self_confine;

I don't really like that this is global to everything - it seems too
easy to accidentally assign to or at the very least be non-obvious
where it is coming from. Can you make it a function call and also
prefix the name with daemon_?

I think it would also be nice to just include description of the 3
reasons why you might want to use this flag from the commit message in
the daemon man page. That makes it very clear when you should use this
(or not).
Ansis June 28, 2016, 2:28 a.m. UTC | #2 
On 27 June 2016 at 11:37, Jesse Gross <jesse@kernel.org> wrote:

> On Sat, Jun 25, 2016 at 4:38 PM, Ansis Atteka <aatteka@ovn.org> wrote:
> > diff --git a/lib/daemon.h b/lib/daemon.h
> > index 4990415..742f382 100644
> > --- a/lib/daemon.h
> > +++ b/lib/daemon.h
> [...]
> > +extern bool self_confine;
>
> I don't really like that this is global to everything - it seems too
> easy to accidentally assign to or at the very least be non-obvious
> where it is coming from. Can you make it a function call and also
> prefix the name with daemon_?
>
> I think it would also be nice to just include description of the 3
> reasons why you might want to use this flag from the commit message in
> the daemon man page. That makes it very clear when you should use this
> (or not).
>

Thanks for review. How about this one -
https://patchwork.ozlabs.org/patch/641301/ ?


> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
diff mbox

Patch

diff --git a/NEWS b/NEWS
index d00b183..987e985 100644
--- a/NEWS
+++ b/NEWS
@@ -70,6 +70,8 @@  Post-v2.5.0
      * Added support for IPv6 tunnels to native tunneling.
    - A wrapper script, 'ovs-tcpdump', to easily port-mirror an OVS port and
      watch with tcpdump
+   - Introduce --no-self-confinement flag that allows daemons to work with
+     sockets outside their run directory.
 
 v2.5.0 - 26 Feb 2016
 ---------------------
diff --git a/lib/daemon-syn.man b/lib/daemon-syn.man
index fcc15cc..9d15939 100644
--- a/lib/daemon-syn.man
+++ b/lib/daemon-syn.man
@@ -3,3 +3,4 @@ 
 [\fB\-\-overwrite\-pidfile\fR]
 [\fB\-\-detach\fR]
 [\fB\-\-no\-chdir\fR]
+[\fB\-\-no\-self\-confinement\fR]
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/lib/daemon.man b/lib/daemon.man
index 78469cd..d8e0f0e 100644
--- a/lib/daemon.man
+++ b/lib/daemon.man
@@ -55,6 +55,18 @@  is not a good directory to use.
 This option has no effect when \fB\-\-detach\fR is not specified.
 .
 .TP
+\fB\-\-no\-self\-confinement\fR
+By default daemon will try to self-confine itself to work with
+files under well-know, at build-time whitelisted directories.
+This flag allows to disable this behavior.
+.IP
+Note that in contrast to other access control implementations that
+are typically enforced from kernel-space (e.g. DAC or MAC),
+self-confinement is imposed from the user-space daemon itself and
+hence should not be considered as a full confinement strategy, but
+instead should be viewed as an additional layer of security.
+.
+.TP
 \fB\-\-user\fR
 Causes \fB\*(PN\fR to run as a different user specified in "user:group", thus
 dropping most of the root privileges. Short forms "user" and ":group" are also
diff --git a/utilities/ovs-ctl.8 b/utilities/ovs-ctl.8
index 4b441b4..14ba9d9 100644
--- a/utilities/ovs-ctl.8
+++ b/utilities/ovs-ctl.8
@@ -180,6 +180,11 @@  By default \fBovs\-ctl\fR passes \fB\-\-mlockall\fR to
 memory, preventing it from being paged to disk.  This option
 suppresses that behavior.
 .
+.IP "\fB\-\-no\-self\-confinement\fR"
+Disable self-confinement for \fBovs-vswitchd\fR and \fBovsdb\-server\fR
+daemons.  This flag should be used when, for example, OpenFlow controller
+creates its Unix Domain Socket outside OVS run directory.
+.
 .IP "\fB\-\-ovsdb\-server\-priority=\fIniceness\fR"
 .IQ "\fB\-\-ovs\-vswitchd\-priority=\fIniceness\fR"
 Sets the \fBnice\fR(1) level used for each daemon.  All of them
diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
index 6bc7ced..b2a97fc 100755
--- a/utilities/ovs-ctl.in
+++ b/utilities/ovs-ctl.in
@@ -162,6 +162,9 @@  do_start_ovsdb () {
                 log_warning_msg "$db (from \$EXTRA_DBS) cannot be read as a database (see error message above)"
             fi
         done
+        if test X"$SELF_CONFINEMENT" = Xno; then
+            set "$@" --no-self-confinement
+        fi
         set "$@" -vconsole:emer -vsyslog:err -vfile:info
         set "$@" --remote=punix:"$DB_SOCK"
         set "$@" --private-key=db:Open_vSwitch,SSL,private_key
@@ -226,13 +229,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 +498,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;