> -----Original Message----- > From: Aaron Conole [mailto:acon...@redhat.com] > Sent: Tuesday, April 12, 2016 2:18 PM > To: Mooney, Sean K <sean.k.moo...@intel.com> > Cc: Ben Pfaff <b...@ovn.org>; dev@openvswitch.org; Flavio Leitner > <f...@sysclose.org>; Traynor, Kevin <kevin.tray...@intel.com>; Panu > Matilainen <pmati...@redhat.com>; Wojciechowicz, RobertX > <robertx.wojciechow...@intel.com>; Andy Zhou <az...@ovn.org>; Daniele Di > Proietto <diproiet...@vmware.com>; Zoltan Kiss <zoltan.k...@linaro.org>; > Christian Ehrhardt <christian.ehrha...@canonical.com> > Subject: Re: [PATCH v11 2/8] util: Add a path canonicalizer > > "Mooney, Sean K" <sean.k.moo...@intel.com> writes: > > >> -----Original Message----- > >> From: Aaron Conole [mailto:acon...@redhat.com] > >> Sent: Tuesday, April 12, 2016 1:12 AM > >> To: Ben Pfaff <b...@ovn.org> > >> Cc: dev@openvswitch.org; Flavio Leitner <f...@sysclose.org>; Traynor, > >> Kevin <kevin.tray...@intel.com>; Panu Matilainen > >> <pmati...@redhat.com>; Wojciechowicz, RobertX > >> <robertx.wojciechow...@intel.com>; Mooney, Sean K > >> <sean.k.moo...@intel.com>; Andy Zhou <az...@ovn.org>; Daniele Di > >> Proietto <diproiet...@vmware.com>; Zoltan Kiss > >> <zoltan.k...@linaro.org>; Christian Ehrhardt > >> <christian.ehrha...@canonical.com> > >> Subject: Re: [PATCH v11 2/8] util: Add a path canonicalizer > >> > >> Hi Ben, > >> > >> Ben Pfaff <b...@ovn.org> writes: > >> > >> > (Yow, that's a lot of CCs.) > >> > >> Lots of cooks in the kitchen on this one. > >> > >> > On Fri, Apr 01, 2016 at 11:31:31AM -0400, Aaron Conole wrote: > >> >> This commit adds a new function (ovs_realpath) to perform the role > >> >> of realpath on various operating systems. The purpose is to ensure > >> >> that a given path to file exists, and to return a completely > >> >> resolved path (sans '.' and '..'). > >> >> > >> >> Signed-off-by: Aaron Conole <acon...@redhat.com> > >> > > >> > Path canonicalization is a pretty big hammer. In other cases where > >> > OVS relies on an absolute path, it uses textual comparison on a > >> > prefix of the name (representing a directory) and rejects use of > ".." > >> > following the prefix. This is pretty easy to get right, and it is > >> > not as heavyweight, and it does not have to actually do file system > >> > operations (stat, readlink, ...), and its verdict can't change as a > >> > result of changes to the file system (e.g. new or modified or > >> > deleted symlinks, NFS servers that are down), and so on. > >> > > >> > Do you think we really need path canonicalization? > >> > >> I was nervous about a user putting escapes in the code. Unlike with, > >> say, vhost user filenames (where we just blanket deny '/' because the > >> semantic is of a file) this is not a file specification, but a > >> directory specification. That implies that we would have to keep > >> state and test for /../, and ../ (at the beginning of the string), at > the least. > >> > >> If you think it's safe to merely test for the presence of these and > >> bail, I'm okay with it, but I didn't want to leave any possibility > >> that a malicious DB user could escape out of the rundir when changing > >> the vhost-socket dir. > > [Mooney, Sean K] > > currently it is possible to use any dir for the vhost-socket dir if > > the absolute path is specified on the ovs-vswitchd command line > > correct? > > I know for a time we used /tmp/ for the sockets. I have also seen > > /dev/vhostuser/ used. > > The run dir (/var/run/openvswitch or /usr/var/run/openvswitch) makes > > the most sense To me as a default. > > > > More of a clarifying question but are you proposing restricting the > > localtions where the vhost-user sockets can be stored or constraining > > the path format to allow/deny relative path specifiers such as "." and > > ".." and converting that path in an canonical absolute path? > > > > I don't have a strong preference but just wanted to make sure I > > understand what is being proposed And what flexibility we would be > > gaining/losing. > > Sure; this would restrict all paths specified in the string to being > subpaths of the ovs_rundir(). So, your case of using /tmp or > /dev/vhostuser/ would not work. On the other hand, a path like > vhostuser/ would be treated as /var/run/openvswitch/vhostuser/. The code > is merely attempting to prevent arbitrary directory escapes by using the > realpath system call. > > I added this after > http://openvswitch.org/pipermail/dev/2016-March/068743.html [Mooney, Sean K] Ah ok that restriction makes sense in that context. We swapped to using the ovs run directory (e.g. /var/run/openvswitch/) when the vhost-user patches were merge to ovs so this will not affect the OpenStack integration with Neutron or odl so this seems ok to me.
Thanks for clarifying. > > Thanks, > -Aaron > > >> > >> I do agree it's heavy. > >> > >> > Thanks, > >> > >> Thanks for the review! > >> > >> > Ben. > > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
