[augeas-devel] escaping special characters in the path sent to aug_get()

[Richard W.M. Jones]

On Wed, Jan 21, 2015 at 03:45:32PM -0800, David Lutterkort wrote:
> On Wed, Jan 21, 2015 at 1:15 PM, Richard W.M. Jones <rjones at redhat.com>
> wrote:
> 
> > On Wed, Jan 21, 2015 at 12:59:40PM -0800, David Lutterkort wrote:
> > > The thing that makes me nervous the most about this change is that
> > > it changes the paths that people get back from Augeas, especially
> > > from aug_match.
> >
> > [I think the first thing to say is that we only care about the API, not
> > about augtool.]
> >
> > I've probably not understood the full implications of this.
> >
> > Programs like libvirt and virt-v2v use the aug_match a lot, and in
> > some cases pass those strings back to aug_get, aug_set, aug_rm.  There
> > are many examples of this in the following file (search for
> > "aug_match"):
> >
> 
> With this change, taking paths returned from aug_match and feeding them
> into aug_get, aug_set, aug_rm etc. now actually becomes safe: before,
> aug_match would return paths that really needed escaping, but weren't so
> that passing them to aug_get could fail. This will no longer be an issue.
> 
> https://github.com/libguestfs/libguestfs/blob/master/v2v/convert_linux.ml
> >
> > For example:
> >
> >  let expr =
> >    sprintf "/file/etc/sysconfig/kernel/%s/value[. = '%s']"
> >      var xen_mod in
> >   let entries = g#aug_match expr in
> >   let entries = Array.to_list entries in
> >   if entries <> [] then (
> >     List.iter (fun e -> ignore (g#aug_rm e)) entries;
> >     modified := true
> >   )
> >
> 
> Yes, this will now work, assuming you replace 'var' in there with
> '(g#aug_escape_name var)'
> 
> 
> > Our real concern is where strings get interpolated into an Augeas
> > expression, especially if those string come from untrusted user input
> > (which is not the case in that file, but could be in general).
> >
> 
> As long as these strings go through aug_escape_name before sticking them
> into a path, all should be well.
> 
> 
> >  > While those can now be directly fed to aug_get, they can no longer
> > > be used to find the underlying file directly. Not sure if that will
> > > cause problems for anybody.
> >
> > I'm not really sure what this means.  What is "underlying file" in
> > this context?  Would it affect code like the above?  Can you give an
> > example of a problem case?
> 
> 
> Sorry, this was a bit obtuse: before this change, you could take a path
> returned by aug_match, strip off '/files' from the beginning and use the
> rest as the path to the actual file in the filesystem. After this change,
> that's no longer the case. If you have a file '/etc/weird-[a]' in the
> filesystem, an aug_match for something in that file will return
> '/files/etc/weird-\[a\]' - you can now pass that back to aug_get, but you
> can't use that to open the actual file (because of the extra '\' in there).

All sounds good to me.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html