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