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

Mooney, Sean K sean.k.mooney at intel.com
Tue Apr 12 12:29:10 UTC 2016



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

> 
> I do agree it's heavy.
> 
> > Thanks,
> 
> Thanks for the review!
> 
> > Ben.




More information about the dev mailing list