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