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

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

[Eric Blake]

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 <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);
+            g_free(buf);
+        }
     } else {
         error_setg(errp, "Unexpected reply type %x expected %x",
                    type, NBD_REP_SERVER);
-- 
2.5.5

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

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 <alex@alex.org.uk>

> @@ -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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1697 bytes --]

On 04/07/2016 11:51 PM, Alex Bligh wrote:
> 
> On 8 Apr 2016, at 02:09, Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>> ---

>> +        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.
> 

NBD_MAX_BUFFER_SIZE is actually 32M, not 32k.

> Not technically the bug you are trying to fix, so

And yes, I need to do a much bigger scrub of qemu code, both client and
server, to allow export names longer than 255, up to the
just-barely-documented 4096 maximum in the NBD protocol.  But you are
right that such an audit is separate from this immediate fix.

> 
> Reviewed-by: Alex Bligh <alex@alex.org.uk>

Thanks.

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


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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3080 bytes --]

[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 <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);
> +            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


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

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

[Alex Bligh]

[-- Attachment #1: Type: text/plain, Size: 3292 bytes --]


On 14 Apr 2016, at 16:26, Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>

Reviewed-by: Alex Bligh <alex@alex.org.uk>


>> ---
>> 
>> 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);
>> +            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





[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]

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

[Max Reitz]

[-- 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 --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2029 bytes --]

On 04/14/2016 03:31 PM, Max Reitz wrote:
> 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.
>>

>> @@ -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.

The server.c code had a nice function for skipping unwanted bytes; and
in a 2.7 series, I borrowed that idea:

https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg01597.html

but for the purpose of minimal churn in 2.6, I don't mind the
(temporary) oddity.

> 
>> +            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

You did the right thing.

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


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

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

[Max Reitz]

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

On 15.04.2016 00:07, Eric Blake wrote:
> On 04/14/2016 03:31 PM, Max Reitz wrote:
>> 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.
>>>
> 
>>> @@ -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.
> 
> The server.c code had a nice function for skipping unwanted bytes;

I know; I added it. ;-)

>                                                                    and
> in a 2.7 series, I borrowed that idea:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg01597.html
> 
> but for the purpose of minimal churn in 2.6, I don't mind the
> (temporary) oddity.

Yeah, I didn't mean funny in the "why is that here" sense, but in the "I
chuckled a little" sense. :-)

>>> +            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
> 
> You did the right thing.

That phrasing makes me feel a little uncomfortable. It's a phrase I'd
expect to hear after I have committed a deed of questionable morality,
which was right only in the greater scheme of things and for the
well-being of the many. So now I'm a bit worried. :-)

(Just kidding, of course.)

Max


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