Libguestfs - Apr 2019 - [PATCH nbdkit 2/2] server: Zero the read buffer before passing it to plugin .pread method.

This bug was found by Eric Blake.

In the .pread method we allocate a buffer in the server and pass it to
the plugin.  The plugin is supposed to fill it with data.  The buffer
was uninitialized so initially contained random heap data, but that's
OK provided the plugin fully overwrote it with data.  All correctly
written plugins ought to do this, however there is the possibility of
an incorrectly written plugin not doing so.  In that case heap memory
would be leaked to the client.

The fix for this is to zero the buffer before passing it to the
plugin, so even if the plugin doesn't fill it properly no heap memory
is leaked.

I checked our existing plugins and they are all safe, except for the
OCaml plugin.  The OCaml plugin had the same kind of mistake and
needed a separate fix.  Again, correctly written OCaml plugins should
be fine, but incorrect ones could leak heap memory.

Since nbdkit supports a stable API and ABI, plugins can be distributed
outside nbdkit and we have no control over whether those plugins are
doing the right thing.

Rich.

In the C part of the OCaml plugin we create a ‘bytes’ [byte array] and
pass it to the OCaml pread method.  The plugin should 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.

We can avoid this by initializing the array with zeroes.

Credit: Eric Blake for finding the bug.
---
 plugins/ocaml/ocaml.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/plugins/ocaml/ocaml.c b/plugins/ocaml/ocaml.c
