Re: [libvirt] [PATCH] virNetSocketNewConnectSSH: Escape netcat path too
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.
.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.
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.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org