9:14 a.m.
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(a)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
10:09 a.m.
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(a)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
3:55 a.m.
New subject: [libvirt] [PATCH] tests: Fix virnetsockettest after SSH command line changes
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(a)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
2641
days inactive
2642
days old
2 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Martin Kletzander -
Michal Privoznik