index d854f48..7193842 100644
--- a/plugins/ocaml/ocaml.c
+++ b/plugins/ocaml/ocaml.c
@@ -444,6 +444,10 @@ pread_wrapper (void *h, void *buf, uint32_t count, uint64_t
offset,
   caml_leave_blocking_section ();
 
   strv = caml_alloc_string (count);
+  /* Initialize the buffer with zeroes in case the plugin does not
+   * fill it completely.
+   */
+  memset (String_val (strv), 0, count);
   offsetv = caml_copy_int64 (offset);
   flagsv = Val_flags (flags);
 
-- 
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.  The simplest
way to avoid any possibility of this happening is to zero the buffer
before passing it to the .pread method.

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/protocol.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/server/protocol.c b/server/protocol.c
index 54d8adb..4d67392 100644
--- a/server/protocol.c
+++ b/server/protocol.c
@@ -658,10 +658,10 @@ protocol_recv_request_send_reply (struct connection *conn)
 
     /* Allocate the data buffer used for either read or write requests. */
     if (cmd == NBD_CMD_READ || cmd == NBD_CMD_WRITE) {
-      buf = malloc (count);
+      buf = calloc (1, count);
       if (buf == NULL) {
       out_of_memory:
-        perror ("malloc");
+        perror ("calloc");
         error = ENOMEM;
         if (cmd == NBD_CMD_WRITE &&
             skip_over_write_buffer (conn->sockin, count) < 0)
-- 
2.20.1

On 4/23/19 4:01 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.  The simplest
> way to avoid any possibility of this happening is to zero the buffer
> before passing it to the .pread method.
> 
> 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/protocol.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
This patch does not document any guarantee. Would it be worth explicitly
mentioning this behavior as part of the API guarantee of .pread, so that
plugins like zero and null can rely on the incoming buffer being
pre-zeroed rather than having to repeat the memset() themselves?

Should this behavior have an opt-out switch for plugins concerned with
speed? For safety, we want this behavior to be on by default (as we
don't know whether out-of-tree plugins might otherwise leak our heap
data). But if we were to add a new field to include/nbd-plugin.h that a
plugin can set when they DON'T want to waste time on the pre-emptive
clear because they promise to fill the entire buffer, then the bug
shifts out of nbdkit for leaking data and over to the plugin for setting
the flag incorrectly if it breaks the promise. The new field won't be
set by any existing out-of-tree plugins, and newly written plugins will
have the documentation of that particular aspect to make their decision
on whether to opt-out.
> diff --git a/server/protocol.c b/server/protocol.c
> index 54d8adb..4d67392 100644
> --- a/server/protocol.c
> +++ b/server/protocol.c
> @@ -658,10 +658,10 @@ protocol_recv_request_send_reply (struct connection
*conn)
>  
>      /* Allocate the data buffer used for either read or write requests. */
>      if (cmd == NBD_CMD_READ || cmd == NBD_CMD_WRITE) {
> -      buf = malloc (count);
> +      buf = calloc (1, count);
Definitely a safety fix for READ, but isn't this a speed penalty for
WRITE?  That is, there is a cost of zeroing the buffer (especially if it
is a 32M request, where zeroing it may be large enough to also have
performance impacts due to cache pressure), when we KNOW that we are
going to immediately rewrite the buffer with incoming data before ever
calling into the plugin's .pwrite, makes it seem like we should limit
the calloc or memset to just the READ path.  I don't know if the added
time spent on calloc() is significant, or if it is in the noise compared
to other overheads such as the socket traffic, but it may be worth
benchmarking (I know you have your fio patches that might help us answer
that question).
>        if (buf == NULL) {
>        out_of_memory:
> -        perror ("malloc");
> +        perror ("calloc");
>          error = ENOMEM;
>          if (cmd == NBD_CMD_WRITE &&
>              skip_over_write_buffer (conn->sockin, count) < 0)
> 
Is there any smarter algorithm we could use? Right now, we are doing a
malloc()/free() per READ/WRITE. And although libc malloc() should in
general ty to be efficient on frequent malloc reuse, we can often still
be faster by reusing allocations rather than doing frequent
malloc()/free().  Would it be worth exploring a patch that creates a
per-thread buffer (maybe initially sized at 64k, and widened only as the
client sends larger requests), where we reuse the buffer between
successive calls rather than reallocating?  We'd still want to initially
zero the buffer when we first allocate one for the thread or when we
enlarge it, but we can argue that after that point, a plugin with a
.pread that does a partial job will only leak data that has already been
previously read from or written to that plugin (and no longer risk a
leak of OUR heap data), which means we avoid both the malloc() and the
memset() in the common case, for slightly faster behavior, and with no
visible difference in output for a plugin that fully fills the buffer in
.pread.

[I still hope to someday improve the nbdkit threading to create helper
threads on demand due to actual client parallel requests, rather than
its current behavior of spawning all parallel threads up front when the
client first connects - that becomes more important if we are also
allocating up to 64M in per-thread buffers, when we don't need the
allocation for a client that only ever sends serialized requests]

And if we DO switch to per-thread reused buffers, it may STILL make
sense to have nbdkit-plugin.h have a way for plugins to opt-in or
opt-out of whether .pread is guaranteed a pre-zeroed buffer on all reads.

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

On 4/23/19 4:01 AM, Richard W.M. Jones wrote:
> In the C part of the OCaml plugin we create a ‘bytes’ [byte array] and
> pass it to the OCaml pread method.  The plugin should 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.
> 
> We can avoid this by initializing the array with zeroes.
> 
> Credit: Eric Blake for finding the bug.
> ---
>  plugins/ocaml/ocaml.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/plugins/ocaml/ocaml.c b/plugins/ocaml/ocaml.c
> index d854f48..7193842 100644
> --- a/plugins/ocaml/ocaml.c
> +++ b/plugins/ocaml/ocaml.c
> @@ -444,6 +444,10 @@ pread_wrapper (void *h, void *buf, uint32_t count,
uint64_t offset,
>    caml_leave_blocking_section ();
>  
>    strv = caml_alloc_string (count);
> +  /* Initialize the buffer with zeroes in case the plugin does not
> +   * fill it completely.
> +   */
> +  memset (String_val (strv), 0, count);
>    offsetv = caml_copy_int64 (offset);
>    flagsv = Val_flags (flags);
Looks reasonable for ensuring there is no sensitive leak. And compared
to my comments on patch 2, at least here you are only slowing down
.pread, and leaving .pwrite unchanged.  Although my other comments on
patch 2 question whether we can still come up with something more
efficient by reusing a buffer rather than having to reinitialize it on
every single NBD_CMD_READ (leaking previously-seen plugin contents is
not the same risk as leaking uninitialized data).

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