Libguestfs - Dec 2018 - [nbdkit PATCH] connections: Don't use uninit memory on early client EOF

Fuzzing with afl found a bug where a 27 byte client sequence
can cause nbdkit to report a strange error message:

$ printf %s $'000\1IHAVEOPT000\6'$'000\7'$'000\1x00' |
tr 0 '\0' |
  ./nbdkit -s memory size=1m >/dev/null
nbdkit: memory: error: client exceeded maximum number of options (32)

The culprit? The client is hanging up on a message boundary,
so conn->recv() returns 0 for EOF instead of -1 for read error,
and our code blindly continues on in a for loop (re-)parsing
the leftover data from the previous option.

Let's fix things to uniformly quit trying to negotiate with a client
that has early EOF at a message boundary, just as we do for one that
quits mid-field, with the one difference that we treat a message
boundary as a warning instead of an error because a client hanging up
after an option response that it didn't like (rather than sending
NBD_OPT_ABORT to inform the server that it won't be negotiating
further) is a surprisingly common behavior among some existing clients.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/connections.c | 60 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 21 deletions(-)

diff --git a/src/connections.c b/src/connections.c
index 58ed6b0..577f466 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -600,6 +600,31 @@ send_newstyle_option_reply_info_export (struct connection
*conn,
   return 0;
 }

+/* Sub-function during _negotiate_handshake_newstyle, to uniformly handle
+ * a client hanging up on a message boundary.
+ */
+static int __attribute__ ((format (printf, 4, 5)))
+conn_recv_full (struct connection *conn, void *buf, size_t len,
+                const char *fmt, ...)
+{
+  int r = conn->recv (conn, buf, len);
+  va_list args;
+
+  if (r == -1) {
+    va_start (args, fmt);
+    nbdkit_verror (fmt, args);
+    va_end (args);
+    return -1;
+  }
+  if (r == 0) {
+    /* During negotiation, client EOF on message boundary is less
+     * severe than failure in the middle of the buffer. */
+    debug ("client closed input socket, closing connection");
+    return -1;
+  }
+  return r;
+}
+
 /* Sub-function of _negotiate_handshake_newstyle_options below.  It
  * must be called on all non-error paths out of the options for-loop
  * in that function.
@@ -639,10 +664,9 @@ _negotiate_handshake_newstyle_options (struct connection
*conn)
   const char *optname;

   for (nr_options = 0; nr_options < MAX_NR_OPTIONS; ++nr_options) {
-    if (conn->recv (conn, &new_option, sizeof new_option) == -1) {
-      nbdkit_error ("read: %m");
+    if (conn_recv_full (conn, &new_option, sizeof new_option,
+                        "read: %m") == -1)
       return -1;
-    }

     version = be64toh (new_option.version);
     if (version != NEW_VERSION) {
@@ -675,10 +699,9 @@ _negotiate_handshake_newstyle_options (struct connection
*conn)

     switch (option) {
     case NBD_OPT_EXPORT_NAME:
-      if (conn->recv (conn, data, optlen) == -1) {
-        nbdkit_error ("read: %s: %m", name_of_nbd_opt (option));
+      if (conn_recv_full (conn, data, optlen,
+                          "read: %s: %m", name_of_nbd_opt (option))
== -1)
         return -1;
-      }
       /* Apart from printing it, ignore the export name. */
       data[optlen] = '\0';
       debug ("newstyle negotiation: %s: "
@@ -715,10 +738,9 @@ _negotiate_handshake_newstyle_options (struct connection
*conn)
         if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
             == -1)
           return -1;
-        if (conn->recv (conn, data, optlen) == -1) {
-          nbdkit_error ("read: %s: %m", name_of_nbd_opt (option));
+        if (conn_recv_full (conn, data, optlen,
+                            "read: %s: %m", name_of_nbd_opt (option))
== -1)
           return -1;
-        }
         continue;
       }

@@ -738,10 +760,9 @@ _negotiate_handshake_newstyle_options (struct connection
*conn)
         if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
             == -1)
           return -1;
-        if (conn->recv (conn, data, optlen) == -1) {
-          nbdkit_error ("read: %s: %m", name_of_nbd_opt (option));
+        if (conn_recv_full (conn, data, optlen,
+                            "read: %s: %m", name_of_nbd_opt (option))
== -1)
           return -1;
-        }
         continue;
       }

