Re: [libvirt] [PATCHv2] uml: sanity check external data before using it
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