7:18 a.m.
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
9:28 a.m.
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
2:38 p.m.
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
4784
days inactive
4788
days old
2 comments
2 participants
participants (2)
-
Eric Blake -
Guido Günther