@@ -780,10 +801,9 @@ _negotiate_handshake_newstyle_options (struct connection
*conn)
     case NBD_OPT_INFO:
     case NBD_OPT_GO:
       optname = name_of_nbd_opt (option);
-      if (conn->recv (conn, data, optlen) == -1) {
-        nbdkit_error ("read: %s: %m", optname);
+      if (conn_recv_full (conn, data, optlen,
+                          "read: %s: %m", optname) == -1)
         return -1;
-      }

       if (optlen < 6) { /* 32 bit export length + 16 bit nr info */
         debug ("newstyle negotiation: %s option length < 6",
optname);
@@ -880,10 +900,9 @@ _negotiate_handshake_newstyle_options (struct connection
*conn)
       /* Unknown option. */
       if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_UNSUP) == -1)
         return -1;
-      if (conn->recv (conn, data, optlen) == -1) {
-        nbdkit_error ("read: %m");
+      if (conn_recv_full (conn, data, optlen,
+                          "read: %m") == -1)
         return -1;
-      }
     }

     /* Note, since it's not very clear from the protocol doc, that the
@@ -931,10 +950,9 @@ _negotiate_handshake_newstyle (struct connection *conn)
   }

   /* Client now sends us its 32 bit flags word ... */
-  if (conn->recv (conn, &conn->cflags, sizeof conn->cflags) == -1)
{
-    nbdkit_error ("read: %m");
+  if (conn_recv_full (conn, &conn->cflags, sizeof conn->cflags,
+                      "read: %m") == -1)
     return -1;
-  }
   conn->cflags = be32toh (conn->cflags);
   /* ... which we check for accuracy. */
   debug ("newstyle negotiation: client flags: 0x%x",
conn->cflags);
-- 
2.17.2

