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.montez...@gmail.com> >
signature.asc
Description: OpenPGP digital signature
