On 07/27/2011 01:32 PM, Guido Günther wrote: Pasting from that URL gave awkward results below; can you address my comments below, then post a v2 as a proper patch against libvirt.git? Why is "sh -c" passed as a single argument, separate from the argument passed to that shell? Yes, ssh will join all arguments, then resplit the combined argument according to shell rules (I hate the fact that ssh duplicates shell word parsing), but we should either be providing a single argument to ssh with the entire shell script, or breaking this into one argument per word to be joined by ssh (three total), instead of doing half and half (two words in one argument, the third word [hairy script] in another). s/VNC tunnel/connection tunnel/ s/VNC connection attempts/connection attempts/ This code is not a VNC tunnel. grep -q is not portable to Solaris, even though it is required by POSIX. Instead, use grep with stdout and stderr redirected to /dev/null. Why split the nc probe from the if? We could use the more compact: "if %s -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then" I don't like this part. It is not safe to pass %s as the pathname through an additional round of shell parsing, since if the pathname has anything that might be construed as shell metacharacters, the parse will be destroyed. To some extent, that is already a pre-existing bug (slightly mitigated by the fact that 'path' is under libvirt's control, and should not be containing arbitrary characters unless you passed odd directory choices to ./configure). But we aren't even evaluating the 'netcat' variable for sanity - and that is an arbitrary string given completely by user input - sounds like we need an independent patch to reject a user-provided netcat that would be misparsed by ssh and any extra shell expansions. Without that, then this patch would need the burden of adding an extra level of escaping to match an extra level of shell parsing. Yuck. We should avoid eval at all costs, since that adds a third(!) level of shell parsing on top of the two we're already suffering from (ssh and sh -c). I'd much rather see: sh -c 'CMD=<either empty or -q 0>; nc $CMD -U path' than sh -c 'CMD=<either nc -q 0 -U path or nc -U path>; eval "$CMD"' By the way, you can avoid one level of shell parsing by using this printf: "sh -c 'CMD=..; \"$1\" $CMD -U \"$2\"' sh %s %s", netcat, path that is, supply 'netcat' and 'path' to ssh at the same level of quoting they have always been used, with the shell script referring only to positional parameters rather than doing yet another round of parsing on those parameters. But if we already have to plug the hole of user-provided 'netcat' being safe for ssh re-parsing, I don't know that we're doing much better by going through those extra hoops. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

