"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 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