Libguestfs - Nov 2019 - Re: [PATCH libnbd] api: Allow NBD URIs to be restricted.

Previous discussion:
https://www.redhat.com/archives/libguestfs/2019-August/msg00102.html

Last night I experimentally added support for URIs that contain the
query parameter tls-psk-file, as part of rewriting the tests to cover
more of the URI code.  So you can now have a URI like:

  nbds://alice@localhost/?tls-psk-file=keys.psk

However there's an obvious security problem here because now any
libnbd program which takes URIs from less trusted sources will open a
local file under the user's control.

So it's time to restrict what can appear in URIs.

I've added three new APIs for this purpose, see generator/generator in
this patch for documentation.  The defaults are fairly liberal, except
we do prevent opening local files (except socket) by default.

Rich.

New APIs are added which let you enable or disable features of NBD
URIs, mainly for security reasons.

tls-psk-file is *disabled* by default for obvious security reasons.
All other features are enabled by default.
---
 generator/generator | 127 +++++++++++++++++++++++++++++++++++++++++++-
 lib/handle.c        |  26 +++++++++
 lib/internal.h      |   5 ++
 lib/uri.c           |  31 +++++++++--
 tests/connect-uri.c |   2 +
 5 files changed, 186 insertions(+), 5 deletions(-)

diff --git a/generator/generator b/generator/generator
index 6cfe1fd..c2ff0db 100755
--- a/generator/generator
+++ b/generator/generator
@@ -991,7 +991,15 @@ let handshake_flags = {
     "NO_ZEROES",      1 lsl 1;
     ]
 }
-let all_flags = [ cmd_flags; handshake_flags ]
+let allow_transport_flags = {
+  flag_prefix = "ALLOW_TRANSPORT";
+  flags = [
+    "TCP",   1 lsl 0;
+    "UNIX",  1 lsl 1;
+    "VSOCK", 1 lsl 2;
+  ]
+}
+let all_flags = [ cmd_flags; handshake_flags; allow_transport_flags ]
 
 (* Calls.
  *
@@ -1445,6 +1453,75 @@ C<\"qemu:dirty-bitmap:...\"> for qemu-nbd
     see_also = ["L<nbd_block_status(3)>"];
   };
 