On Thu, Dec 20, 2018 at 08:57:02PM -0600, Eric Blake
wrote:
> Fuzzing with afl found a bug where a 27 byte client sequence
> can cause nbdkit to report a strange error message:
> 
> $ printf %s
$'000\1IHAVEOPT000\6'$'000\7'$'000\1x00' | tr 0
'\0' |
>   ./nbdkit -s memory size=1m >/dev/null
> nbdkit: memory: error: client exceeded maximum number of options (32)
> 
> The culprit? The client is hanging up on a message boundary,
> so conn->recv() returns 0 for EOF instead of -1 for read error,
> and our code blindly continues on in a for loop (re-)parsing
> the leftover data from the previous option.
> 
> Let's fix things to uniformly quit trying to negotiate with a client
> that has early EOF at a message boundary, just as we do for one that
> quits mid-field, with the one difference that we treat a message
> boundary as a warning instead of an error because a client hanging up
> after an option response that it didn't like (rather than sending
> NBD_OPT_ABORT to inform the server that it won't be negotiating
> further) is a surprisingly common behavior among some existing clients.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  src/connections.c | 60 ++++++++++++++++++++++++++++++-----------------
>  1 file changed, 39 insertions(+), 21 deletions(-)
> 
> diff --git a/src/connections.c b/src/connections.c
> index 58ed6b0..577f466 100644
> --- a/src/connections.c
> +++ b/src/connections.c
> @@ -600,6 +600,31 @@ send_newstyle_option_reply_info_export (struct
connection *conn,
>    return 0;
>  }
> 
> +/* Sub-function during _negotiate_handshake_newstyle, to uniformly handle
> + * a client hanging up on a message boundary.
> + */
> +static int __attribute__ ((format (printf, 4, 5)))
> +conn_recv_full (struct connection *conn, void *buf, size_t len,
> +                const char *fmt, ...)
> +{
> +  int r = conn->recv (conn, buf, len);
> +  va_list args;
> +
> +  if (r == -1) {
> +    va_start (args, fmt);
> +    nbdkit_verror (fmt, args);
> +    va_end (args);
> +    return -1;
> +  }
> +  if (r == 0) {
> +    /* During negotiation, client EOF on message boundary is less
> +     * severe than failure in the middle of the buffer. */
> +    debug ("client closed input socket, closing connection");
> +    return -1;
> +  }
> +  return r;
> +}
> +
>  /* Sub-function of _negotiate_handshake_newstyle_options below.  It
>   * must be called on all non-error paths out of the options for-loop
>   * in that function.
> @@ -639,10 +664,9 @@ _negotiate_handshake_newstyle_options (struct
connection *conn)
>    const char *optname;
> 
>    for (nr_options = 0; nr_options < MAX_NR_OPTIONS; ++nr_options) {
> -    if (conn->recv (conn, &new_option, sizeof new_option) == -1) {
> -      nbdkit_error ("read: %m");
> +    if (conn_recv_full (conn, &new_option, sizeof new_option,
> +                        "read: %m") == -1)
>        return -1;
> -    }
> 
>      version = be64toh (new_option.version);
>      if (version != NEW_VERSION) {
> @@ -675,10 +699,9 @@ _negotiate_handshake_newstyle_options (struct
connection *conn)
> 
>      switch (option) {
>      case NBD_OPT_EXPORT_NAME:
> -      if (conn->recv (conn, data, optlen) == -1) {
> -        nbdkit_error ("read: %s: %m", name_of_nbd_opt (option));
> +      if (conn_recv_full (conn, data, optlen,
> +                          "read: %s: %m", name_of_nbd_opt
(option)) == -1)
>          return -1;
> -      }
>        /* Apart from printing it, ignore the export name. */
>        data[optlen] = '\0';
>        debug ("newstyle negotiation: %s: "
> @@ -715,10 +738,9 @@ _negotiate_handshake_newstyle_options (struct
connection *conn)
>          if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
>              == -1)
>            return -1;
> -        if (conn->recv (conn, data, optlen) == -1) {
> -          nbdkit_error ("read: %s: %m", name_of_nbd_opt
(option));
> +        if (conn_recv_full (conn, data, optlen,
> +                            "read: %s: %m", name_of_nbd_opt
(option)) == -1)
>            return -1;
> -        }
>          continue;
>        }
> 
> @@ -738,10 +760,9 @@ _negotiate_handshake_newstyle_options (struct
connection *conn)
>          if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
>              == -1)
>            return -1;
> -        if (conn->recv (conn, data, optlen) == -1) {
> -          nbdkit_error ("read: %s: %m", name_of_nbd_opt
(option));
> +        if (conn_recv_full (conn, data, optlen,
> +                            "read: %s: %m", name_of_nbd_opt
(option)) == -1)
>            return -1;
> -        }
>          continue;
>        }
> 
> @@ -780,10 +801,9 @@ _negotiate_handshake_newstyle_options (struct
connection *conn)
>      case NBD_OPT_INFO:
>      case NBD_OPT_GO:
>        optname = name_of_nbd_opt (option);
> -      if (conn->recv (conn, data, optlen) == -1) {
> -        nbdkit_error ("read: %s: %m", optname);
> +      if (conn_recv_full (conn, data, optlen,
> +                          "read: %s: %m", optname) == -1)
>          return -1;
> -      }
> 
>        if (optlen < 6) { /* 32 bit export length + 16 bit nr info */
>          debug ("newstyle negotiation: %s option length < 6",
optname);
> @@ -880,10 +900,9 @@ _negotiate_handshake_newstyle_options (struct
connection *conn)
>        /* Unknown option. */
>        if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_UNSUP) ==
-1)
>          return -1;
> -      if (conn->recv (conn, data, optlen) == -1) {
> -        nbdkit_error ("read: %m");
> +      if (conn_recv_full (conn, data, optlen,
> +                          "read: %m") == -1)
>          return -1;
> -      }
>      }
> 
>      /* Note, since it's not very clear from the protocol doc, that the
> @@ -931,10 +950,9 @@ _negotiate_handshake_newstyle (struct connection
*conn)
>    }
> 
>    /* Client now sends us its 32 bit flags word ... */
> -  if (conn->recv (conn, &conn->cflags, sizeof conn->cflags)
== -1) {
> -    nbdkit_error ("read: %m");
> +  if (conn_recv_full (conn, &conn->cflags, sizeof conn->cflags,
> +                      "read: %m") == -1)
>      return -1;
> -  }
>    conn->cflags = be32toh (conn->cflags);
>    /* ... which we check for accuracy. */
>    debug ("newstyle negotiation: client flags: 0x%x",
conn->cflags);
> -- 
> 2.17.2
Thanks for diagnosing this.

I have pushed the patch because I wanted to continue with fuzzing.
There are some small changes (only in the error message text) because
I rebased it on top of a patch I had queued up to improve error
reporting.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top