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

[Alex Bligh]

On 14 Apr 2016, at 16:26, Eric Blake  wrote:

> [adding qemu-block in cc, since Paolo can't send pull request]
> 
> On 04/07/2016 07:09 PM, 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, 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 

Reviewed-by: Alex Bligh 


>> ---
>> 
>> 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, , 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);
>> +g_free(buf);
>> +}
>> } else {
>> error_setg(errp, "Unexpected reply type %x expected %x",
>>type, NBD_REP_SERVER);
>> 
> 
> --
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

--
Alex Bligh






signature.asc
Description: Message signed with OpenPGP using GPGMail

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

[Eric Blake]

[adding qemu-block in cc, since Paolo can't send pull request]

On 04/07/2016 07:09 PM, 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, 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 
> ---
> 
> 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, , 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);
> +g_free(buf);
> +}
>  } else {
>  error_setg(errp, "Unexpected reply type %x expected %x",
> type, NBD_REP_SERVER);
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature