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.