<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jan 21, 2015 at 1:15 PM, Richard W.M. Jones <span dir="ltr"><<a href="mailto:rjones@redhat.com" target="_blank">rjones@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>On Wed, Jan 21, 2015 at 12:59:40PM -0800, David Lutterkort wrote:<br>
> The thing that makes me nervous the most about this change is that<br>
> it changes the paths that people get back from Augeas, especially<br>
> from aug_match.<br>
<br>
</span>[I think the first thing to say is that we only care about the API, not<br>
about augtool.]<br>
<br>
I've probably not understood the full implications of this.<br>
<br>
Programs like libvirt and virt-v2v use the aug_match a lot, and in<br>
some cases pass those strings back to aug_get, aug_set, aug_rm.  There<br>
are many examples of this in the following file (search for<br>
"aug_match"):<br></blockquote><div><br></div><div>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.<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<a href="https://github.com/libguestfs/libguestfs/blob/master/v2v/convert_linux.ml" target="_blank">https://github.com/libguestfs/libguestfs/blob/master/v2v/convert_linux.ml</a><br>
<br>
For example:<br>
<br>
 let expr =<br>
   sprintf "/file/etc/sysconfig/kernel/%s/value[. = '%s']"<br>
     var xen_mod in<br>
  let entries = g#aug_match expr in<br>
  let entries = Array.to_list entries in<br>
  if entries <> [] then (<br>
    List.iter (fun e -> ignore (g#aug_rm e)) entries;<br>
    modified := true<br>
  )<br></blockquote><div><br></div><div>Yes, this will now work, assuming you replace 'var' in there with '(g#aug_escape_name var)'<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Our real concern is where strings get interpolated into an Augeas<br>
expression, especially if those string come from untrusted user input<br>
(which is not the case in that file, but could be in general).<br></blockquote><div><br></div><div>As long as these strings go through aug_escape_name before sticking them into a path, all should be well.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span>
> While those can now be directly fed to aug_get, they can no longer<br>
> be used to find the underlying file directly. Not sure if that will<br>
> cause problems for anybody.<br>
<br>
</span>I'm not really sure what this means.  What is "underlying file" in<br>
this context?  Would it affect code like the above?  Can you give an<br>
example of a problem case?</blockquote><div><br></div><div>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).<br><br></div><div>David<br></div></div></div></div>