[libvirt] [PATCH] rpc: avoid ssh interpreting malicious hostname as arguments
Inspired by the recent GIT / Mercurial security flaws (http://blog.recurity-labs.com/2017-08-10/scm-vulns), consider someone/something manages to feed libvirt a bogus URI such as: virsh -c qemu+ssh://-oProxyCommand=gnome-calculator/system In this case, the hosname "-oProxyCommand=gnome-calculator" will get interpreted as an argument to ssh, not a hostname. Fortunately, due to the set of args we have following the hostname, SSH will then interpret our bit of shell script that runs 'nc' on the remote host as a cipher name, which is clearly invalid. This makes ssh exit during argv parsing and so it never tries to run gnome-calculator. We are lucky this time, but lets be more paranoid, by using '--' to explicitly tell SSH when it has finished seeing command line options. This forces it to interpret "-oProxyCommand=gnome-calculator" as a hostname, and thus see a fail from hostname lookup. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/rpc/virnetsocket.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index d228c8a8c..23089afef 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -868,7 +868,7 @@ int virNetSocketNewConnectSSH(const char *nodename, if (!netcat) netcat = "nc"; - virCommandAddArgList(cmd, nodename, "sh", "-c", NULL); + virCommandAddArgList(cmd, "--", nodename, "sh", "-c", NULL); virBufferEscapeShell(&buf, netcat); if (virBufferCheckError(&buf) < 0) { -- 2.13.5
On 08/29/2017 04:14 PM, Daniel P. Berrange wrote:
Inspired by the recent GIT / Mercurial security flaws (http://blog.recurity-labs.com/2017-08-10/scm-vulns), consider someone/something manages to feed libvirt a bogus URI such as:
Yeah, I was wondering this myself when reading the news on my PTO but you beat me to it :-)
virsh -c qemu+ssh://-oProxyCommand=gnome-calculator/system
In this case, the hosname "-oProxyCommand=gnome-calculator" will get interpreted as an argument to ssh, not a hostname. Fortunately, due to the set of args we have following the hostname, SSH will then interpret our bit of shell script that runs 'nc' on the remote host as a cipher name, which is clearly invalid. This makes ssh exit during argv parsing and so it never tries to run gnome-calculator.
We are lucky this time, but lets be more paranoid, by using '--' to explicitly tell SSH when it has finished seeing command line options. This forces it to interpret "-oProxyCommand=gnome-calculator" as a hostname, and thus see a fail from hostname lookup.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/rpc/virnetsocket.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index d228c8a8c..23089afef 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -868,7 +868,7 @@ int virNetSocketNewConnectSSH(const char *nodename, if (!netcat) netcat = "nc";
- virCommandAddArgList(cmd, nodename, "sh", "-c", NULL); + virCommandAddArgList(cmd, "--", nodename, "sh", "-c", NULL);
virBufferEscapeShell(&buf, netcat); if (virBufferCheckError(&buf) < 0) {
ACK and safe for the freeze. Michal
Commit e4cb8500810a changed the way ssh command line is created by adding '--' before the hostname in order to fix a potential security flaw. However it failed to modify the tests, so let's do that. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- Pushed under the build-breaker rule. tests/virnetsockettest.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index c36886137a03..9f9a24348449 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -491,7 +491,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 " + .expectOut = "-- somehost sh -c 'if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " "ARG=-q0;" "else " "ARG=;" @@ -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 '" + .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;" "else " @@ -528,7 +528,7 @@ mymain(void) .noTTY = false, .noVerify = true, .path = "/tmp/socket", - .expectOut = "-p 9000 -l fred -o StrictHostKeyChecking=no somehost sh -c '" + .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;" "else " @@ -550,7 +550,7 @@ mymain(void) struct testSSHData sshData5 = { .nodename = "crashyhost", .path = "/tmp/socket", - .expectOut = "crashyhost sh -c " + .expectOut = "-- crashyhost sh -c " "'if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " "ARG=-q0;" "else " @@ -567,7 +567,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 '" + .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;" "else " @@ -582,7 +582,7 @@ mymain(void) .nodename = "somehost", .netcat = "nc -4", .path = "/tmp/socket", - .expectOut = "somehost sh -c 'if ''nc -4'' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + .expectOut = "-- somehost sh -c 'if ''nc -4'' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " "ARG=-q0;" "else " "ARG=;" -- 2.14.1
participants (3)
-
Daniel P. Berrange -
Martin Kletzander -
Michal Privoznik