In the C part of the OCaml plugin we created a ‘bytes’ [byte array] and passed it to the OCaml pread method. The plugin is supposed to overwrite the array with the returned data. However if (eg. because of a bug) the plugin does not fill the array then whatever was in the OCaml or possibly even the C heap before the allocation is returned to the client, possibly resulting in a leak of sensitive data. This changes the signature of the pread function in OCaml plugins. Instead of passing in an uninitialized ‘bytes’ object of the right length, the count is passed explicitly and the pread method should return a string of the correct length. This is both more similar to how other language plugins work, and is safer because all allocation is done on the OCaml side. - pread : 'a -> bytes -> int64 -> flags -> unit + pread : 'a -> int32 -> int64 -> flags -> string Credit: Eric Blake for finding the bug. --- plugins/ocaml/ocaml.c | 16 ++++++++++++---- plugins/ocaml/NBDKit.ml | 4 ++-- plugins/ocaml/NBDKit.mli | 2 +- tests/test_ocaml_plugin.ml | 8 +++++--- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/plugins/ocaml/ocaml.c b/plugins/ocaml/ocaml.c index d854f48..39704e2 100644 --- a/plugins/ocaml/ocaml.c +++ b/plugins/ocaml/ocaml.c @@ -439,15 +439,16 @@ pread_wrapper (void *h, void *buf, uint32_t count, uint64_t offset, uint32_t flags) { CAMLparam0 (); - CAMLlocal4 (rv, strv, offsetv, flagsv); + CAMLlocal4 (rv, countv, offsetv, flagsv); + mlsize_t len; caml_leave_blocking_section (); - strv = caml_alloc_string (count); + countv = caml_copy_int32 (count); offsetv = caml_copy_int64 (offset); flagsv = Val_flags (flags); - value args[] = { *(value *) h, strv, offsetv, flagsv }; + value args[] = { *(value *) h, countv, offsetv, flagsv }; rv = caml_callbackN_exn (pread_fn, sizeof args / sizeof args[0], args); if (Is_exception_result (rv)) { nbdkit_error ("%s", caml_format_exception (Extract_exception (rv))); @@ -455,7 +456,14 @@ pread_wrapper (void *h, void *buf, uint32_t count, uint64_t offset, CAMLreturnT (int, -1); } - memcpy (buf, String_val (strv), count); + len = caml_string_length (rv); + if (len < count) { + nbdkit_error ("buffer returned from pread is too small"); + caml_enter_blocking_section (); + CAMLreturnT (int, -1); + } + + memcpy (buf, String_val (rv), count); caml_enter_blocking_section (); CAMLreturnT (int, 0); diff --git a/plugins/ocaml/NBDKit.ml b/plugins/ocaml/NBDKit.ml index b03ff5a..14b9803 100644 --- a/plugins/ocaml/NBDKit.ml +++ b/plugins/ocaml/NBDKit.ml @@ -65,7 +65,7 @@ type 'a plugin = { can_zero : ('a -> bool) option; can_fua : ('a -> fua_flag) option; - pread : ('a -> bytes -> int64 -> flags -> unit) option; + pread : ('a -> int32 -> int64 -> flags -> string) option; pwrite : ('a -> string -> int64 -> flags -> unit) option; flush : ('a -> flags -> unit) option; trim : ('a -> int32 -> int64 -> flags -> unit) option; @@ -146,7 +146,7 @@ external set_dump_plugin : (unit -> unit) -> unit = "ocaml_nbdkit_set_dump_plugi external set_can_zero : ('a -> bool) -> unit = "ocaml_nbdkit_set_can_zero" external set_can_fua : ('a -> fua_flag) -> unit = "ocaml_nbdkit_set_can_fua" -external set_pread : ('a -> bytes -> int64 -> flags -> unit) -> unit = "ocaml_nbdkit_set_pread" +external set_pread : ('a -> int32 -> int64 -> flags -> string) -> unit = "ocaml_nbdkit_set_pread" external set_pwrite : ('a -> string -> int64 -> flags -> unit) -> unit = "ocaml_nbdkit_set_pwrite" external set_flush : ('a -> flags -> unit) -> unit = "ocaml_nbdkit_set_flush" external set_trim : ('a -> int32 -> int64 -> flags -> unit) -> unit = "ocaml_nbdkit_set_trim" diff --git a/plugins/ocaml/NBDKit.mli b/plugins/ocaml/NBDKit.mli index d1e1343..7482118 100644 --- a/plugins/ocaml/NBDKit.mli +++ b/plugins/ocaml/NBDKit.mli @@ -67,7 +67,7 @@ type 'a plugin = { can_zero : ('a -> bool) option; can_fua : ('a -> fua_flag) option; - pread : ('a -> bytes -> int64 -> flags -> unit) option; (* required *) + pread : ('a -> int32 -> int64 -> flags -> string) option; (* required *) pwrite : ('a -> string -> int64 -> flags -> unit) option; flush : ('a -> flags -> unit) option; trim : ('a -> int32 -> int64 -> flags -> unit) option; diff --git a/tests/test_ocaml_plugin.ml b/tests/test_ocaml_plugin.ml index 842f10e..f27c099 100644 --- a/tests/test_ocaml_plugin.ml +++ b/tests/test_ocaml_plugin.ml @@ -28,9 +28,11 @@ let test_close h = let test_get_size h = Int64.of_int (Bytes.length h.disk) -let test_pread h buf offset _ = - let len = Bytes.length buf in - Bytes.blit h.disk (Int64.to_int offset) buf 0 len +let test_pread h count offset _ = + let count = Int32.to_int count in + let buf = Bytes.create count in + Bytes.blit h.disk (Int64.to_int offset) buf 0 count; + Bytes.unsafe_to_string buf let test_pwrite h buf offset _ = let len = String.length buf in -- 2.20.1

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

On Tue, Apr 23, 2019 at 10:31:06AM -0500, Eric Blake wrote: Yes, I believe that was a mistake. nbdkit_extents_new already calls nbdkit_error so calling perror isn't necessary. Perhaps I should split this into a separate patch. OK, I'll split that other thing out into a separate patch, and push it all. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW