[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2] uml: sanity check external data before using it



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]