| Message ID | 1460077777-31004-1-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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.
[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); >
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
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
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.
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
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);
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(-)