Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't fail handshake on NBD_OPT_LIST descriptions

[Alex Bligh]

On 8 Apr 2016, at 02:09, Eric Blake <address@hidden> wrote:

> The NBD Protocol states that NBD_REP_SERVER may set
> 'length > sizeof(namelen) + namelen'; in which case the rest
> of the packet is a UTF-8 description of the export.  While we
> don't know of any NBD servers that send this description yet,
> we had better consume the data so we don't choke when we start
> to talk to such a server.
> 
> Also, a (buggy/malicious) server that replies with length <
> sizeof(namelen) would cause us to block waiting for bytes that
> the server is not sending, and one that replies with super-huge
> lengths could cause us to temporarily allocate up to 4G memory.
> Sanity check things before blindly reading incorrectly.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
> 
> Yet another case of code introduced in 2.6 that doesn't play
> nicely with spec-compliant servers...
> 
> Hopefully I've squashed them all now?
> 
> nbd/client.c | 23 +++++++++++++++++++++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 6777e58..48f2a21 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -192,13 +192,18 @@ static int nbd_receive_list(QIOChannel *ioc, char 
> **name, Error **errp)
>             return -1;
>         }
>     } else if (type == NBD_REP_SERVER) {
> +        if (len < sizeof(namelen) || len > NBD_MAX_BUFFER_SIZE) {
> +            error_setg(errp, "incorrect option length");
> +            return -1;
> +        }
>         if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
>             error_setg(errp, "failed to read option name length");
>             return -1;
>         }
>         namelen = be32_to_cpu(namelen);
> -        if (len != (namelen + sizeof(namelen))) {
> -            error_setg(errp, "incorrect option mame length");
> +        len -= sizeof(namelen);
> +        if (len < namelen) {
> +            error_setg(errp, "incorrect option name length");
>             return -1;
>         }
>         if (namelen > 255) {

Shouldn't that be 4096? You are after all reading up to NBD_MAX_BUFFER_SIZE 
(32K) of data just earlier.

Not technically the bug you are trying to fix, so

Reviewed-by: Alex Bligh <address@hidden>

> @@ -214,6 +219,20 @@ static int nbd_receive_list(QIOChannel *ioc, char 
> **name, Error **errp)
>             return -1;
>         }
>         (*name)[namelen] = '\0';
> +        len -= namelen;
> +        if (len) {
> +            char *buf = g_malloc(len + 1);
> +            if (read_sync(ioc, buf, len) != len) {
> +                error_setg(errp, "failed to read export description");
> +                g_free(*name);
> +                g_free(buf);
> +                *name = NULL;
> +                return -1;
> +            }
> +            buf[len] = '\0';
> +            TRACE("Ignoring export description: %s", buf);
> +            g_free(buf);
> +        }
>     } else {
>         error_setg(errp, "Unexpected reply type %x expected %x",
>                    type, NBD_REP_SERVER);
> -- 
> 2.5.5
> 
> 

-- 
Alex Bligh