Libguestfs - Apr 2019 - Re: [PATCH nbdkit v2 1/2] ocaml: Change pread method to avoid leaking heap memory.

Version 1 was here:
https://www.redhat.com/archives/libguestfs/2019-April/msg00144.html

Version 2 makes a couple of much larger changes:

The OCaml patch changes the API of the pread method so it matches what
other language bindings are already doing, ie. get the language plugin
to return a newly allocated buffer, check it is long enough, copy out
the data.

The server patch implements a per-thread buffer for all pread and
pwrite calls which is zeroed whenever it is extended to avoid the heap
memory leak.

Rich.

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 4/23/19 10:09 AM, Richard W.M. Jones wrote:
> 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
> 
Incompatible change (all older OCaml plugins will fail to build/run with
the newer nbdkit), but we never promised compatibility for language
bindings.
> ---
>  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(-)
I wondered if nbdkit-ocaml-plugin.pod also needed a change, but it calls
out NBDKit.mli as the official interface and you patched that instead.

LGTM.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 4/23/19 10:09 AM, Richard W.M. Jones wrote:
> 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.
Could merge these last two paragraphs into one and drop the special-case
mention of OCaml (now that the previous commit makes OCaml like all the
other plugins).
> 
> 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(-)
> 
>      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;
Old code called perror() when nbdkit_extents_new() failed...
>          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;
> +      }
...new code does not. nbdkit_error() might be nicer than perror()
anyways. I'll let you decide what, if any, error reporting needs to be
done beyond merely sending ENOMEM to the client.

> +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);
You could memset just the tail of the enlarged buffer compared to its
previous size, but this is fine, too.
> +    threadlocal->buffer = ptr;
> +    threadlocal->buffer_size = size;
> +  }
> +
> +  return threadlocal->buffer;
> +}
> 
Other than my question about extent allocation failure, LGTM.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org