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

[Ben Pfaff]

I think I'd prefer to avoid path canonicalization.  If it proves not
flexible enough, we can add it in later.

Thanks,

Ben.

On Wed, Apr 13, 2016 at 12:08:58PM -0400, Aaron Conole wrote:
> Hi Ben,
> 
> I have rebased (due to conflicts) the series and am set to resubmit;
> however I'd like to get closure on this issue before I do.
> 
> I have no strong feelings either way - I can manually scan the string
> for .. or use the realpath code I've put here. Since this only happens
> at initialization time, I figured it didn't matter how heavy it was,
> but that may not be the case (especially due to where initialization
> occurs).
> 
> Please advise what you would rather see - I can make the changes and test
> with them.
> 
> Thanks :-)
> -Aaron
> 
> Aaron Conole <aconole at redhat.com> writes:
> 
> > 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.
> >
> > I do agree it's heavy.
> >
> >> Thanks,
> >
> > Thanks for the review!
> >
> >> Ben.
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev