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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list