[Libguestfs] [PATCH nbdkit v2 2/2] server: Use a thread-local pread/pwrite buffer to avoid leaking heap data.

[Richard W.M. Jones]

If the plugin .pread method did not fill the buffer with data then the
contents of the heap could be leaked back to the client.  To avoid
this create a thread-local data buffer which is initialized to zero
and expanded (with zeroes) as required.

This buffer is shared between pread and pwrite which makes the code a
little bit simpler.  Also this may improve locality by reusing the
same memory for all pread and pwrite calls in the same thread.

I have checked plugins shipped with nbdkit and none of them are
vulnerable in this way.  They all do one of these things:

 - They fully set the buffer with a single call to something like
   memcpy, memset, etc.

 - They use a loop like ‘while (count > 0)’ and correctly update count
   and the buffer pointer on each iteration.

 - For language plugins (except OCaml), they check that the string
   length returned from the language plugin is at least as long as the
   requested data, before memcpy-ing the data to the return buffer.

 - For OCaml, see the previous commit.

Of course I cannot check plugins which may be supplied by others.

Credit: Eric Blake for finding the bug.
---
 server/internal.h    |  1 +
 server/protocol.c    | 16 +++++++++-------
 server/threadlocal.c | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/server/internal.h b/server/internal.h
index 837cd4a..817f022 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -350,6 +350,7 @@ extern void threadlocal_set_sockaddr (const struct sockaddr *addr,
 /*extern void threadlocal_get_sockaddr ();*/
 extern void threadlocal_set_error (int err);
 extern int threadlocal_get_error (void);
+extern void *threadlocal_buffer (size_t size);
 
 /* Declare program_name. */
 #if HAVE_DECL_PROGRAM_INVOCATION_SHORT_NAME == 1
diff --git a/server/protocol.c b/server/protocol.c
index 3f89f6d..9e8eea5 100644
--- a/server/protocol.c
+++ b/server/protocol.c
@@ -611,7 +611,7 @@ protocol_recv_request_send_reply (struct connection *conn)
   uint16_t cmd, flags;
   uint32_t magic, count, error = 0;
   uint64_t offset;
-  CLEANUP_FREE char *buf = NULL;
+  char *buf;
   CLEANUP_EXTENTS_FREE struct nbdkit_extents *extents = NULL;
 
   /* Read the request packet. */
@@ -656,12 +656,12 @@ protocol_recv_request_send_reply (struct connection *conn)
       goto send_reply;
     }
 
-    /* Allocate the data buffer used for either read or write requests. */
+    /* Get the data buffer used for either read or write requests.
+     * This is a common per-thread data buffer, it must not be freed.
+     */
     if (cmd == NBD_CMD_READ || cmd == NBD_CMD_WRITE) {
-      buf = malloc (count);
+      buf = threadlocal_buffer ((size_t) count);
       if (buf == NULL) {
-      out_of_memory:
-        perror ("malloc");
         error = ENOMEM;
         if (cmd == NBD_CMD_WRITE &&
             skip_over_write_buffer (conn->sockin, count) < 0)
@@ -673,8 +673,10 @@ protocol_recv_request_send_reply (struct connection *conn)
     /* Allocate the extents list for block status only. */
     if (cmd == NBD_CMD_BLOCK_STATUS) {
       extents = nbdkit_extents_new (offset, conn->exportsize);
-      if (extents == NULL)
-        goto out_of_memory;
+      if (extents == NULL) {
+        error = ENOMEM;
+        goto send_reply;
+      }
     }
 
     /* Receive the write data buffer. */
diff --git a/server/threadlocal.c b/server/threadlocal.c
index e556dbc..49ae1ac 100644
--- a/server/threadlocal.c
+++ b/server/threadlocal.c
@@ -58,6 +58,8 @@ struct threadlocal {
   struct sockaddr *addr;
   socklen_t addrlen;
   int err;
+  void *buffer;
+  size_t buffer_size;
 };
 
 static pthread_key_t threadlocal_key;
@@ -69,6 +71,7 @@ free_threadlocal (void *threadlocalv)
 
   free (threadlocal->name);
   free (threadlocal->addr);
+  free (threadlocal->buffer);
   free (threadlocal);
 }
 
@@ -189,3 +192,37 @@ threadlocal_get_error (void)
   errno = err;
   return threadlocal ? threadlocal->err : 0;
 }
+
+/* Return the single pread/pwrite buffer for this thread.  The buffer
+ * size is increased to ‘size’ bytes if required.
+ *
+ * The buffer starts out as zeroes but after use may contain data from
+ * previous requests.  This is fine because: (a) Correctly written
+ * plugins should overwrite the whole buffer on each request so no
+ * leak should occur.  (b) The aim of this buffer is to avoid leaking
+ * random heap data from the core server; previous request data from
+ * the plugin is not considered sensitive.
+ */
+extern void *
+threadlocal_buffer (size_t size)
+{
+  struct threadlocal *threadlocal = pthread_getspecific (threadlocal_key);
+
+  if (!threadlocal)
+    abort ();
+
+  if (threadlocal->buffer_size < size) {
+    void *ptr;
+
+    ptr = realloc (threadlocal->buffer, size);
+    if (ptr == NULL) {
+      nbdkit_error ("threadlocal_buffer: realloc: %m");
+      return NULL;
+    }
+    memset (ptr, 0, size);
+    threadlocal->buffer = ptr;
+    threadlocal->buffer_size = size;
+  }
+
+  return threadlocal->buffer;
+}
-- 
2.20.1