[Libguestfs] [engineering.redhat.com #497355] [nbdkit PATCH 1/4] server: Fix regression for NBD_OPT_INFO before NBD_OPT_GO

[Red Hat Product Security]

Hi Eric,

Thank you for cc'ing secalert at .

I'll forward this to the team.

Best regards.

On Thu Sep 19 00:01:27 2019, eblake at redhat.com wrote:
> Most known NBD clients do not bother with NBD_OPT_INFO (except for
> clients like 'qemu-nbd --list' that don't ever intend to connect), but
> go straight to NBD_OPT_GO. However, it's not too hard to hack up qemu
> to add in an extra client step (whether info on the same name, or more
> interestingly, info on a different name), as a patch against qemu
> commit 6f214b30445:
>
> | diff --git i/nbd/client.c w/nbd/client.c
> | index f6733962b49b..425292ac5ea9 100644
> | --- i/nbd/client.c
> | +++ w/nbd/client.c
> | @@ -1038,6 +1038,14 @@ int nbd_receive_negotiate(AioContext
> *aio_context, QIOChannel *ioc,
> | * TLS). If it is not available, fall back to
> | * NBD_OPT_LIST for nicer error messages about a missing
> | * export, then use NBD_OPT_EXPORT_NAME. */
> | + if (getenv ("HACK"))
> | + info->name[0]++;
> | + result = nbd_opt_info_or_go(ioc, NBD_OPT_INFO, info, errp);
> | + if (getenv ("HACK"))
> | + info->name[0]--;
> | + if (result < 0) {
> | + return -EINVAL;
> | + }
> | result = nbd_opt_info_or_go(ioc, NBD_OPT_GO, info, errp);
> | if (result < 0) {
> | return -EINVAL;
>
> This works just fine in 1.14.0, where we call .open only once (so the
> INFO and GO repeat calls into the same plugin handle), but in 1.14.1
> it regressed into causing an assertion failure: we are now calling
> .open a second time on a connection that is already opened.
>
> $ nbdkit -rfv null &
> $ hacked-qemu-io -f raw -r nbd://localhost -c quit
> ...
> nbdkit: null[1]: debug: null: open readonly=1
> nbdkit: backend.c:179: backend_open: Assertion `h->handle == NULL'
> failed.
>
> Worse, on the mainline development, we have recently made it possible
> for plugins to actively report different information for different
> export names; for example, a plugin may choose to report different
> answers for .can_write on export A than for export B; but if we share
> cached handles, then an NBD_OPT_INFO on one export prevents correct
> answers for NBD_OPT_GO on the second export name. (The HACK envvar in
> my qemu modifications can be used to demonstrate cross-name requests,
> which are even less likely in a real client).
>
> The solution is to call .close after NBD_OPT_INFO, coupled with enough
> glue logic to reset cached connection handles back to the state
> expected by .open. This in turn means factoring out another backend_*
> function, but also gives us an opportunity to change
> backend_set_handle to no longer accept NULL.
>
> The assertion failure is, to some extent, a possible denial of service
> attack (one client can force nbdkit to exit by merely sending OPT_INFO
> before OPT_GO, preventing the next client from connecting), although
> this is mitigated by using TLS to weed out untrusted clients. Still,
> the fact that we introduced a potential DoS attack while trying to fix
> a traffic amplification security bug is not very nice.
>
> Sadly, as there are no known clients that easily trigger this mode of
> operation (OPT_INFO before OPT_GO), there is no easy way to cover this
> via a testsuite addition. I may end up hacking something into libnbd.
>
> Fixes: c05686f957
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> server/internal.h | 4 +++-
> server/backend.c | 13 +++++++++++++
> server/connections.c | 2 +-
> server/filters.c | 5 +----
> server/plugins.c | 4 ----
> server/protocol-handshake-newstyle.c | 6 ++++++
> 6 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/server/internal.h b/server/internal.h
> index c31bb340..da4fae19 100644
> --- a/server/internal.h
> +++ b/server/internal.h
> @@ -334,9 +334,11 @@ extern int backend_open (struct backend *b,
> struct connection *conn,
> __attribute__((__nonnull__ (1, 2)));
> extern int backend_prepare (struct backend *b, struct connection
> *conn)
> __attribute__((__nonnull__ (1, 2)));
> +extern void backend_close (struct backend *b, struct connection
> *conn)
> + __attribute__((__nonnull__ (1, 2)));
> extern void backend_set_handle (struct backend *b, struct connection
> *conn,
> void *handle)
> - __attribute__((__nonnull__ (1, 2 /* not 3 */)));
> + __attribute__((__nonnull__ (1, 2, 3)));
> extern bool backend_valid_range (struct backend *b, struct connection
> *conn,
> uint64_t offset, uint32_t count)
> __attribute__((__nonnull__ (1, 2)));
> diff --git a/server/backend.c b/server/backend.c
> index 3b213bfb..64dbf7db 100644
> --- a/server/backend.c
> +++ b/server/backend.c
> @@ -201,10 +201,23 @@ backend_prepare (struct backend *b, struct
> connection *conn)
> return b->prepare (b, conn, h->can_write == 0);
> }
>
> +void
> +backend_close (struct backend *b, struct connection *conn)
> +{
> + struct b_conn_handle *h = &conn->handles[b->i];
> +
> + debug ("%s: close", b->name);
> +
> + b->close (b, conn);
> + memset (h, -1, sizeof *h);
> + h->handle = NULL;
> +}
> +
> void
> backend_set_handle (struct backend *b, struct connection *conn, void
> *handle)
> {
> assert (b->i < conn->nr_handles);
> + assert (conn->handles[b->i].handle == NULL);
> conn->handles[b->i].handle = handle;
> }
>
> diff --git a/server/connections.c b/server/connections.c
> index 819f7b86..3c4296ea 100644
> --- a/server/connections.c
> +++ b/server/connections.c
> @@ -369,7 +369,7 @@ free_connection (struct connection *conn)
> */
> if (!quit && connection_get_handle (conn, 0)) {
> lock_request (conn);
> - backend->close (backend, conn);
> + backend_close (backend, conn);
> unlock_request (conn);
> }
>
> diff --git a/server/filters.c b/server/filters.c
> index 1ee62829..1091c2dd 100644
> --- a/server/filters.c
> +++ b/server/filters.c
> @@ -225,12 +225,9 @@ filter_close (struct backend *b, struct
> connection *conn)
> struct backend_filter *f = container_of (b, struct backend_filter,
> backend);
> void *handle = connection_get_handle (conn, b->i);
>
> - debug ("%s: close", b->name);
> -
> if (handle && f->filter.close)
> f->filter.close (handle);
> - backend_set_handle (b, conn, NULL);
> - b->next->close (b->next, conn);
> + backend_close (b->next, conn);
> }
>
> /* The next_functions structure contains pointers to backend
> diff --git a/server/plugins.c b/server/plugins.c
> index 9b44c548..727fb0e0 100644
> --- a/server/plugins.c
> +++ b/server/plugins.c
> @@ -273,12 +273,8 @@ plugin_close (struct backend *b, struct
> connection *conn)
>
> assert (connection_get_handle (conn, 0));
>
> - debug ("close");
> -
> if (p->plugin.close)
> p->plugin.close (connection_get_handle (conn, 0));
> -
> - backend_set_handle (b, conn, NULL);
> }
>
> static int64_t
> diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-
> handshake-newstyle.c
> index 87e0bcd7..06fc53ad 100644
> --- a/server/protocol-handshake-newstyle.c
> +++ b/server/protocol-handshake-newstyle.c
> @@ -469,6 +469,12 @@ negotiate_handshake_newstyle_options (struct
> connection *conn)
> if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) ==
> -1)
> return -1;
>
> + if (option == NBD_OPT_INFO) {
> + if (backend->finalize (backend, conn) == -1)
> + return -1;
> + backend_close (backend, conn);
> + }
> +
> break;
>
> case NBD_OPT_STRUCTURED_REPLY:


--
Pedro Sampaio - Red Hat Product Security
3635 1D27 6A05 6AC3 BC0B 30A4 52CF 575B E51B 20F4