[ovs-dev] [PATCH v11 2/8] util: Add a path canonicalizer

[Aaron Conole]

"Mooney, Sean K" <sean.k.mooney at intel.com> writes:

>> -----Original Message-----
>> From: Aaron Conole [mailto:aconole at redhat.com]
>> Sent: Tuesday, April 12, 2016 1:12 AM
>> To: Ben Pfaff <blp at ovn.org>
>> Cc: dev at openvswitch.org; Flavio Leitner <fbl at sysclose.org>; Traynor,
>> Kevin <kevin.traynor at intel.com>; Panu Matilainen <pmatilai at redhat.com>;
>> Wojciechowicz, RobertX <robertx.wojciechowicz at intel.com>; Mooney, Sean K
>> <sean.k.mooney at intel.com>; Andy Zhou <azhou at ovn.org>; Daniele Di
>> Proietto <diproiettod at vmware.com>; Zoltan Kiss <zoltan.kiss at linaro.org>;
>> Christian Ehrhardt <christian.ehrhardt at canonical.com>
>> Subject: Re: [PATCH v11 2/8] util: Add a path canonicalizer
>> 
>> Hi Ben,
>> 
>> Ben Pfaff <blp at 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 <aconole at 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.
>