[Libguestfs] Re: [libnbd PATCH] uri: Sanitize user-provided hostnames

Dan Berrangé ran a free trial of zeropath (http://zeropath.com/) AI analysis on libnbd, and it highlighted the following: "When using nbd+ssh:// URIs the library constructs an argv array for ssh from parsed URI parts (server, port, user, unix socket, nbd-port) and execs it. The server component is used directly as an ssh argument; if it begins with '-' an attacker can inject ssh options (e.g. -oProxyCommand=...) that cause ssh to run local commands. There is no protection (such as rejecting leading '-' in server or inserting a '--' to stop option parsing), so an attacker who can supply the URI can cause local command execution in the client process." eg with this.... "nbdinfo nbd+ssh://-oProxyCommand=rm%20run.in" you'll get a failure to start the NBD connection, but it none the less deletes the file 'run.in' in the local working directory The RFCs are vague enough that it is not immediately obvious whether there is any possibility of a valid hostname with a leading - (see https://www.netmeister.org/blog/hostnames.html). Still, it is better to pass the user's string on to ssh's determination of a valid hostname (which does appear to reject leading -) rather than trying to teach libnbd what patterns to allow, and thereby avoid risking any pattern written in libnbd accidentally being too restrictive. Do this by using "--" to end ssh options before the hostname, but that in turn must come after any use of -oUser=. With this in place, we now get a sane error rather than spawning a calculator with: $ nbdinfo nbd+ssh://-oProxyCommand=gnome-calculator hostname contains invalid characters /home/eblake/libnbd/info/.libs/nbdinfo: nbd_connect_uri: recv: server disconnected unexpectedly See also Libvirt commit e4cb8500 (Aug 2017), which in turn was inspired by GIT security flaws (http://blog.recurity-labs.com/2017-08-10/scm-vulns). We have put out a request to Red Hat security on whether this warrants a CVE in libnbd; however, as the problem was easy to identify using only free AI resources, and the problem itself is relatively low priority (to exploit it, an attacker has to convince an admin to run a program that will use libnbd on an untrusted URI), so we are publishing this now rather than waiting for any embargo. If a CVE is assigned, it will be announced to the mailing list in a followup post. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; CC: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- lib/uri.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/uri.c b/lib/uri.c index 3b099fe5..7413c434 100644 --- a/lib/uri.c +++ b/lib/uri.c @@ -291,8 +291,6 @@ uri_connect_ssh (struct nbd_handle *h, const xmlURIPtr uri, const_string_vector_append (&ssh_command, "-p"); const_string_vector_append (&ssh_command, port_str); - const_string_vector_append (&ssh_command, uri->server); - /* SSH user always comes from the authority part of the URI. */ if (uri->user) { /* Don't allow any special tokens in the username which ssh might @@ -310,6 +308,10 @@ uri_connect_ssh (struct nbd_handle *h, const xmlURIPtr uri, const_string_vector_append (&ssh_command, ssh_user); } + const_string_vector_append (&ssh_command, "--"); + const_string_vector_append (&ssh_command, uri->server); + + /* All further argc beginning with - are options to nc. */ const_string_vector_append (&ssh_command, "nc"); if (unixsocket) { const_string_vector_append (&ssh_command, "-U");