Hi Eric, Thank you for cc'ing secalert@.
I'll forward this to the team. Best regards. On Thu Sep 19 00:01:27 2019, [email protected] 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 <[email protected]> > --- > 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 _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
