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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3387 bytes --]

On 08.04.2016 03:09, Eric Blake 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,

Well, you can still set length to anything and just send less... Both is
equally non-compliant. But I agree that the check makes sense.

>                            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 <eblake@redhat.com>
> ---
> 
> 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) {
> @@ -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);

I find this funny, somehow.

Perhaps it's because this may explicitly print something while
explaining that it's being ignored.

> +            g_free(buf);
> +        }
>      } else {
>          error_setg(errp, "Unexpected reply type %x expected %x",
>                     type, NBD_REP_SERVER);
> 

Thanks Eric, I applied this patch to my block branch (for 2.6). If this
was not your intention, please speak up. :-)

https://github.com/XanClic/qemu/commits/block

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]