2012/7/8 Doug Goldstein <cardoe@xxxxxxxxxx>:
> On Sun, Jul 8, 2012 at 5:44 AM, Matthias Bolte
> <matthias.bolte@xxxxxxxxxxxxxx> wrote:
>> Also ensure that the virBuffer used to store the downloaded data
>> does not overflow.
>> ---
>>
>> v2:
>> - Ensure that the used virBuffer dos not overflow.
>>
>> src/esx/esx_driver.c | 2 +-
>> src/esx/esx_vi.c | 62 +++++++++++++++++++++++++++++++++++++++++++------
>> src/esx/esx_vi.h | 3 +-
>> 3 files changed, 57 insertions(+), 10 deletions(-)
>> -esxVI_CURL_WriteBuffer(char *data, size_t size, size_t nmemb, void *buffer)
>> +esxVI_CURL_WriteBuffer(char *data, size_t size, size_t nmemb, void *userdata)
>> {
>> + virBufferPtr buffer = userdata;
>> +
>> if (buffer != NULL) {
>> - virBufferAdd((virBufferPtr) buffer, data, size * nmemb);
>> + /*
>> + * Using a virBuffer to store the download data limits the downloadable
>> + * size. This is no problem as esxVI_CURL_Download and esxVI_CURL_Perform
>> + * are meant to download small things such as VMX files, VMDK metadata
>> + * files and SOAP responses.
>> + */
>> + if (virBufferUse(buffer) > UINT32_MAX / 2) {
>> + return 0;
>> + }
>
> This could never be true since the type would have already resulted in
> an overflow and therefore would be less than UINT32_MAX / 2 (which is
> equivalent to INT32_MAX).
That's not true, virBufferUse returns unsigned int. Also virBuffer
stores it's size internally as unsigned int. I just looked it up
again. But then there is a bug in virBufferGrow that uses an
intermediate int variable to store the new size. This limits a
virBuffer to hold at most 2GB. Also virBuffer doesn't do any overflow
checks internally.
> Something like the below would ensure that
> you are always within the bounds of your type :
>
> if ((size * nmemb) <= (INT32_MAX - virBufferUse(buffer)) {
> virBufferAdd(...)
> return size * nmemb;
> }
Therefore this isn't safe either. virBufferAdd can grow the buffer
beyond the required size, for INT32_MAX it'll probably overflow. I'll
use INT32_MAX / 2 as the limit here, because now I assume that a
virBuffer can safely hold at most INT32_MAX / 2. This is the same
pattern as with UINT32_MAX / 2 where I assumed a virBuffer would be
unsigned int clean in it's internals.
>> @@ -365,9 +379,33 @@ esxVI_CURL_Download(esxVI_CURL *curl, const char *url, char **content)
>> return -1;
>> }
>>
>> + if (length != NULL && *length > 0) {
>> + /*
>> + * Using a virBuffer to store the download data limits the downloadable
>> + * size. This is no problem as esxVI_CURL_Download is meant to download
>> + * small things such as VMX of VMDK metadata files.
>> + */
>> + if (*length > UINT32_MAX / 2) {
>
> Use INT32_MAX
I'll use INT32_MAX / 2 for the reason explain above.
--
Matthias Bolte
http://photron.blogspot.com
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list