[ovs-dev] [PATCH v12 3/6] netdev-dpdk: Restrict vhost_sock_dir

[Daniele Di Proietto]

If vhost-sock-dir is empty, vhost_sock_dir will be NULL, because process_vhost_flags() will return 0.



On 26/04/2016 12:42, "Aaron Conole" <aconole at redhat.com> wrote:

>Since the vhost-user sockets directory now comes from the database, it is
>possible for any user with database access to program an arbitrary filesystem
>location for the sockets directory. This could result in unprivileged users
>creating or deleting arbitrary filesystem files by using specially crafted
>names. To prevent this, use the introduced ovs_realpath function to resolve
>the filesystem location and ensure that it resolves under the ovs_rundir.
>
>Signed-off-by: Aaron Conole <aconole at redhat.com>
>---
>Previous: http://openvswitch.org/pipermail/dev/2016-April/069030.html
>
>v12:
>* Converted to using strstr instead of canonicalization
>
> lib/netdev-dpdk.c    | 27 ++++++++++++++++++++-------
> vswitchd/vswitch.xml |  4 +++-
> 2 files changed, 23 insertions(+), 8 deletions(-)
>
>diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>index a393cee..9793fc5 100644
>--- a/lib/netdev-dpdk.c
>+++ b/lib/netdev-dpdk.c
>@@ -2897,6 +2897,9 @@ dpdk_init__(const struct ovsrec_open_vswitch *ovs_cfg)
>     int argc;
>     int err;
>     cpu_set_t cpuset;
>+#ifndef VHOST_CUSE
>+    char *sock_dir_subcomponent;
>+#endif
> 
>     if (!smap_get_bool(&ovs_cfg->other_config, "dpdk-init", false)) {
>         VLOG_INFO("DPDK Disabled - to change this requires a restart.\n");
>@@ -2909,15 +2912,25 @@ dpdk_init__(const struct ovsrec_open_vswitch *ovs_cfg)
>     if (process_vhost_flags("cuse-dev-name", xstrdup("vhost-net"),
>                             PATH_MAX, ovs_cfg, &cuse_dev_name)) {
> #else
>-    if (process_vhost_flags("vhost-sock-dir", xstrdup(ovs_rundir()),
>-                            NAME_MAX, ovs_cfg, &vhost_sock_dir)) {
>+    if (process_vhost_flags("vhost-sock-dir", xstrdup(""),
>+                            NAME_MAX, ovs_cfg, &sock_dir_subcomponent)) {
>         struct stat s;
>-
>-        err = stat(vhost_sock_dir, &s);
>-        if (err) {
>-            VLOG_ERR("vhost-user sock directory '%s' does not exist.",
>-                      vhost_sock_dir);
>+        if (!strstr(sock_dir_subcomponent, "..")) {
>+            vhost_sock_dir = xasprintf("%s/%s", ovs_rundir(),
>+                                       sock_dir_subcomponent);
>+
>+            err = stat(vhost_sock_dir, &s);
>+            if (err) {
>+                VLOG_ERR("vhost-user sock directory '%s' does not exist.",
>+                         vhost_sock_dir);
>+            }
>+        } else {
>+            vhost_sock_dir = xstrdup(ovs_rundir());
>+            VLOG_ERR("vhost-user sock directory request '%s/%s' has invalid"
>+                     "characters '..' - using %s instead.",
>+                     ovs_rundir(), sock_dir_subcomponent, ovs_rundir());
>         }
>+        free(sock_dir_subcomponent);
> #endif
>     }
> 
>diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>index 5965b10..e6d7359 100644
>--- a/vswitchd/vswitch.xml
>+++ b/vswitchd/vswitch.xml
>@@ -302,7 +302,9 @@
>       <column name="other_config" key="vhost-sock-dir"
>               type='{"type": "string"}'>
>         <p>
>-          Specifies the path to the vhost-user unix domain socket files.
>+          Specifies the path to the vhost-user unix domain socket files. This
>+          path must exist and be a subdirectory tree of the Open vSwitch
>+          run directory.
>         </p>
>         <p>
>           Defaults to the working directory of the application. Changing this
>-- 
>2.5.5
>