[libvirt PATCH v2 4/6] virnetclient: Don't unnecessarily quote user-provided values

If the output of virNetClientDoubleEscapeShell() matches its input, then no escaping actually happened and quoting the value in the generated script is unnecessary. With this change, awkward use of quotes such as sh -c 'if 'nc' -q' is completely gone when using the default settings. Closes: https://gitlab.com/libvirt/libvirt/-/issues/273 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com&gt; --- src/rpc/virnetclient.c | 36 +++++++++++++++++++++++++++--------- tests/virnetsockettest.c | 28 ++++++++++++++-------------- 2 files changed, 41 insertions(+), 23 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 9c7047c7f8..cd92af1669 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -420,10 +420,12 @@ virNetClientSSHHelperCommand(virNetClientProxy proxy, const char *driverURI, bool readonly) { - g_autofree char *netcatPathSafe = virNetClientDoubleEscapeShell(netcatPath ? netcatPath : "nc"); - g_autofree char *driverURISafe = virNetClientDoubleEscapeShell(driverURI); + g_autofree char *netcatPathSafe = NULL; + g_autofree char *driverURISafe = NULL; g_autofree char *nccmd = NULL; g_autofree char *helpercmd = NULL; + const char *netcatPathQuotes = ""; + const char *driverURIQuotes = ""; if (netcatPath) { if (proxy == VIR_NET_CLIENT_PROXY_AUTO) { @@ -436,20 +438,36 @@ virNetClientSSHHelperCommand(virNetClientProxy proxy, _("netcat path not valid with native proxy mode")); return NULL; } + } else { + netcatPath = "nc"; + } + + /* Escape user-provided values so that they're safe for use as part + * of our generated shell snippet. If escaping was necessary, we + * will also need to add quotes around all uses of each value */ + netcatPathSafe = virNetClientDoubleEscapeShell(netcatPath); + if (STRNEQ(netcatPathSafe, netcatPath)) { + netcatPathQuotes = "'"; + } + driverURISafe = virNetClientDoubleEscapeShell(driverURI); + if (STRNEQ(driverURISafe, driverURI)) { + driverURIQuotes = "'"; } nccmd = g_strdup_printf( - "if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "if %s%s%s -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " "ARG=-q0; " "else " "ARG=; " "fi; " - "'%s' $ARG -U %s", - netcatPathSafe, netcatPathSafe, socketPath); - - helpercmd = g_strdup_printf("virt-ssh-helper%s'%s'", - readonly ? " -r " : " ", - driverURISafe); + "%s%s%s $ARG -U %s", + netcatPathQuotes, netcatPathSafe, netcatPathQuotes, + netcatPathQuotes, netcatPathSafe, netcatPathQuotes, + socketPath); + + helpercmd = g_strdup_printf("virt-ssh-helper%s %s%s%s", + readonly ? " -r" : "", + driverURIQuotes, driverURISafe, driverURIQuotes); switch (proxy) { case VIR_NET_CLIENT_PROXY_AUTO: diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index adff6a0f9e..09c3ba13ad 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -571,12 +571,12 @@ mymain(void) .path = "/tmp/socket", .netcat = "nc", .expectOut = "-T -e none -- somehost sh -c '" - "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "if nc -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " "ARG=-q0; " "else " "ARG=; " "fi; " - "'nc' $ARG -U /tmp/socket" + "nc $ARG -U /tmp/socket" "'\n", }; if (virTestRun("SSH test 1", testSocketSSH, &sshData1) < 0) @@ -591,12 +591,12 @@ mymain(void) .noVerify = false, .path = "/tmp/socket", .expectOut = "-p 9000 -l fred -T -e none -o BatchMode=yes -- somehost sh -c '" - "if 'netcat' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "if netcat -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " "ARG=-q0; " "else " "ARG=; " "fi; " - "'netcat' $ARG -U /tmp/socket" + "netcat $ARG -U /tmp/socket" "'\n", }; if (virTestRun("SSH test 2", testSocketSSH, &sshData2) < 0) @@ -611,12 +611,12 @@ mymain(void) .noVerify = true, .path = "/tmp/socket", .expectOut = "-p 9000 -l fred -T -e none -o StrictHostKeyChecking=no -- somehost sh -c '" - "if 'netcat' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "if netcat -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " "ARG=-q0; " "else " "ARG=; " "fi; " - "'netcat' $ARG -U /tmp/socket" + "netcat $ARG -U /tmp/socket" "'\n", }; if (virTestRun("SSH test 3", testSocketSSH, &sshData3) < 0) @@ -635,12 +635,12 @@ mymain(void) .path = "/tmp/socket", .netcat = "nc", .expectOut = "-T -e none -- crashyhost sh -c '" - "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "if nc -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " "ARG=-q0; " "else " "ARG=; " "fi; " - "'nc' $ARG -U /tmp/socket" + "nc $ARG -U /tmp/socket" "'\n", .dieEarly = true, }; @@ -654,12 +654,12 @@ mymain(void) .keyfile = "/root/.ssh/example_key", .noVerify = true, .expectOut = "-i /root/.ssh/example_key -T -e none -o StrictHostKeyChecking=no -- example.com sh -c '" - "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "if nc -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " "ARG=-q0; " "else " "ARG=; " "fi; " - "'nc' $ARG -U /tmp/socket" + "nc $ARG -U /tmp/socket" "'\n", }; if (virTestRun("SSH test 6", testSocketSSH, &sshData6) < 0) @@ -718,14 +718,14 @@ mymain(void) .path = "/tmp/socket", .expectOut = "-T -e none -- somehost sh -c '" "if which virt-ssh-helper >/dev/null 2>&1; then " - "virt-ssh-helper -r 'qemu:///session'; " + "virt-ssh-helper -r qemu:///session; " "else " - "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "if nc -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " "ARG=-q0; " "else " "ARG=; " "fi; " - "'nc' $ARG -U /tmp/socket; " + "nc $ARG -U /tmp/socket; " "fi" "'\n" }; @@ -736,7 +736,7 @@ mymain(void) .nodename = "somehost", .proxy = VIR_NET_CLIENT_PROXY_NATIVE, .expectOut = "-T -e none -- somehost sh -c '" - "virt-ssh-helper -r 'qemu:///session'" + "virt-ssh-helper -r qemu:///session" "'\n" }; if (virTestRun("SSH test 11", testSocketSSH, &sshData11) < 0) -- 2.35.1