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

[Laine Stump]

  On 07/16/2010 03:04 AM, Eduardo Otubo wrote:
> On 07/15/2010 11:01 PM, Laine Stump wrote:
>> On 07/15/2010 06:37 PM, Eduardo Otubo wrote:
>>> The line src/phyp/phyp_driver.c:427 was crashing by buffer overflow
>>> if the return of the command wasn't<=10. The highest number for a
>>> LPAR ID is 256 per machine, no need to allocate 10 bytes for it. So,
>>> adjusting the correct size (+1 byte for the '\n') and checking for
>>> errors.
>>
>> This analysis doesn't make sense to me. On the one hand, you say that
>> the function was crashing if the return from the command wasn't<= 10
>> bytes long (implying that it sometimes was longer), on the other hand,
>> you say that part of the solution is to change the code to only handle
>> 4 bytes (you forgot about the trailing NULL there, BTW - should have
>> been 3), because you'll never get more than that.
>
> I'm sorry. My bad here. The lssyscfg (with the correct parameters and 
> greps) will return something like this:
>
> 5
> 4
> 6
> 7
> 10
> 1
> 143
>
> These are the LPAR IDs. Each line cannot be bigger than 3 characters 
> (+1 for '\n'). So, I just allocate 4 bytes to hold these values, one 
> at a time and memset it with zeros. If there's an error, there will be 
> a single line with the error message, like this one:
>
> "HSCL8018 The managed system rico was not found."
>
> IMO the best choice is to loop in the result and if I can't find a 
> '\n' after 3 character, I return an error. My mistake was to check the 
> entire string returned from exec. My bad, sorry.
>
> I think this patch is much clearer on what the problem is.
(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)

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.

It takes advantage of the fact that virStrToLong_i will terminate on any 
non-numeric (not just NULL) to avoid the copy completely thus 
eliminating any built-in limits, and uses virStrToLong_i's returned 
"endptr" in place of counting characters itself. The resulting 
advantages are:

1) It is more "future proof". Maybe for now lssyscfg will not have any 
id's > 256, that may change in the future, and we have no way of knowing 
how long this code will last. For example, even if there are never more 
than 999 of them, maybe someone down the road will decide that these 
id's should be randomly generated for security reasons or something.

2) Because there is no fixed limit, there is no possibility of any "off 
by one" bugs.

3) The complicated character copying loop has been eliminated.

4) Also, error and success exit paths have been merged, meaning that 
resources are cleared in a single place, making it more difficult to 
forget to free resources on one exit path but not the other.

All of these make the function easier to verify as bug-free now, and 
will make future maintenance easier, since it will be both easier to 
understand, and more difficult to break (this current iteration of 
patches is ample proof that the current design makes it more difficult 
to catch bugs ;-)