Libguestfs - Jun 2019 - Re: [libnbd PATCH 0/2] Tighten URI parser

I'm not sure whether we want to go with just the first patch (reject
nbd:unix:/path but still accept nbd:/path), or squash the two in order
to go with the second (reject both abbreviated forms, and require
scheme://...).  Either way, though, nbdkit -U - --run '$nbd' will now
error out rather than inadvertently connect over TCP to
localhost:10809 instead of the intended Unix connection (and in the
meantime, you want to use --run '$unixsocket', or maybe we should
teach nbdkit to support --run '$uri').

Eric Blake (2):
  uri: Reject nbd:unix:/path/to/socket as invalid URI
  uri: Reject nbd:unix:/path/to/socket as invalid URI

 lib/connect.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

-- 
2.20.1

libxml2 parses it as valid per RFC 3986, as the nbd: scheme with no
authority and a relative path. This string has been used with qemu to
request a Unix socket, such that nbdkit --run produces it for $nbd
(compared to $unixsocket), but accepting it as a URI means that we
instead try to connect to a TCP socket with a default authority
(localhost, port 10809), and nbdkit ignores the path element
'unix:/path/to/socket' and you end up with a connection (or an
attempt) to a completely different server.

All scheme://authority forms have a path that is either empty or
begins with '/'.  This does not reject 'nbd:/path/to/socket'
(with no
authority but an absolute path), but at least that form has not been
in use by qemu.

Reported-by: Martin Kletzander <mkletzan@redhat.com>
---
 lib/connect.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/connect.c b/lib/connect.c
index 96ed1ca..8b19e1e 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -291,12 +291,14 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const
char *raw_uri)
    * nbd_unlocked_set_tls_* to match...
    */

-  /* Export name. */
+  /* Export name. Insist on the scheme://[authority][/absname] form. */
   if (uri->path) {
     if (uri->path[0] == '/')
       r = nbd_unlocked_set_export_name (h, &uri->path[1]);
-    else
-      r = nbd_unlocked_set_export_name (h, uri->path);
+    else {
+      set_error (EINVAL, "URI must begin with '%s://'",
uri->scheme);
+      goto cleanup;
+    }
   }
   else
     r = nbd_unlocked_set_export_name (h, "");
-- 
2.20.1

libxml2 parses it as valid per RFC 3986, as the nbd: scheme with no
authority and a relative path. This string has been used with qemu to
request a Unix socket, such that nbdkit --run produces it for $nbd
(compared to $unixsocket), but accepting it as a URI means that we
instead try to connect to a TCP socket with a default authority
(localhost, port 10809), and nbdkit ignores the path element
'unix:/path/to/socket' and you end up with a connection (or an
attempt) to a completely different server.

It's easiest to just reject all URIs that do not use the
scheme://authority form, at which point any path element is either
empty or begins with '/'.  This rejects
'nbd:unix:/path/to/socket'
which nbdkit produces, and also rejects 'nbd:/path/to/socket' (with no
authority but an absolute path) even though that form is less likely
to appear in the wild as qemu did not use it.

Reported-by: Martin Kletzander <mkletzan@redhat.com>
---
 lib/connect.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/lib/connect.c b/lib/connect.c
index 8b19e1e..e9d76ff 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -28,6 +28,7 @@
 #include <netinet/tcp.h>
 #include <sys/types.h>
 #include <sys/socket.h>
+#include <assert.h>

 #ifdef HAVE_LIBXML2
 #include <libxml/uri.h>
@@ -275,6 +276,12 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const
char *raw_uri)
     goto cleanup;
   }

+  /* Insist on the scheme://[authority][/absname] form. */
+  if (strncmp (raw_uri + strlen (uri->scheme), "://", 3)) {
+    set_error (EINVAL, "URI must begin with '%s://'",
uri->scheme);
+    goto cleanup;
+  }
+
   for (i = 0; i < nqueries; i++) {
     if (strcmp (queries[i].name, "socket") == 0)
       unixsocket = queries[i].value;
@@ -291,14 +298,11 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const
char *raw_uri)
    * nbd_unlocked_set_tls_* to match...
    */

-  /* Export name. Insist on the scheme://[authority][/absname] form. */
+  /* Export name. */
   if (uri->path) {
-    if (uri->path[0] == '/')
-      r = nbd_unlocked_set_export_name (h, &uri->path[1]);
-    else {
-      set_error (EINVAL, "URI must begin with '%s://'",
uri->scheme);
-      goto cleanup;
-    }
+    /* Since we require scheme://authority above, any path is absolute */
+    assert (uri->path[0] == '/');
+    r = nbd_unlocked_set_export_name (h, &uri->path[1]);
   }
   else
     r = nbd_unlocked_set_export_name (h, "");
-- 
2.20.1

