[libvirt] [PATCH] uml: avoid crash on partial read
Coverity detected a potential dereference of uninitialized memory if recvfrom got cut short. * src/uml/uml_driver.c (umlMonitorCommand): Validate complete read prior to dereferencing res. --- The patch borrows ideas from macvtap.c, the only other file in libvirt that currently uses recvfrom. I did not analyze whether this is a security hole, where a malicious UDP packet could intentionally force the dereferencing of uninitialized memory to misbehave in a controlled manner. src/uml/uml_driver.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index bbea429..eec239f 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -733,14 +733,24 @@ static int umlMonitorCommand(virConnectPtr conn, } do { + ssize_t nbytes; addrlen = sizeof(addr); - if (recvfrom(priv->monitor, &res, sizeof res, 0, - (struct sockaddr *)&addr, &addrlen) < 0) { + nbytes = recvfrom(priv->monitor, &res, sizeof res, 0, + (struct sockaddr *)&addr, &addrlen) < 0; + if (nbytes < 0) { + if (errno == EAGAIN || errno == EINTR) + continue; virReportSystemError(errno, _("cannot read reply %s"), cmd); goto error; } + if (nbytes < sizeof res) { + virReportSystemError(errno, + _("incomplete reply %s"), + cmd); + goto error; + } if (VIR_REALLOC_N(retdata, retlen + res.length) < 0) { virReportOOMError(); -- 1.6.6.1
Eric Blake wrote:
Coverity detected a potential dereference of uninitialized memory if recvfrom got cut short.
* src/uml/uml_driver.c (umlMonitorCommand): Validate complete read prior to dereferencing res. ---
The patch borrows ideas from macvtap.c, the only other file in libvirt that currently uses recvfrom.
I did not analyze whether this is a security hole, where a malicious UDP packet could intentionally force the dereferencing of uninitialized memory to misbehave in a controlled manner.
That warning highlights a problem even with a complete read (and malicious content). It exposes what is at the very least a risk of DoS, since we use the just-read (and not sanity-checked) res.length as the new buffer size in VIR_REALLOC_N.
src/uml/uml_driver.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index bbea429..eec239f 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -733,14 +733,24 @@ static int umlMonitorCommand(virConnectPtr conn, }
do { + ssize_t nbytes; addrlen = sizeof(addr); - if (recvfrom(priv->monitor, &res, sizeof res, 0, - (struct sockaddr *)&addr, &addrlen) < 0) { + nbytes = recvfrom(priv->monitor, &res, sizeof res, 0, + (struct sockaddr *)&addr, &addrlen) < 0; + if (nbytes < 0) { + if (errno == EAGAIN || errno == EINTR) + continue; virReportSystemError(errno, _("cannot read reply %s"), cmd); goto error; } + if (nbytes < sizeof res) { + virReportSystemError(errno, + _("incomplete reply %s"), + cmd); + goto error; + }
ACK to the above patch, but we need one more: Please add a check here to ensure that res.length is no larger than sizeof res.data.
if (VIR_REALLOC_N(retdata, retlen + res.length) < 0) { virReportOOMError();
On Tue, Mar 02, 2010 at 05:16:05PM -0700, Eric Blake wrote:
Coverity detected a potential dereference of uninitialized memory if recvfrom got cut short.
* src/uml/uml_driver.c (umlMonitorCommand): Validate complete read prior to dereferencing res. ---
The patch borrows ideas from macvtap.c, the only other file in libvirt that currently uses recvfrom.
I did not analyze whether this is a security hole, where a malicious UDP packet could intentionally force the dereferencing of uninitialized memory to misbehave in a controlled manner.
src/uml/uml_driver.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index bbea429..eec239f 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -733,14 +733,24 @@ static int umlMonitorCommand(virConnectPtr conn, }
do { + ssize_t nbytes; addrlen = sizeof(addr); - if (recvfrom(priv->monitor, &res, sizeof res, 0, - (struct sockaddr *)&addr, &addrlen) < 0) { + nbytes = recvfrom(priv->monitor, &res, sizeof res, 0, + (struct sockaddr *)&addr, &addrlen) < 0; + if (nbytes < 0) { + if (errno == EAGAIN || errno == EINTR) + continue; virReportSystemError(errno, _("cannot read reply %s"), cmd); goto error; } + if (nbytes < sizeof res) { + virReportSystemError(errno, + _("incomplete reply %s"), + cmd); + goto error; + }
if (VIR_REALLOC_N(retdata, retlen + res.length) < 0) { virReportOOMError();
ACK, looks fine ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard wrote:
On Tue, Mar 02, 2010 at 05:16:05PM -0700, Eric Blake wrote:
Coverity detected a potential dereference of uninitialized memory if recvfrom got cut short.
* src/uml/uml_driver.c (umlMonitorCommand): Validate complete read prior to dereferencing res. ---
The patch borrows ideas from macvtap.c, the only other file in libvirt that currently uses recvfrom.
I did not analyze whether this is a security hole, where a malicious UDP packet could intentionally force the dereferencing of uninitialized memory to misbehave in a controlled manner.
src/uml/uml_driver.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index bbea429..eec239f 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -733,14 +733,24 @@ static int umlMonitorCommand(virConnectPtr conn, }
do { + ssize_t nbytes; addrlen = sizeof(addr); - if (recvfrom(priv->monitor, &res, sizeof res, 0, - (struct sockaddr *)&addr, &addrlen) < 0) { + nbytes = recvfrom(priv->monitor, &res, sizeof res, 0, + (struct sockaddr *)&addr, &addrlen) < 0; + if (nbytes < 0) { + if (errno == EAGAIN || errno == EINTR) + continue; virReportSystemError(errno, _("cannot read reply %s"), cmd); goto error; } + if (nbytes < sizeof res) { + virReportSystemError(errno, + _("incomplete reply %s"), + cmd); + goto error; + }
if (VIR_REALLOC_N(retdata, retlen + res.length) < 0) { virReportOOMError();
ACK, looks fine !
I pushed this, and then (sorry I missed it the first time) noticed that we'd rather avoid that new use of errno. errno is not defined for a shorter-than-expected read, so including strerror(errno) in the diagnostic would be misleading.
According to Jim Meyering on 3/3/2010 2:29 AM:
ACK, looks fine !
I pushed this, and then (sorry I missed it the first time) noticed that we'd rather avoid that new use of errno. errno is not defined for a shorter-than-expected read, so including strerror(errno) in the diagnostic would be misleading.
Good point. Were you planning on the followup patch for this, or do I need to pick up the slack? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
According to Jim Meyering on 3/3/2010 2:29 AM:
ACK, looks fine !
I pushed this, and then (sorry I missed it the first time) noticed that we'd rather avoid that new use of errno. errno is not defined for a shorter-than-expected read, so including strerror(errno) in the diagnostic would be misleading.
Good point. Were you planning on the followup patch for this, or do I need to pick up the slack?
I figured you'd do it while addressing the malicious input problem a few lines below.
Otherwise, a malicious packet could cause a DoS via spurious out-of-memory failure. * src/uml/uml_driver.c (umlMonitorCommand): Validate that incoming data is reliable before using it to allocate/dereference memory. Don't report bogus errno on short read. Reported by Jim Meyering. --- src/uml/uml_driver.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index eec239f..130d1ae 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -746,11 +746,17 @@ static int umlMonitorCommand(virConnectPtr conn, goto error; } if (nbytes < sizeof res) { - virReportSystemError(errno, + virReportSystemError(0, _("incomplete reply %s"), cmd); goto error; } + if (sizeof res < res.length) { + virReportSystemError(0, + _("invalid length in reply %s"), + cmd); + goto error; + } if (VIR_REALLOC_N(retdata, retlen + res.length) < 0) { virReportOOMError(); -- 1.6.6.1
According to Eric Blake on 3/3/2010 9:34 AM:
Otherwise, a malicious packet could cause a DoS via spurious out-of-memory failure.
+ if (sizeof res < res.length) {
Phooey; posted the wrong version. That should be sizeof res.data, not sizeof res, given the later use of: memcpy(retdata + retlen, res.data, res.length); -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
According to Eric Blake on 3/3/2010 9:34 AM:
Otherwise, a malicious packet could cause a DoS via spurious out-of-memory failure.
+ if (sizeof res < res.length) {
Phooey; posted the wrong version. That should be sizeof res.data, not sizeof res, given the later use of:
memcpy(retdata + retlen, res.data, res.length);
I knew what you meant ;) Glad you caught it.
Eric Blake wrote:
Otherwise, a malicious packet could cause a DoS via spurious out-of-memory failure.
* src/uml/uml_driver.c (umlMonitorCommand): Validate that incoming data is reliable before using it to allocate/dereference memory. Don't report bogus errno on short read. Reported by Jim Meyering. --- src/uml/uml_driver.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index eec239f..130d1ae 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -746,11 +746,17 @@ static int umlMonitorCommand(virConnectPtr conn, goto error; } if (nbytes < sizeof res) { - virReportSystemError(errno, + virReportSystemError(0, _("incomplete reply %s"), cmd); goto error; } + if (sizeof res < res.length) { + virReportSystemError(0, + _("invalid length in reply %s"), + cmd); + goto error; + }
Thanks. That looks perfect. ACK. Hmm... while you're there, you might want to save 4 lines by joining those unnecessarily-continued ones: virReportSystemError(0, _("invalid length in reply %s"), cmd);
According to Jim Meyering on 3/3/2010 9:41 AM:
Eric Blake wrote:
if (nbytes < sizeof res) { - virReportSystemError(errno, + virReportSystemError(0, _("incomplete reply %s"), cmd); goto error; } + if (sizeof res < res.length) { + virReportSystemError(0, + _("invalid length in reply %s"),
Hmm... while you're there, you might want to save 4 lines by joining those unnecessarily-continued ones:
virReportSystemError(0, _("invalid length in reply %s"), cmd);
Sure. Respin coming up (this time, in a new thread, to make it easier to spot). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel Veillard -
Eric Blake -
Jim Meyering