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

[Laszlo Ersek]

On 02/09/22 23:07, Eric Blake wrote:
> 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
> 

Acked-by: Laszlo Ersek <lersek at redhat.com>