On Tue, Jun 25, 2019 at 09:09:58PM -0500, Eric Blake
wrote:
>I'm not sure whether we want to go with just the first patch (reject
>nbd:unix:/path but still accept nbd:/path), or squash the two in order
>to go with the second (reject both abbreviated forms, and require
>scheme://...).  Either way, though, nbdkit -U - --run '$nbd' will
now
>error out rather than inadvertently connect over TCP to
>localhost:10809 instead of the intended Unix connection (and in the
>meantime, you want to use --run '$unixsocket', or maybe we should
>teach nbdkit to support --run '$uri').
>
Oh, I get it now. So "nbd:unix:/path" is a valid URL without authority
where the
path is "unix:/path".  Well, TIL =)
It is nicely visible here:

  https://tools.ietf.org/html/rfc3986#section-3

These patches are fine from my standpoint, although I must say thinking about
the above as defined in the RFC makes me re-evaluate my feelings about it.
Having URI without authority just use a local file (socket) makes sense now.
Even having the unix: prefix to show that it is a UNIX socket seems sensible.
At least unless you realize that having an actual URI it makes sense to have
that as a part of the scheme, as is customary.

What I am trying to say is that maybe just the first patch is enough.  When
people want to just write, for example nbd:/tmp/my_nbd.sock by hand, it is
easier than typing the whole nbd+unix:?socket=/tmp/my_nbd.sock.  On the other
hand, how often do you need to write it manually...

Anyway, I don't really have strong feelings about this, so whatever you
choose
is fine as long as nbd:unix:/path does not try connecting to localhost over
network =)
>Eric Blake (2):
>  uri: Reject nbd:unix:/path/to/socket as invalid URI
>  uri: Reject nbd:unix:/path/to/socket as invalid URI
The second one should say "Reject nbd:/path/to/socket as invalid URL",
I guess.


Thank you.
>
> lib/connect.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
>-- 
>2.20.1
>
>_______________________________________________
>Libguestfs mailing list
>Libguestfs@redhat.com
>https://www.redhat.com/mailman/listinfo/libguestfs

On 6/26/19 4:05 AM, Martin Kletzander wrote:
> On Tue, Jun 25, 2019 at 09:09:58PM -0500, Eric Blake wrote:
>> I'm not sure whether we want to go with just the first patch
(reject
>> nbd:unix:/path but still accept nbd:/path), or squash the two in order
>> to go with the second (reject both abbreviated forms, and require
>> scheme://...).  Either way, though, nbdkit -U - --run '$nbd'
will now
>> error out rather than inadvertently connect over TCP to
>> localhost:10809 instead of the intended Unix connection (and in the
>> meantime, you want to use --run '$unixsocket', or maybe we
should
>> teach nbdkit to support --run '$uri').
>>
> 
> Oh, I get it now. So "nbd:unix:/path" is a valid URL without
authority
> where the
> path is "unix:/path".  Well, TIL =)
> It is nicely visible here:
> 
>  https://tools.ietf.org/html/rfc3986#section-3
> 
> These patches are fine from my standpoint, although I must say thinking
> about
> the above as defined in the RFC makes me re-evaluate my feelings about it.
> Having URI without authority just use a local file (socket) makes sense
> now.
Except that our URI proposal is stating that we want:

nbd+unix:///exportname?socket=/path/to/sock

Using a mere nbd+unix:/path/to/sock would be wrong, as it would treat
'/path/to/sock' as the export name, not the socket name.
> Even having the unix: prefix to show that it is a UNIX socket seems
> sensible.
> At least unless you realize that having an actual URI it makes sense to
> have
> that as a part of the scheme, as is customary.
> 
> What I am trying to say is that maybe just the first patch is enough.  When
> people want to just write, for example nbd:/tmp/my_nbd.sock by hand, it is
> easier than typing the whole nbd+unix:?socket=/tmp/my_nbd.sock.  On the
> other
> hand, how often do you need to write it manually...
My worry is that nbd:/path/to/sock is wrong for use as a Unix socket
connection, because it leaves no place for the export name (even if the
export name is '').  It is too confusing if 'nbd:export'
sometimes
treats the path part as the export name, and sometimes as the socket
name, so I'd rather that we ALWAYS treat the path part as the export name.
> 
> Anyway, I don't really have strong feelings about this, so whatever you
> choose
> is fine as long as nbd:unix:/path does not try connecting to localhost over
> network =)
> 
>> Eric Blake (2):
>>  uri: Reject nbd:unix:/path/to/socket as invalid URI
>>  uri: Reject nbd:unix:/path/to/socket as invalid URI
> 
> The second one should say "Reject nbd:/path/to/socket as invalid
URL", I
> guess.
My actual plan is to squash it into a single patch; I just posted as two
patches in case we only wanted to go halfway (the first patch). But
given this conversation, you've convinced me that it is more confusing
to allow anything not of the form nbd://, even if it is valid as a URI.
(That is, we are explicitly declaring that when using the nbd: or
nbd+unix: scheme, we have additional restrictions that make only a
subset of valid URIs usable in our scheme).

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

I think this series is fine.  I have no particular preference whether
you want to go with just the first, or squash both patches together.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org