Consider having a nc binary in the path with a space in its name,
for example '/tmp/fo o/nc'

This results in libvirt running SSH with the following arg value

  "'if ''/tmp/fo o/nc'' -q 2>&1 | grep \"requires
    an argument\" >/dev/null 2>&1; then ARG=-q0;
    else ARG=;fi;''/tmp/fo o/nc'' $ARG -U
    /var/run/libvirt/libvirt-sock'"

The use of the single quote escaping was introduced by

  commit 6ac6238de33fc74e7545b245ae273d1bfd658808
  Author: Guido Günther <agx@sigxcpu.org>
  Date:   Thu Oct 13 21:49:01 2011 +0200

    Use virBufferEscapeShell in virNetSocketNewConnectSSH

    to escape the netcat command since it's passed to the shell. Adjust
    expected test case output accordingly.

While the intention of this change was good, the result is broken as it
is still underquoted.

On the SSH server side, SSH itself runs the command via the shell.
Our command is then invoking the shell again. Thus we see

$ virsh -c qemu+ssh://root@domokun/system?netcat=%2Ftmp%2Ffo%20o%2Fnc list
error: failed to connect to the hypervisor
error: End of file while reading data: sh: /tmp/fo: No such file or directory: Input/output error

With the second level of escaping added we can now successfully use a nc
binary with a space in the path.

The original test case added was misleading as it illustrated using a
binary path of 'nc -4' which is not a path, it is a command with a
separate argument, which is getting interpreted as a path.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/rpc/virnetclient.c   | 8 ++++++++
 src/rpc/virnetsocket.c   | 9 +++++++++
 tests/virnetsockettest.c | 6 +++---
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 64855fb8d6..53d8b219ea 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -490,6 +490,10 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
     DEFAULT_VALUE(knownHostsVerify, "normal");
 
     virBufferEscapeShell(&buf, netcatPath);
+    if (!(nc = virBufferContentAndReset(&buf)))
+        goto no_memory;
+    virBufferEscapeShell(&buf, nc);
+    VIR_FREE(nc);
     if (!(nc = virBufferContentAndReset(&buf)))
         goto no_memory;
 
@@ -596,6 +600,10 @@ virNetClientPtr virNetClientNewLibssh(const char *host,
     DEFAULT_VALUE(knownHostsVerify, "normal");
 
     virBufferEscapeShell(&buf, netcatPath);
+    if (!(nc = virBufferContentAndReset(&buf)))
+        goto no_memory;
+    virBufferEscapeShell(&buf, nc);
+    VIR_FREE(nc);
     if (!(nc = virBufferContentAndReset(&buf)))
         goto no_memory;
 
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index ebd304707a..a469907779 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -903,6 +903,15 @@ int virNetSocketNewConnectSSH(const char *nodename,
         return -1;
     }
     quoted = virBufferContentAndReset(&buf);
+
+    virBufferEscapeShell(&buf, quoted);
+    VIR_FREE(quoted);
+    if (virBufferCheckError(&buf) < 0) {
+        virCommandFree(cmd);
+        return -1;
+    }
+    quoted = virBufferContentAndReset(&buf);
+
     /*
      * This ugly thing is a shell script to detect availability of
      * the -q option for 'nc': debian and suse based distros need this
diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c
index bb8357f7cd..8cad351605 100644
--- a/tests/virnetsockettest.c
+++ b/tests/virnetsockettest.c
@@ -661,15 +661,15 @@ mymain(void)
 
     struct testSSHData sshData7 = {
         .nodename = "somehost",
-        .netcat = "nc -4",
+        .netcat = "/tmp/fo o/nc",
         .path = "/tmp/socket",
         .expectOut = "-T -e none -- somehost sh -c '"
-                     "if ''nc -4'' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
+                     "if \'''\\''/tmp/fo o/nc'\\'''' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
                          "ARG=-q0;"
                      "else "
                          "ARG=;"
                      "fi;"
-                     "''nc -4'' $ARG -U /tmp/socket'\n",
+                     "'''\\''/tmp/fo o/nc'\\'''' $ARG -U /tmp/socket'\n",
     };
     if (virTestRun("SSH test 7", testSocketSSH, &sshData7) < 0)
         ret = -1;
-- 
2.21.0

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

On 9/30/19 7:34 AM, Daniel P. Berrangé wrote:
> Consider having a nc binary in the path with a space in its name,
> for example '/tmp/fo o/nc'
> 
> This results in libvirt running SSH with the following arg value
> 
>    "'if ''/tmp/fo o/nc'' -q 2>&1 | grep \"requires
>      an argument\" >/dev/null 2>&1; then ARG=-q0;
>      else ARG=;fi;''/tmp/fo o/nc'' $ARG -U
>      /var/run/libvirt/libvirt-sock'"
> 

> With the second level of escaping added we can now successfully use a nc
> binary with a space in the path.
> 
> The original test case added was misleading as it illustrated using a
> binary path of 'nc -4' which is not a path, it is a command with a
> separate argument, which is getting interpreted as a path.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   src/rpc/virnetclient.c   | 8 ++++++++
>   src/rpc/virnetsocket.c   | 9 +++++++++
>   tests/virnetsockettest.c | 6 +++---
>   3 files changed, 20 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

[We had an IRC chat about whether this was a possible CVE - the answer 
was no - although the bug can be exploited with "/path/to/';date" as a 
way to get the remote machine to execute date, it's not an elevation of 
privilege, because if you already have ssh access to the remote machine, 
you don't need libvirt misquoting the nc binary's name to make ssh do 
what you want.]

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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