[Aptitude-devel] Bug#767533: Quick Question

[Manuel A. Fernandez Montecelo]

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>