Libguestfs - Nov 2017 - [nbdkit PATCH 0/2] Better response to bogus NBD_CMD_READ

When facing a malicious client that is sending bogus NBD_CMD_READ,
we should make sure that we never end up in a situation where we
could try to treat the tail from a command that we diagnosed as
bad as being further commands.

Eric Blake (2):
  connections: Report mid-message EOF as fatal
  connections: Hang up early on insanely large WRITE requests

 src/connections.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

-- 
2.13.6

If we encounter any read error in the middle of the message
(including the NBD_CMD_WRITE payload), we are no longer in sync
with the client and should not try to read anything further.
For mid-message EOF, the problem wasn't too bad (the next
iteration of the loop would also see EOF); but if it is some
other error (perhaps a transient EIO due to reading from a
flaky block device), not marking the connection failed could
result in us trying to treat the tail of the previous partial
payload as further commands.  Usually a magic number mismatch
would flag the problem, but we should never base our behavior
on the contents of that random payload.

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

diff --git a/src/connections.c b/src/connections.c
index 0cbf54a..d0ef6a5 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -873,7 +873,7 @@ handle_request (struct connection *conn,
   return r;
 }

-static void
+static int
 skip_over_write_buffer (int sock, size_t count)
 {
   char buf[BUFSIZ];
@@ -883,12 +883,16 @@ skip_over_write_buffer (int sock, size_t count)
     r = read (sock, buf, count > BUFSIZ ? BUFSIZ : count);
     if (r == -1) {
       nbdkit_error ("skipping write buffer: %m");
-      return;
+      return -1;
+    }
+    if (r == 0)  {
+      nbdkit_error ("unexpected early EOF");
+      errno = EBADMSG;
+      return -1;
     }
-    if (r == 0)
-      return;
     count -= r;
   }
+  return 0;
 }

 /* Convert a system errno to an NBD_E* error code. */
@@ -965,8 +969,9 @@ recv_request_send_reply (struct connection *conn)
   if (r == -1)
     return -1;
   if (r == 0) {                 /* request not valid */
-    if (cmd == NBD_CMD_WRITE)
-      skip_over_write_buffer (conn->sockin, count);
+    if (cmd == NBD_CMD_WRITE &&
+        skip_over_write_buffer (conn->sockin, count) < 0)
+      return -1;
     goto send_reply;
   }

@@ -976,8 +981,9 @@ recv_request_send_reply (struct connection *conn)
     if (buf == NULL) {
       perror ("malloc");
       error = ENOMEM;
-      if (cmd == NBD_CMD_WRITE)
-        skip_over_write_buffer (conn->sockin, count);
+      if (cmd == NBD_CMD_WRITE &&
+          skip_over_write_buffer (conn->sockin, count) < 0)
+        return -1;
       goto send_reply;
     }
   }
@@ -985,14 +991,14 @@ recv_request_send_reply (struct connection *conn)
   /* Receive the write data buffer. */
   if (cmd == NBD_CMD_WRITE) {
     r = conn->recv (conn, buf, count);
+    if (r == 0) {
+      errno = EBADMSG;
+      r = -1;
+    }
     if (r == -1) {
       nbdkit_error ("read data: %m");
       return -1;
     }
-    if (r == 0) {
-      debug ("client closed input unexpectedly, closing connection");
-      return 0;                 /* disconnect */
-    }
   }

   /* Perform the request.  Only this part happens inside the request lock. */
-- 
2.13.6

We have logic to send an ENOMEM error to a client that tries
NBD_CMD_WRITE with a payload larger than MAX_REQUEST_SIZE (64M),
but still end up trying to skip over the client's payload to
stay in sync for receiving the next command.  If the bad client
request is only partially larger than our maximum, this is still
nice behavior; but a worst-case client could cause us to waste
time on read()ing nearly 4G of data before we ever get to send
our error reply.

For a client that bad, it is better to just disconnect.  Even if
we wanted to be nice and send an error message reply, we'd still
be out of sync for further reads, so the simplest option is to
just silently disconnect.

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

diff --git a/src/connections.c b/src/connections.c
index d0ef6a5..8dc1925 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -879,6 +879,11 @@ skip_over_write_buffer (int sock, size_t count)
   char buf[BUFSIZ];
   ssize_t r;

+  if (count > MAX_REQUEST_SIZE * 2) {
+    nbdkit_error ("write request too large to skip");
+    return -1;
+  }
+
   while (count > 0) {
     r = read (sock, buf, count > BUFSIZ ? BUFSIZ : count);
     if (r == -1) {
-- 
2.13.6

ACK series.

I believe you're able to push the patches directly upstream?

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