[libvirt] [PATCHv2] uml: sanity check external data before using it
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 | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index eec239f..4a5db9d 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -740,15 +740,15 @@ static int umlMonitorCommand(virConnectPtr conn, if (nbytes < 0) { if (errno == EAGAIN || errno == EINTR) continue; - virReportSystemError(errno, - _("cannot read reply %s"), - cmd); + virReportSystemError(errno, _("cannot read reply %s"), cmd); goto error; } if (nbytes < sizeof res) { - virReportSystemError(errno, - _("incomplete reply %s"), - cmd); + virReportSystemError(0, _("incomplete reply %s"), cmd); + goto error; + } + if (sizeof res.data < res.length) { + virReportSystemError(0, _("invalid length in reply %s"), cmd); goto error; } -- 1.6.6.1
On 03/03/2010 11:52 AM, 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 | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index eec239f..4a5db9d 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -740,15 +740,15 @@ static int umlMonitorCommand(virConnectPtr conn, if (nbytes < 0) { if (errno == EAGAIN || errno == EINTR) continue; - virReportSystemError(errno, - _("cannot read reply %s"), - cmd); + virReportSystemError(errno, _("cannot read reply %s"), cmd); goto error; } if (nbytes < sizeof res) { - virReportSystemError(errno, - _("incomplete reply %s"), - cmd); + virReportSystemError(0, _("incomplete reply %s"), cmd); + goto error; + } + if (sizeof res.data < res.length) { + virReportSystemError(0, _("invalid length in reply %s"), cmd); goto error; }
Let me preface this by saying that this is the first time I've ever looked at UML. With that being said, I'm not sure this above check is correct. sizeof(res.data) is always a constant 512, but res.length may or may not vary based on the message. That is, it looks to me like it's perfectly valid for res.length to be less than res.data. In point of fact the code in uml_mconsole.c (which is where this was ported from) ignores res.length altogether and just uses strlen(res.data) to realloc and copy. I'd be wary of pushing this code without testing it out under UML. -- Chris Lalancette
On 03/09/2010 12:17 PM, Chris Lalancette wrote:
if (nbytes < sizeof res) { - virReportSystemError(errno, - _("incomplete reply %s"), - cmd); + virReportSystemError(0, _("incomplete reply %s"), cmd); + goto error; + } + if (sizeof res.data < res.length) { + virReportSystemError(0, _("invalid length in reply %s"), cmd); goto error; }
Let me preface this by saying that this is the first time I've ever looked at UML.
I'm with you in the fresh eyes category on this one :)
With that being said, I'm not sure this above check is correct. sizeof(res.data) is always a constant 512, but res.length may or may not vary based on the message. That is, it looks to me like it's perfectly valid for res.length to be less than res.data.
That is a true statement. But this check is whether res.length is _larger_ than 512, in which case the packet is bogus.
In point of fact the code in uml_mconsole.c (which is where this was ported from) ignores res.length altogether and just uses strlen(res.data) to realloc and copy.
Possibly a valid approach, but the version in this patch was the one recommended by Jim. For that matter, is strlen(res.data) safe, or can it run past the end of res.length bytes if the user (maliciously) failed to pass in a NUL terminator?
I'd be wary of pushing this code without testing it out under UML.
All right then - any advice from someone who HAS used UML on how to test this beyond mere code inspection? Related question: does libvirt-tck exercise UML? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/09/2010 04:31 PM, Eric Blake wrote:
On 03/09/2010 12:17 PM, Chris Lalancette wrote:
if (nbytes < sizeof res) { - virReportSystemError(errno, - _("incomplete reply %s"), - cmd); + virReportSystemError(0, _("incomplete reply %s"), cmd); + goto error; + } + if (sizeof res.data < res.length) { + virReportSystemError(0, _("invalid length in reply %s"), cmd); goto error; }
Let me preface this by saying that this is the first time I've ever looked at UML.
I'm with you in the fresh eyes category on this one :)
With that being said, I'm not sure this above check is correct. sizeof(res.data) is always a constant 512, but res.length may or may not vary based on the message. That is, it looks to me like it's perfectly valid for res.length to be less than res.data.
That is a true statement. But this check is whether res.length is _larger_ than 512, in which case the packet is bogus.
Gah, I looked at it backwards. You are right of course. Still, we might want to verify that res.length is valid.
In point of fact the code in uml_mconsole.c (which is where this was ported from) ignores res.length altogether and just uses strlen(res.data) to realloc and copy.
Possibly a valid approach, but the version in this patch was the one recommended by Jim. For that matter, is strlen(res.data) safe, or can it run past the end of res.length bytes if the user (maliciously) failed to pass in a NUL terminator?
This is a good point too. I don't know the answer.
I'd be wary of pushing this code without testing it out under UML.
All right then - any advice from someone who HAS used UML on how to test this beyond mere code inspection? Related question: does libvirt-tck exercise UML?
I know danpb originally wrote the code, and does use it. Whether libvirt-tck exercises it is unknown to me. -- Chris Lalancette
participants (2)
-
Chris Lalancette -
Eric Blake