[libvirt] [PATCH] virNetSocketNewConnectSSH: Escape netcat path too

[Guido Günther]

On Fri, Oct 14, 2011 at 08:28:48AM -0600, Eric Blake wrote:
> On 10/14/2011 06:18 AM, Guido Günther wrote:
> >What holds for netcat is also true for the socket path since it can be
> >part of the connection URI as well. Make both subject to the same amount
> >of shell parsing.
> 
> Hmm, I'm not sure about this.
> 
> >-        .path = "/tmp/socket",
> >+        .path = "/tmp/so cket",
> 
> First off, is the socket name ever likely to have shell
> metacharacters?  Or is it always something created by libvirt, using
> only sane naming conventions?  If we are guaranteed that the socket
> path is sane, then it never needs quoting.

You can specify the socket as well on the connection URI:

virsh -c 'qemu+ssh://<host>/system?socket=/tmp/foo' list

> >          .expectOut = "somehost sh -c 'if ''nc -4'' -q 2>&1 | grep \"requires an argument\">/dev/null 2>&1; then "
> >                                           "ARG=-q0;"
> >                                       "else "
> >                                           "ARG=;"
> >                                       "fi;"
> >-                                     "''nc -4'' $ARG -U /tmp/socket'\n",
> >+                                     "''nc -4'' $ARG -U ''/tmp/so cket'\n",
> 
> Second, if the socket name is ever not sane, then this isn't quoted
> correctly.  Notice the resulting command:
> 
> sh -c '...; ''nc -4'' -U ''/tmp/so cket'
> 
> is the same as:
> 
> sh -c '...; nc -4 -U /tmp/so cket'
> 
> which passes the arguments "/tmp/so" and "cket" to nc.  We _want_ to
> do word splitting on the netcat argument (that is, if the user
> passes "nc -4" for their nc program, we want the shell to see "nc"
> and "-4" separately), but we want the socket name to be a single
> entity.

The main motivation was to have the same level of quoting on nc and
socket so they behave the same (principle of least surprise). 

> 
> I'm inclined to NACK this patch unless you can prove that a
> situation exists where a socket name with metacharacters is
> attempted, and even then, fix the patch to quote the name properly.
>
> 
> I'm also still a bit worried that we aren't quite quoting things
> properly.  ssh does crazy things with its arguments, almost doing a
> full round of shell expansions, prior to ever calling sh -c in the
> first place; and then sh -c does another round of expansions.

The current code tries to minimize the rounds of quoting (as you
suggested) and which I think is good. What I still don't like is that
one can trivially do things like:

	virsh -c 'qemu+ssh://<host>/system?socket=%3Btouch%20/tmp/test'

But since we assume shell access anyway and since the user can invoke
any binary by giving the nc command this is probably a non issue.
Cheers,
 -- Guido