* Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't fail handshake on NBD_OPT_LIST descriptions
2016-04-08 1:09 [Qemu-devel] [PATCH for-2.6] nbd: Don't fail handshake on NBD_OPT_LIST descriptions Eric Blake
@ 2016-04-08 5:51 ` Alex Bligh
2016-04-08 13:52 ` Eric Blake
2016-04-14 15:26 ` Eric Blake
2016-04-14 21:31 ` Max Reitz
2 siblings, 1 reply; 8+ messages in thread
From: Alex Bligh @ 2016-04-08 5:51 UTC (permalink / raw)
To: Eric Blake; +Cc: Alex Bligh, qemu-devel@nongnu.org, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't fail handshake on NBD_OPT_LIST descriptions
2016-04-08 5:51 ` Alex Bligh
@ 2016-04-08 13:52 ` Eric Blake
0 siblings, 0 replies; 8+ messages in thread
From: Eric Blake @ 2016-04-08 13:52 UTC (permalink / raw)
To: Alex Bligh; +Cc: qemu-devel@nongnu.org, Paolo Bonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't fail handshake on NBD_OPT_LIST descriptions
2016-04-08 1:09 [Qemu-devel] [PATCH for-2.6] nbd: Don't fail handshake on NBD_OPT_LIST descriptions Eric Blake
2016-04-08 5:51 ` Alex Bligh
@ 2016-04-14 15:26 ` Eric Blake
2016-04-14 15:46 ` Alex Bligh
2016-04-14 21:31 ` Max Reitz
2 siblings, 1 reply; 8+ messages in thread
From: Eric Blake @ 2016-04-14 15:26 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, alex, qemu block
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't fail handshake on NBD_OPT_LIST descriptions
2016-04-14 15:26 ` Eric Blake
@ 2016-04-14 15:46 ` Alex Bligh
0 siblings, 0 replies; 8+ messages in thread
From: Alex Bligh @ 2016-04-14 15:46 UTC (permalink / raw)
To: Eric Blake; +Cc: Alex Bligh, qemu-devel@nongnu.org, Paolo Bonzini, qemu block
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't fail handshake on NBD_OPT_LIST descriptions
2016-04-08 1:09 [Qemu-devel] [PATCH for-2.6] nbd: Don't fail handshake on NBD_OPT_LIST descriptions Eric Blake
2016-04-08 5:51 ` Alex Bligh
2016-04-14 15:26 ` Eric Blake
@ 2016-04-14 21:31 ` Max Reitz
2016-04-14 22:07 ` Eric Blake
2 siblings, 1 reply; 8+ messages in thread
From: Max Reitz @ 2016-04-14 21:31 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: pbonzini, alex
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't fail handshake on NBD_OPT_LIST descriptions
2016-04-14 21:31 ` Max Reitz
@ 2016-04-14 22:07 ` Eric Blake
2016-04-14 22:21 ` Max Reitz
0 siblings, 1 reply; 8+ messages in thread
From: Eric Blake @ 2016-04-14 22:07 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: pbonzini, alex
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't fail handshake on NBD_OPT_LIST descriptions
2016-04-14 22:07 ` Eric Blake
@ 2016-04-14 22:21 ` Max Reitz
0 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2016-04-14 22:21 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: pbonzini, alex
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread