[Libguestfs] [nbdkit PATCH 3/4] server: Forbid NUL in export and context names

[Eric Blake]

A client that requests export or metacontext "a\0b" is not
spec-compliant (NBD strings must be UTF-8 with no embedded NUL).
What's more, our new nbdkit_export_name() and reflection plugin means
that a plugin serving content specific to export "a" may be exposing
something different from whatever the client was attempting to see.
Rather than changing our interface to allow embedded NUL, it's better
to just reject such a client as buggy.  This also means checking the
export name passed to NBD_OPT_SET_META_CONTEXT, rather than completely
ignoring it (although the next patch will further tweak that for a
different spec compliance reason).

It also didn't help that we were inconsistent between malloc/strcpy
vs. malloc/memcpy/set NUL.

I don't know of any existing NBD client that makes it easy to send
such an invalid export name, nor do I see any reason to hack libnbd
into providing that non-compliant behavior.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 server/protocol-handshake-newstyle.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c
index 38978c67..785944eb 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -277,12 +277,15 @@ negotiate_handshake_newstyle_options (struct connection *conn)
       debug ("newstyle negotiation: %s: client requested export '%s'",
              name_of_nbd_opt (option), data);
       free (conn->exportname);
-      conn->exportname = malloc (optlen+1);
+      conn->exportname = strndup (data, optlen);
       if (conn->exportname == NULL) {
-        nbdkit_error ("malloc: %m");
+        nbdkit_error ("strndup: %m");
+        return -1;
+      }
+      if (strlen (conn->exportname) != optlen) {
+        nbdkit_error ("export name must not contain NUL bytes");
         return -1;
       }
-      strcpy (conn->exportname, data);

       /* We have to finish the handshake by sending handshake_finish. */
       if (finish_newstyle_options (conn, &exportsize) == -1)
@@ -420,13 +423,15 @@ negotiate_handshake_newstyle_options (struct connection *conn)
          * save it in the connection.
          */
         free (conn->exportname);
-        conn->exportname = malloc (exportnamelen+1);
+        conn->exportname = strndup (&data[4], exportnamelen);
         if (conn->exportname == NULL) {
           nbdkit_error ("malloc: %m");
           return -1;
         }
-        memcpy (conn->exportname, &data[4], exportnamelen);
-        conn->exportname[exportnamelen] = '\0';
+        if (strlen (conn->exportname) != exportnamelen) {
+          nbdkit_error ("export name must not contain NUL bytes");
+          return -1;
+        }
         debug ("newstyle negotiation: %s: client requested export '%s'",
                optname, conn->exportname);

@@ -547,9 +552,15 @@ negotiate_handshake_newstyle_options (struct connection *conn)
           continue;
         }

-        /* Discard the export name. */
+        /* Discard the export name, after checking that it is valid. */
         memcpy (&exportnamelen, &data[0], 4);
         exportnamelen = be32toh (exportnamelen);
+        what = "checking export name";
+        if (exportnamelen > optlen-8 /* NB optlen >= 8, see above */)
+          goto opt_meta_invalid_option_len;
+        what = "export name must not contain NUL bytes";
+        if (strnlen (&data[4], exportnamelen) != exportnamelen)
+          goto opt_meta_invalid_option_len;
         opt_index = 4 + exportnamelen;

         /* Read the number of queries. */
-- 
2.21.0