[Aptitude-devel] Bug#767533: Quick Question

[Joshua Rogers]

It looks like download_item.c:99 has the same code, just FYI.

-- Joshua Rogers <https://internot.info/>
On 01/11/14 07:03, Manuel A. Fernandez Montecelo wrote:
> Source: aptitude
> Version: 0.6.11-1
>
> 2014-10-30 11:23 Joshua Rogers:
>> Hi guys,
>>
>>
>> I was looking at the Aptitude source code, and came across this in the
>> src/download_list.cc file, starting from line 313:
>>
>>>           char intbuf[50]; // Waay more than enough.
>>>
>>>           sprintf(intbuf,
>>>                   " [ %sB/%sB ]",
>>>                   SizeToStr(serf->CurrentSize).c_str(),
>>>                   SizeToStr(serf->TotalSize).c_str());
>>
>> It is my understanding that 'serf->TotalSize' is determined by the
>> header values that the webserver sends to the client prior to sending
>> off the whole file.
>>
>> Since it uses the header values given by Apache, is it not possible to
>> spoof those numbers to cause a buffer overflow?
>>
>> Doing a quick check, the same code is used in src/download_item.cc on
>> line 99.
>> SizeToStr goes up to 'YottaBytes', I believe, so if one were to set the
>> size header of '100000000000000000000000000000000000000000 yottabytes',
>> they could cause a buffer overflow.
>> That is 1e+65 bytes.
>>
>> It probably isn't of concern, but I'd just like to report it incase. I
>> think it's good practise too.
>> Since it's not really a serious thing, I won't bother sending this to
>> the Debian security team.
>>
>>
>> Thanks,
>
> I think that this deserves at the very least to submit a bug report,
> to track
> the issue (doing it in BCC, please include the bug report address in
> future
> replies to this thread).  I think that the security team should be
> notified, so
> they can take a look and evaluate the risk, can you please do that?
>
> Taking a brief look at the code, the conversion function
> (apt-pkg/contrib/strutl.cc in apt package) says:
>
> -----------
> // SizeToStr - Convert a long into a human readable size        /*{{{*/
> // ---------------------------------------------------------------------
> /* A max of 4 digits are shown before conversion to the next highest
> unit.
>   The max length of the string will be 5 chars unless the size is > 10
>      YottaBytes (E24) */
> string SizeToStr(double Size)
> -----------
>
> Even without a deep analysis of how the conversion inside that
> function is
> performed at the moment (internally it uses buffers of 300 characters
> at the
> moment, so the output string, for legitimate reasons or mistake, could
> be bigger
> than the 50 allocated in aptitude), serf->CurrentSize is an "unsigned
> long
> long", which is 8 bytes in amd64 (and most/all other systems with
> wordsize==64),
> and then it's cast into the double of SizeToStr.  The max value of
> this value is
> (/usr/include/limits.h):
>
> -----------
> /* Maximum value an `unsigned long long int' can hold.  (Minimum is
> 0.)  */
> #   define ULLONG_MAX   18446744073709551615ULL
> -----------
>
> This value is 20 characters long when printed as '%llu' in amd64, so
> if this is
> the case, the full string in aptitude printed as " [ %sB/%sB ]" means
> 3+20+2+20+3=48 characters, null aside, but assuming that the given
> number is
> printed in 20 characters at most -- so the current side of 50 would be
> enough.
>
> But if the implicit conversion to double plays some tricks and the 20
> characters
> are actually more, or if the external project decides to add
> whitespace around
> the number before returning, or if in some architecture the "unsigned
> long long"
> can hold bigger numbers, it can indeed cause overflows.
>
> And in general, there's no need to risk this kind of overflows, which
> can be
> propagated even by copy and paste or if the envolving string is
> modified to
> e.g. " [ %sbytes/%sbytes ]".  Instead of using sprintf, snprintf (with
> the size
> of the buffer) should be used -- if not a better method to translate
> those sizes
> into string.
>
> Thanks for taking care.
>
> -- 
> Manuel A. Fernandez Montecelo <manuel.montezelo at gmail.com>
>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 884 bytes
Desc: OpenPGP digital signature
URL: <http://lists.alioth.debian.org/pipermail/aptitude-devel/attachments/20141101/ba44a741/attachment-0001.sig>