10:09 a.m.
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