Re: [Qemu-devel] [PATCH v3 15/19] nbd/client: Add nbd_receive_export_list()

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

12.01.2019 20:58, Eric Blake wrote:
> We want to be able to detect whether a given qemu NBD server is
> exposing the right export(s) and dirty bitmaps, at least for
> regression testing.  We could use 'nbd-client -l' from the upstream
> NBD project to list exports, but it's annoying to rely on
> out-of-tree binaries; furthermore, nbd-client doesn't necessarily
> know about all of the qemu NBD extensions.  Thus, we plan on adding
> a new mode to qemu-nbd that merely sniffs all possible information
> from the server during handshake phase, then disconnects and dumps
> the information.
> 
> This patch adds the low-level client code for grabbing the list
> of exports.  It benefits from the recent refactoring patches, as
> well as a minor tweak of changing nbd_opt_go() to nbd_opt_info_or_go(),
> in order to share as much code as possible when it comes to doing
> validation of server replies.  The resulting information is stored
> in an array of NBDExportInfo which has been expanded to any
> description string, along with a convenience function for freeing
> the list.
> 
> Note: a malicious server could exhaust memory of a client by feeding
> an unending loop of exports; perhaps we should place a limit on how
> many we are willing to receive. But note that a server could
> reasonably be serving an export for every file in a large directory,
> where an arbitrary limit in the client means we can't list anything
> from such a server; the same happens if we just run until the client
> fails to malloc() and thus dies by an abort(), where the limit is
> no longer arbitrary but determined by available memory.  Since the
> client is already planning on being short-lived, it's hard to call
> this a denial of service attack that would starve off other uses,
> so it does not appear to be a security issue.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20181215135324.152629-19-eblake@redhat.com>
> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> 
> ---
> v3: mention security (non-)issue in commit message [Rich], formatting
> tweaks
> ---

[..]

> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -330,11 +330,14 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
>   }
> 
> 
> -/* Returns -1 if NBD_OPT_GO proves the export @info->name cannot be
> +/*
> + * Returns -1 if NBD_OPT_GO proves the export @info->name cannot be
>    * used, 0 if NBD_OPT_GO is unsupported (fall back to NBD_OPT_LIST and
>    * NBD_OPT_EXPORT_NAME in that case), and > 0 if the export is good to
> - * go (with the rest of @info populated). */
> -static int nbd_opt_go(QIOChannel *ioc, NBDExportInfo *info, Error **errp)
> + * go (with the rest of @info populated).
> + */

Don't you want to upgrade a comment a little bit, to reflect support for OPT_INFO?

> +static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
> +                              NBDExportInfo *info, Error **errp)
>   {
>       NBDOptionReply reply;
>       uint32_t len = strlen(info->name);
> @@ -347,7 +350,8 @@ static int nbd_opt_go(QIOChannel *ioc, NBDExportInfo *info, Error **errp)
>        * flags still 0 is a witness of a broken server. */
>       info->flags = 0;
> 
> -    trace_nbd_opt_go_start(info->name);
> +    assert(opt == NBD_OPT_GO || opt == NBD_OPT_INFO);
> +    trace_nbd_opt_go_start(nbd_opt_lookup(opt), info->name);

I'd prefer to upgrade trace-point name too, as well as other several trace_nbd_opt_go_* trace
points in the function.

>       buf = g_malloc(4 + len + 2 + 2 * info->request_sizes + 1);
>       stl_be_p(buf, len);
>       memcpy(buf + 4, info->name, len);

[..]

> +/*
> + * nbd_receive_export_list:
> + * Query details about a server's exports, then disconnect without
> + * going into transmission phase. Return a count of the exports listed
> + * in @info by the server, or -1 on error. Caller must free @info using
> + * nbd_free_export_list().
> + */
> +int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> +                            const char *hostname, NBDExportInfo **info,
> +                            Error **errp)
> +{
> +    int result;
> +    int count = 0;
> +    int i;
> +    int rc;
> +    int ret = -1;
> +    NBDExportInfo *array = NULL;
> +    QIOChannel *sioc = NULL;
> +
> +    *info = NULL;
> +    result = nbd_start_negotiate(ioc, tlscreds, hostname, &sioc, true, NULL,
> +                                 errp);
> +    if (tlscreds && sioc) {
> +        ioc = sioc;
> +    }
> +
> +    switch (result) {
> +    case 2:
> +    case 3:
> +        /* newstyle - use NBD_OPT_LIST to populate array, then try
> +         * NBD_OPT_INFO on each array member. If structured replies
> +         * are enabled, also try NBD_OPT_LIST_META_CONTEXT. */
> +        if (nbd_send_option_request(ioc, NBD_OPT_LIST, 0, NULL, errp) < 0) {
> +            goto out;
> +        }
> +        while (1) {
> +            char *name;
> +            char *desc;
> +
> +            rc = nbd_receive_list(ioc, &name, &desc, errp);
> +            if (rc < 0) {
> +                goto out;
> +            } else if (rc == 0) {
> +                break;
> +            }
> +            array = g_renew(NBDExportInfo, array, ++count);
> +            memset(&array[count - 1], 0, sizeof(*array));
> +            array[count - 1].name = name;
> +            array[count - 1].description = desc;
> +            array[count - 1].structured_reply = result == 3;
> +        }
> +
> +        for (i = 0; i < count; i++) {
> +            array[i].request_sizes = true;
> +            rc = nbd_opt_info_or_go(ioc, NBD_OPT_INFO, &array[i], errp);
> +            if (rc < 0) {
> +                goto out;
> +            } else if (rc == 0) {
> +                /* Pointless to try rest of loop. If OPT_INFO doesn't work,
> +                 * it's unlikely that meta contexts work either */
> +                break;
> +            }
> +
> +            /* TODO: Grab meta contexts */
> +        }
> +
> +        /* Send NBD_OPT_ABORT as a courtesy before hanging up */
> +        nbd_send_opt_abort(ioc);
> +        break;
> +    case 1: /* newstyle, but limited to EXPORT_NAME */
> +        error_setg(errp, "Server does not support export lists");
> +        /* We can't even send NBD_OPT_ABORT, so merely hang up */

But, on the other hand, why not to send it? It will be unknown for the server,
so, it will have to close the connection accordingly to the protocol, isn't it
better a bit?

> +        goto out;
> +    case 0: /* oldstyle, parse length and flags */
> +        array = g_new0(NBDExportInfo, 1);
> +        array->name = g_strdup("");
> +        count = 1;
> +
> +        if (nbd_negotiate_finish_oldstyle(ioc, array, errp) < 0) {
> +            return -EINVAL;

goto out, you mean.
And with at least this one fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>



-- 
Best regards,
Vladimir