[libvirt] [PATCH] phyp: Fixing possible buffer overflow

[Eduardo Otubo]

On 08/05/2010 04:04 PM, Laine Stump wrote:
> On 08/04/2010 03:00 PM, Eduardo Otubo wrote:
>> Hello Laine,
>>
>> It's been quite a while since we had this discussion about my patch.
>> Sorry about the delay on replaying, I had a congress and I've been
>> sick in the last couple of days. Now going back to work. :-)
>>
>>> (this new patch is corrupt and doesn't apply. Not sure how you sent it,
>>> but I'm guessing your mailer mangled it. It's usually better to only
>>> send patches with git send-email, or to attach the output of git
>>> format-patch to the mail, rather than pasting it inline)
>>
>> I don't know what happened, I always send patches using git
>> send-email. I'll recheck next times. Thanks.
>>
>>>
>>> This version is closer *in location* to avoiding the overflow, but it's
>>> still comparing the wrong index - it's looking at the index into the
>>> full array (ret, ie "i") rather than into the small copy that is used to
>>> convert (id_c, ie "j").
>>>
>>> Also, it still allows writing past the end of the array (i == j == 4, so
>>> it can write to id_c[4]). Even allowing a write to id_c[3] would be a
>>> problem, since that would overwrite the terminating NULL, thus creating
>>> the possibility that virStrToLong_i could overrun the end of the array
>>> (or, if virStrToLong_i fails, the VIR_ERROR following it would try to
>>> print a string that is not NULL terminated - a *much* worse prospect).
>>>
>>> Additionally the 2nd occurence of "memset(id_c, 0, 10)" inside the loop
>>> that I noted in my last mail has been left unchanged (and this memset
>>> will be done every time the function is called, even if there is no
>>> overflow of string length, so it is *guaranteed* the buffer will be
>>> overflowed).
>>>
>>> Finally, in the case that the output of the exec is too long, this new
>>> patch will simply exit from the loop early and return success, rather
>>> than logging an error and returning failure.
>>>
>>> These problems *could* be fixed by 1) changing "i <= 4" to "j < 3", 2)
>>> fixing the 2nd memset to clear 4 bytes instead of 10, and 3) turning the
>>> case of "j >= 3" into an error instead of just exiting the loop.
>>>
>>> However... did you look at the counter-proposed patch in my previous
>>> email? I'm assuming that either you didn't notice it, or that you're
>>> trying to come up with a patch that makes the fewest changes possible
>>> while fixing the perceived bug. I think that patch would be a better
>>> idea for several reasons, and as long as we're fixing this function, we
>>> may as well fix it in the best way possible.
>>
>> I just saw your patch now, seem reasonable. I understand your points
>> and I believe your patch fixes the bug pretty well. Applied here,
>> compiled and run with some tests, seems pretty ack for me.
>
> I just want to verify: you applied the patch from my mail with 0
> changes, and built/tested with that, correct?
>
>

Yes, applied your patch with no changes to a clean branch.

-- 
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
eotubo at linux.vnet.ibm.com