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

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