10:52 a.m.
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
1:17 p.m.
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
3:31 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:54 p.m.
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
5372
days inactive
5378
days old
3 comments
2 participants
participants (2)
-
Chris Lalancette -
Eric Blake