[libvirt] [PATCH] virNetSocketNewConnectSSH: Escape netcat path too
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. Cheers, -- Guido --- src/rpc/virnetsocket.c | 30 +++++++++++++++++++++--------- tests/virnetsockettest.c | 14 +++++++------- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index e4eff49..d7a055f 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -612,9 +612,10 @@ int virNetSocketNewConnectSSH(const char *nodename, const char *path, virNetSocketPtr *retsock) { - char *quoted; + char *qnetcat, *qpath; virCommandPtr cmd; - virBuffer buf = VIR_BUFFER_INITIALIZER; + virBuffer ncbuf = VIR_BUFFER_INITIALIZER; + virBuffer pathbuf = VIR_BUFFER_INITIALIZER; *retsock = NULL; @@ -643,13 +644,23 @@ int virNetSocketNewConnectSSH(const char *nodename, virCommandAddArgList(cmd, nodename, "sh", "-c", NULL); - virBufferEscapeShell(&buf, netcat); - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); + virBufferEscapeShell(&ncbuf, netcat); + if (virBufferError(&ncbuf)) { + virBufferFreeAndReset(&ncbuf); virReportOOMError(); return -1; } - quoted = virBufferContentAndReset(&buf); + + virBufferEscapeShell(&pathbuf, path); + if (virBufferError(&pathbuf)) { + virBufferFreeAndReset(&ncbuf); + virBufferFreeAndReset(&pathbuf); + virReportOOMError(); + return -1; + } + + qnetcat = virBufferContentAndReset(&ncbuf); + qpath = virBufferContentAndReset(&pathbuf); /* * This ugly thing is a shell script to detect availability of * the -q option for 'nc': debian and suse based distros need this @@ -666,10 +677,11 @@ int virNetSocketNewConnectSSH(const char *nodename, "else " "ARG=;" "fi;" - "'%s' $ARG -U %s'", - quoted, quoted, path); + "'%s' $ARG -U '%s", + qnetcat, qnetcat, qpath); - VIR_FREE(quoted); + VIR_FREE(qnetcat); + VIR_FREE(qpath); return virNetSocketNewConnectCommand(cmd, retsock); } diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index 6320ce0..5c93f0f 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -501,7 +501,7 @@ mymain(void) "else " "ARG=;" "fi;" - "'nc' $ARG -U /tmp/socket'\n", + "'nc' $ARG -U '/tmp/socket\n", }; if (virtTestRun("SSH test 1", 1, testSocketSSH, &sshData1) < 0) ret = -1; @@ -520,7 +520,7 @@ mymain(void) "else " "ARG=;" "fi;" - "'netcat' $ARG -U /tmp/socket'\n", + "'netcat' $ARG -U '/tmp/socket\n", }; if (virtTestRun("SSH test 2", 1, testSocketSSH, &sshData2) < 0) ret = -1; @@ -539,7 +539,7 @@ mymain(void) "else " "ARG=;" "fi;" - "'netcat' $ARG -U /tmp/socket'\n", + "'netcat' $ARG -U '/tmp/socket\n", }; if (virtTestRun("SSH test 3", 1, testSocketSSH, &sshData3) < 0) ret = -1; @@ -561,7 +561,7 @@ mymain(void) "else " "ARG=;" "fi;" - "'nc' $ARG -U /tmp/socket'\n", + "'nc' $ARG -U '/tmp/socket\n", .dieEarly = true, }; if (virtTestRun("SSH test 5", 1, testSocketSSH, &sshData5) < 0) @@ -578,7 +578,7 @@ mymain(void) "else " "ARG=;" "fi;" - "'nc' $ARG -U /tmp/socket'\n", + "'nc' $ARG -U '/tmp/socket\n", }; if (virtTestRun("SSH test 6", 1, testSocketSSH, &sshData6) < 0) ret = -1; @@ -586,13 +586,13 @@ mymain(void) struct testSSHData sshData7 = { .nodename = "somehost", .netcat = "nc -4", - .path = "/tmp/socket", + .path = "/tmp/so cket", .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", }; if (virtTestRun("SSH test 7", 1, testSocketSSH, &sshData7) < 0) ret = -1; -- 1.7.6.3
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
participants (2)
-
Eric Blake -
Guido Günther