[Libguestfs] [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>
CC: Daniel P. Berrangé <berrange(a)redhat.com>
---
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");
--
2.51.0