[Libguestfs] [libnbd PATCH 1/3] api: Drop server control of memset() prior to NBD_CMD_READ

[Eric Blake]

The recent CVE-2022-0485 demonstrated that clients that pass in an
uninitialized buffer to nbd_pread and friends, but are then not
careful about checking for read errors, were subjected to
server-dependent behavior on whether their use of the buffer after
failure saw sanitized zeroes or prior heap contents.

Making our choice of whether to sanitize the buffer be under the
control of what the server negotiates is not ideal.  We commonly test
with servers that support structured replies (nbdkit by default, as
well as qemu-nbd), and less frequently with servers that lack them
(nbd-server as of the current nbd-3.23 release, or 'nbdkit --no-sr'),
thus, we are already used to the runtime speed of libnbd sanitizing a
read buffer, and more likely to miss the additional security impact
caused when a less-common server can turn a client bug into a heap
leak.

This patch makes the sanitization unconditional if we actually reach
the point of issuing NBD_CMD_READ (to make it easier to backport in
isolation for distros that want secure-by-default for clients that
call nbd_pread correctly).  Note that with just this patch, there are
still some nbd_pread errors where the buffer is left uninitialized
(such as calling nbd_pread before nbd_connect, or if the pread is
aborted client-side due to detecting invalid parameters with
nbd_set_strict_mode); but client apps are generally less likely to
trip those corner cases, and any heap leak in those cases is not
dependent on server behavior.

Then upcoming patches will then hoist the memset() even earlier, to
give guaranteed buffer contents on all error paths, then add a new API
to allow clients to inform libnbd when to skip the sanitization as an
optimization (either because the client sanitized the buffer itself,
or has audited to ensure that an uninitialized buffer is not
dereferenced).

Fixes: 7c2543e9 ("lib: For structured replies, zero the buffer ahead of time.", v0.1)
---
 lib/rw.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/rw.c b/lib/rw.c
index 4ade7508..b5d3dc44 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -1,5 +1,5 @@
 /* NBD client library in userspace
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2020, 2022 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -250,9 +250,11 @@ nbd_internal_command_common (struct nbd_handle *h,
    * ahead of time which avoids any security problems.  I measured the
    * overhead of this and for non-TLS there is no measurable overhead
    * in the highly intensive loopback case.  For TLS we get a
-   * performance gain, go figure.
+   * performance gain, go figure.  For an older server with only
+   * simple replies, it's still better to do the same memset() so we
+   * don't have behavior that is server-dependent.
    */
-  if (h->structured_replies && cmd->data && type == NBD_CMD_READ)
+  if (cmd->data && type == NBD_CMD_READ)
     memset (cmd->data, 0, cmd->count);

   /* Add the command to the end of the queue. Kick the state machine
-- 
2.34.1