On 07/27/2011 01:32 PM, Guido Günther wrote:

On Wed, Jul 27, 2011 at 10:53:01AM -0600, Eric Blake wrote:

...

[adding libvir-list]

On 07/27/2011 10:28 AM, Whit Blauvelt wrote:

...

Looks like my real problem may be not incorporating a Debian/Ubuntu patch before building 0.9.x, since netcat differs:

https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/517478

Reading that bug report, it looks like upstream libvirt.git should be patched to add a runtime test on whether a remote nc supports the -q option, rather than blindly assuming that 'nc -q' exists on the remote side. This is the patch we're currently using in Debian for that (based on a version originally forwarded from Ubuntu):

http://anonscm.debian.org/gitweb/?p=pkg-libvirt/libvirt.git;a=blob;f=debian/...

I'd be happy to push that since this is the distro specific patch I have to adjust the most ;)

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?

+++ b/src/rpc/virnetsocket.c

22 @@ -596,9 +596,30 @@ int virNetSocketNewConnectSSH(const char *nodename,

23 if (noTTY)

24 virCommandAddArgList(cmd, "-T", "-o", "BatchMode=yes",

25 "-e", "none", NULL);

26 - virCommandAddArgList(cmd, nodename,

27 - netcat ? netcat : "nc",

28 - "-U", path, NULL);

29 +

30 + virCommandAddArgList(cmd, nodename, "sh -c", NULL);

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).

31 + /*

32 + * This ugly thing is a shell script to detect availability of

33 + * the -q option for 'nc': debian and suse based distros need this

34 + * flag to ensure the remote nc will exit on EOF, so it will go away

35 + * when we close the VNC tunnel. If it doesn't go away, subsequent

36 + * VNC connection attempts will hang.

s/VNC tunnel/connection tunnel/ s/VNC connection attempts/connection attempts/ This code is not a VNC tunnel.

37 + *

38 + * Fedora's 'nc' doesn't have this option, and apparently defaults

39 + * to the desired behavior.

40 + */

41 + virCommandAddArgFormat(cmd, "'%s -q 2>&1 | grep -q \"requires an argument\";"

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.

42 + "if [ $? -eq 0 ] ; then"

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"

43 + " CMD=\"%s -q 0 -U %s\";"

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.

44 + "else"

45 + " CMD=\"%s -U %s\";"

46 + "fi;"

47 + "eval \"$CMD\";'",

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@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:

On 07/29/2011 03:12 AM, Guido Günther wrote:

...

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;

I'd prefer that we had the term Shell somewhere in the name, and perhaps keep it similar to other escaping functions. How about:

virBufferEscapeShell;

Also, virsh.c could use this new function in its implementation of 'virsh echo --shell'.

...

/** + * 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;

I'd _really_ like to make this smart enough to omit outer '' if the entire string does not need quoting (again, see how virsh cmdEcho does it).

...

+ + while (*p) { + /* Replace literal ' with a close ', a \', and a open ' */ + if (*p == '\'') + virBufferAddLit(buf, "'\\''"); + else + virBufferAddChar(buf, *p);

That's a bit slow compared to the other virBufferEscape* methods which pre-allocate based on worst case, then do things directly into the output buffer rather than making a function call for every byte.

Overall, the idea is nice, and I also imagine using it in virCommandToString to output unambiguous log messages for commands about to be run, but it's not quite ready yet.

I'm torn on whether to touch this up and include it in 0.9.4 (since it is turning into a frequently reported issue), or defer it to post-release since it is starting to grow in scope.

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