+  "set_uri_allow_transports", {
+    default_call with
+    args = [ Flags ("mask", allow_transport_flags) ]; ret = RErr;
+    permitted_states = [ Created ];
+    shortdesc = "set the allowed transports in NBD URIs";
+    longdesc = "\
+Set which transports are allowed to appear in NBD URIs.  The
+default is to allow any transports.
+
+The C<mask> parameter may contain any of the following flags
+ORed together:
+
+=over 4
+
+=item C<LIBNBD_ALLOW_TRANSPORT_TCP>
+
+=item C<LIBNBD_ALLOW_TRANSPORT_UNIX>
+
+=item C<LIBNBD_ALLOW_TRANSPORT_VSOCK>
+
+=back";
+    see_also = ["L<nbd_connect_uri(3)>";
"L<nbd_set_uri_allow_tls(3)>"];
+  };
+
+  "set_uri_allow_tls", {
+    default_call with
+    args = [ Enum ("tls", tls_enum) ]; ret = RErr;
+    permitted_states = [ Created ];
+    shortdesc = "set the allowed TLS settings in NBD URIs";
+    longdesc = "\
+Set which TLS settings are allowed to appear in NBD URIs.  The
+default is to allow either non-TLS or TLS URIs.
+
+The C<tls> parameter can be:
+
+=over 4
+
+=item C<LIBNBD_TLS_DISABLE>
+
+TLS URIs are not permitted, ie. a URI such as C<nbds://...>
+will be rejected.
+
+=item C<LIBNBD_TLS_ALLOW>
+
+This is the default.  TLS may be used or not, depending on
+whether the URI uses C<nbds> or C<nbd>.
+
+=item C<LIBNBD_TLS_REQUIRE>
+
+TLS URIs are required.  All URIs must use C<nbs>.
+
+=back";
+    see_also = ["L<nbd_connect_uri(3)>";
"L<nbd_set_uri_allow_transports(3)>"];
+  };
+
+  "set_uri_allow_local_file", {
+    default_call with
+    args = [ Bool "allow" ]; ret = RErr;
+    permitted_states = [ Created ];
+    shortdesc = "set the allowed transports in NBD URIs";
+    longdesc = "\
+Allow NBD URIs to reference local files.  This is I<disabled>
+by default.
+
+Currently this setting only controls whether the C<tls-psk-file>
+parameter in NBD URIs is allowed.";
+    see_also = ["L<nbd_connect_uri(3)>"];
+  };
+
   "connect_uri", {
     default_call with
     args = [ String "uri" ]; ret = RErr;
@@ -1539,7 +1616,50 @@ be present for the other transports.
 
 =item B<tls-psk-file=>F<PSKFILE>
 
-Set the PSK file.  See L<nbd_set_tls_psk_file(3)>.
+Set the PSK file.  See L<nbd_set_tls_psk_file(3)>.  Note
+this is not allowed by default - see next section.
+
+=back
+
+=head2 Disable URI features
+
+For security reasons you might want to disable certain URI
+features.  Pre-filtering URIs is error-prone and should not
+be attempted.  Instead use the libnbd APIs below to control
+what can appear in URIs.  Note you must call these functions
+on the same handle before calling C<nbd_connect_uri> or
+L<nbd_aio_connect_uri(3)>.
+
+=over 4
+
+=item TCP, Unix domain socket or C<AF_VSOCK> transports
+
+Default: all allowed
+
+To select which transports are allowed call
+L<nbd_set_uri_allow_transports(3)>.
+
+=item TLS
+
+Default: both non-TLS and TLS connections allowed
+
+To force TLS off or on in URIs call
+L<nbd_set_uri_allow_tls(3)>.
+
+=item Connect to Unix domain socket in the local filesystem
+
+Default: allowed
+
+To prevent this you must disable the C<+unix> transport
+using L<nbd_set_uri_allow_transports(3)>.
+
+=item Read from local files
+
+Default: denied
+
+To allow URIs to contain references to local files
+(eg. for parameters like C<tls-psk-file>) call
+L<nbd_set_uri_allow_local_file(3)>.
 
 =back
 
@@ -2900,6 +3020,9 @@ let first_version = [
   "aio_connect_socket", (1, 2);
   "connect_vsock", (1, 2);
   "aio_connect_vsock", (1, 2);
+  "set_uri_allow_transports", (1, 2);
+  "set_uri_allow_tls", (1, 2);
+  "set_uri_allow_local_file", (1, 2);
 
   (* These calls are proposed for a future version of libnbd, but
    * have not been added to any released version so far.
diff --git a/lib/handle.c b/lib/handle.c
index 1d4a527..cdbca86 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -64,6 +64,11 @@ nbd_create (void)
   h->unique = 1;
   h->tls_verify_peer = true;
   h->request_sr = true;
+
+  h->uri_allow_transports = (uint32_t) -1;
+  h->uri_allow_tls = LIBNBD_TLS_ALLOW;
+  h->uri_allow_local_file = false;
+
   h->gflags = (LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE |
                LIBNBD_HANDSHAKE_FLAG_NO_ZEROES);
 
@@ -360,3 +365,24 @@ nbd_unlocked_get_protocol (struct nbd_handle *h)
 
   return h->protocol;
 }
+
+int
+nbd_unlocked_set_uri_allow_transports (struct nbd_handle *h, uint32_t mask)
+{
+  h->uri_allow_transports = mask;
+  return 0;
+}
+
+int
+nbd_unlocked_set_uri_allow_tls (struct nbd_handle *h, int tls)
+{
+  h->uri_allow_tls = tls;
+  return 0;
+}
+
+int
+nbd_unlocked_set_uri_allow_local_file (struct nbd_handle *h, bool allow)
+{
+  h->uri_allow_local_file = allow;
+  return 0;
+}
diff --git a/lib/internal.h b/lib/internal.h
index 435cd36..50c0a9b 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -76,6 +76,11 @@ struct nbd_handle {
   bool request_sr;
   char **request_meta_contexts;
 
+  /* Allowed in URIs, see lib/uri.c. */
+  uint32_t uri_allow_transports;
+  int uri_allow_tls;
+  bool uri_allow_local_file;
+
   /* Global flags from the server. */
   uint16_t gflags;
 
diff --git a/lib/uri.c b/lib/uri.c
index b3dfe7d..704641c 100644
--- a/lib/uri.c
+++ b/lib/uri.c
@@ -216,6 +216,24 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const
char *raw_uri)
     goto cleanup;
   }
 
+  /* Check the transport is allowed. */
+  if ((transport == tcp &&
+       (h->uri_allow_transports & LIBNBD_ALLOW_TRANSPORT_TCP) == 0) ||
+      (transport == unix_sock &&
+       (h->uri_allow_transports & LIBNBD_ALLOW_TRANSPORT_UNIX) == 0) ||
+      (transport == vsock &&
+       (h->uri_allow_transports & LIBNBD_ALLOW_TRANSPORT_VSOCK) == 0)) {
+    set_error (EPERM, "URI transport %s is not permitted",
uri->scheme);
+    goto cleanup;
+  }
+
+  /* Check TLS is allowed. */
+  if ((tls && h->uri_allow_tls == LIBNBD_TLS_DISABLE) ||
+      (!tls && h->uri_allow_tls == LIBNBD_TLS_REQUIRE)) {
+    set_error (EPERM, "URI TLS setting %s is not permitted",
uri->scheme);
+    goto cleanup;
+  }
+
   /* Parse the socket parameter. */
   for (i = 0; i < nqueries; i++) {
     if (strcmp (queries[i].name, "socket") == 0)
@@ -239,9 +257,16 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const
char *raw_uri)
 
   /* Look for some tls-* parameters.  XXX More to come. */
   for (i = 0; i < nqueries; i++) {
-    if (strcmp (queries[i].name, "tls-psk-file") == 0 &&
-        nbd_unlocked_set_tls_psk_file (h, queries[i].value) == -1)
-      goto cleanup;
+    if (strcmp (queries[i].name, "tls-psk-file") == 0) {
+      if (! h->uri_allow_local_file) {
+        set_error (EPERM,
+                   "local file access (tls-psk-file) is not allowed,
"
+                   "call nbd_set_uri_allow_local_file to enable
this");
+        goto cleanup;
+      }
+      if (nbd_unlocked_set_tls_psk_file (h, queries[i].value) == -1)
+        goto cleanup;
+    }
   }
 
   /* Username. */
diff --git a/tests/connect-uri.c b/tests/connect-uri.c
index 9eb34d1..6e7d168 100644
--- a/tests/connect-uri.c
+++ b/tests/connect-uri.c
@@ -73,6 +73,8 @@ main (int argc, char *argv[])
     exit (77);
   }
 
+  nbd_set_uri_allow_local_file (nbd, true);
+
   if (nbd_connect_uri (nbd, URI) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
-- 
2.23.0

On 10/20/19 6:06 AM, Richard W.M. Jones wrote:
> New APIs are added which let you enable or disable features of NBD
> URIs, mainly for security reasons.
> 
> tls-psk-file is *disabled* by default for obvious security reasons.
> All other features are enabled by default.
> ---
> @@ -1445,6 +1453,75 @@ C<\"qemu:dirty-bitmap:...\"> for
qemu-nbd
>       see_also = ["L<nbd_block_status(3)>"];
>     };
>   
> +  "set_uri_allow_transports", {
> +    default_call with
> +    args = [ Flags ("mask", allow_transport_flags) ]; ret =
RErr;
> +    permitted_states = [ Created ];
> +    shortdesc = "set the allowed transports in NBD URIs";
> +    longdesc = "\
> +Set which transports are allowed to appear in NBD URIs.  The
> +default is to allow any transports.
'any transport.'
> +
> +The C<mask> parameter may contain any of the following flags
> +ORed together:
> +
> +=over 4
> +
> +=item C<LIBNBD_ALLOW_TRANSPORT_TCP>
> +
> +=item C<LIBNBD_ALLOW_TRANSPORT_UNIX>
> +
> +=item C<LIBNBD_ALLOW_TRANSPORT_VSOCK>
> +
> +=back";
> +    see_also = ["L<nbd_connect_uri(3)>";
"L<nbd_set_uri_allow_tls(3)>"];
> +  };
Worth L<nbd_get_uri_allow_tls(3)> to query the current permitted 
transports? Similar for other new set_ APIs.
> +
> +  "set_uri_allow_tls", {
> +    default_call with
> +    args = [ Enum ("tls", tls_enum) ]; ret = RErr;
> +    permitted_states = [ Created ];
> +    shortdesc = "set the allowed TLS settings in NBD URIs";
> +    longdesc = "\
> +Set which TLS settings are allowed to appear in NBD URIs.  The
> +default is to allow either non-TLS or TLS URIs.
> +
> +The C<tls> parameter can be:
> +
> +=over 4
> +
> +=item C<LIBNBD_TLS_DISABLE>
> +
> +TLS URIs are not permitted, ie. a URI such as C<nbds://...>
> +will be rejected.
> +
> +=item C<LIBNBD_TLS_ALLOW>
> +
> +This is the default.  TLS may be used or not, depending on
> +whether the URI uses C<nbds> or C<nbd>.
> +
> +=item C<LIBNBD_TLS_REQUIRE>
> +
> +TLS URIs are required.  All URIs must use C<nbs>.
C<nbds>

> +=item Connect to Unix domain socket in the local filesystem
> +
> +Default: allowed
> +
> +To prevent this you must disable the C<+unix> transport
Is the + in C<+unix> intentional?

Otherwise looks like a good addition.

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