1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] rpc: avoid ssh interpreting malicious hostname as arguments

Daniel P. Berrange posted 1 patch 7 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20170829141459.28606-1-berrange@redhat.com
src/rpc/virnetsocket.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[libvirt] [PATCH] rpc: avoid ssh interpreting malicious hostname as arguments
Posted by Daniel P. Berrange 7 years, 8 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: avoid ssh interpreting malicious hostname as arguments
Posted by Michal Privoznik 7 years, 8 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] tests: Fix virnetsockettest after SSH command line changes
Posted by Martin Kletzander 7 years, 8 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.