Quote strings so they're safe to pass to the shell. Based on glib's g_quote_string. --- src/libvirt_private.syms | 1 + src/util/buf.c | 29 +++++++++++++++++++++++++++++ src/util/buf.h | 1 + 3 files changed, 31 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 853ee62..eb16fb6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -28,6 +28,7 @@ virBufferError; virBufferEscapeSexpr; virBufferEscapeString; virBufferFreeAndReset; +virBufferQuoteString; virBufferStrcat; virBufferURIEncodeString; virBufferUse; diff --git a/src/util/buf.c b/src/util/buf.c index 5002486..8106231 100644 --- a/src/util/buf.c +++ b/src/util/buf.c @@ -472,6 +472,35 @@ virBufferURIEncodeString (virBufferPtr buf, const char *str) } /** + * virBufferQuoteString: + * @buf: the buffer to append to + * @str: an unquoted string + * + * Quotes a string so that the shell (/bin/sh) will interpret the + * quoted string as unquoted_string. + */ +void +virBufferQuoteString(const virBufferPtr buf, const char *unquoted_str) +{ + const char *p; + + virBufferAddChar(buf, '\''); + p = unquoted_str; + + while (*p) { + /* Replace literal ' with a close ', a \', and a open ' */ + if (*p == '\'') + virBufferAddLit(buf, "'\\''"); + else + virBufferAddChar(buf, *p); + ++p; + } + + /* close the quote */ + virBufferAddChar(buf, '\''); +} + +/** * virBufferStrcat: * @buf: the buffer to dump * @...: the variable list of strings, the last argument must be NULL diff --git a/src/util/buf.h b/src/util/buf.h index 06d01ba..26a2f35 100644 --- a/src/util/buf.h +++ b/src/util/buf.h @@ -51,6 +51,7 @@ void virBufferStrcat(const virBufferPtr buf, ...) void virBufferEscapeString(const virBufferPtr buf, const char *format, const char *str); void virBufferEscapeSexpr(const virBufferPtr buf, const char *format, const char *str); void virBufferURIEncodeString (const virBufferPtr buf, const char *str); +void virBufferQuoteString(const virBufferPtr buf, const char *str); # define virBufferAddLit(buf_, literal_string_) \ virBufferAdd (buf_, "" literal_string_ "", sizeof literal_string_ - 1) -- 1.7.5.4

On Mon, Aug 01, 2011 at 02:28:59PM -0600, Eric Blake wrote: IMO we have too many fixes going in to 0.9.4 at the last minute and I think we ought to wait to 0.9.5 on this one. Dave

to quote the netcat command since it's passed to the shell. Adjust expected test case output accordingly. --- src/rpc/virnetsocket.c | 25 ++++++++++++++++++++----- tests/virnetsockettest.c | 10 +++++----- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index cba58c6..5ba9d96 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -607,7 +607,10 @@ int virNetSocketNewConnectSSH(const char *nodename, const char *path, virNetSocketPtr *retsock) { + char *quoted; virCommandPtr cmd; + virBuffer buf = VIR_BUFFER_INITIALIZER; + *retsock = NULL; cmd = virCommandNew(binary ? binary : "ssh"); @@ -630,6 +633,19 @@ int virNetSocketNewConnectSSH(const char *nodename, virCommandAddArgList(cmd, "-o", "StrictHostKeyChecking=no", NULL); virCommandAddArgList(cmd, nodename, "sh", "-c", NULL); + + virBufferQuoteString(&buf, netcat ? netcat : "nc"); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + quoted = virBufferContentAndReset(&buf); + if (quoted == NULL) { + virReportOOMError(); + return -1; + } + /* * This ugly thing is a shell script to detect availability of * the -q option for 'nc': debian and suse based distros need this @@ -641,14 +657,13 @@ int virNetSocketNewConnectSSH(const char *nodename, * behavior. */ virCommandAddArgFormat(cmd, - "'if %s -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then" + "'if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then" " ARG=-q0;" "fi;" - "%s $ARG -U %s'", - netcat ? netcat : "nc", - netcat ? netcat : "nc", - path); + "'%s' $ARG -U %s'", + quoted, quoted, path); + VIR_FREE(quoted); return virNetSocketNewConnectCommand(cmd, retsock); } diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index 3816b3c..00bee28 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -496,7 +496,7 @@ mymain(void) struct testSSHData sshData1 = { .nodename = "somehost", .path = "/tmp/socket", - .expectOut = "somehost sh -c 'if nc -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n", + .expectOut = "somehost sh -c 'if ''nc'' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;''nc'' $ARG -U /tmp/socket'\n", }; if (virtTestRun("SSH test 1", 1, testSocketSSH, &sshData1) < 0) ret = -1; @@ -509,7 +509,7 @@ mymain(void) .noTTY = true, .noVerify = false, .path = "/tmp/socket", - .expectOut = "-p 9000 -l fred -T -o BatchMode=yes -e none somehost sh -c 'if netcat -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;netcat $ARG -U /tmp/socket'\n", + .expectOut = "-p 9000 -l fred -T -o BatchMode=yes -e none somehost sh -c 'if ''netcat'' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;''netcat'' $ARG -U /tmp/socket'\n", }; if (virtTestRun("SSH test 2", 1, testSocketSSH, &sshData2) < 0) ret = -1; @@ -522,7 +522,7 @@ mymain(void) .noTTY = false, .noVerify = true, .path = "/tmp/socket", - .expectOut = "-p 9000 -l fred -o StrictHostKeyChecking=no somehost sh -c 'if netcat -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;netcat $ARG -U /tmp/socket'\n", + .expectOut = "-p 9000 -l fred -o StrictHostKeyChecking=no somehost sh -c 'if ''netcat'' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;''netcat'' $ARG -U /tmp/socket'\n", }; if (virtTestRun("SSH test 3", 1, testSocketSSH, &sshData3) < 0) ret = -1; @@ -538,7 +538,7 @@ mymain(void) struct testSSHData sshData5 = { .nodename = "crashyhost", .path = "/tmp/socket", - .expectOut = "crashyhost sh -c 'if nc -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n", + .expectOut = "crashyhost sh -c 'if ''nc'' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;''nc'' $ARG -U /tmp/socket'\n", .dieEarly = true, }; @@ -550,7 +550,7 @@ mymain(void) .path = "/tmp/socket", .keyfile = "/root/.ssh/example_key", .noVerify = true, - .expectOut = "-i /root/.ssh/example_key -o StrictHostKeyChecking=no example.com sh -c 'if nc -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n", + .expectOut = "-i /root/.ssh/example_key -o StrictHostKeyChecking=no example.com sh -c 'if ''nc'' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;''nc'' $ARG -U /tmp/socket'\n", }; if (virtTestRun("SSH test 6", 1, testSocketSSH, &sshData6) < 0) ret = -1; -- 1.7.5.4