[patch] potential integer overflow in libpurple/protocols/oscar/byte_stream.c (Was: FYI: 7e159eaa14b0041fcc3ee5783cd1e4f2d039a1a1 (included in pidgin-2.7.2) is unneeded cruft)

[Mark Doliner]

2010/8/3 Yuriy M. Kaminskiy <yumkam at gmail.com>:
> [moving to security@, as I guess this patch CAN have security implication]
> Note: for me this is theoretical problem, I have NO crashes, NO backtraces, NO
> reproduction cases; maybe crashes mentioned by Paul Aurich related, maybe not -
> /I/ never seen any backtraces/core/..., cannot say for sure.
>
> On Tue, 03 Aug 2010 17:58:22 -0700 Paul Aurich <paul at darkrain42.org> wrote:
>> On 2010-08-03 17:35, Yuriy Kaminskiy wrote:
>>> This patch had sense only before pidgin-2.5.8 (seems someone was even more slow
>>> with pushing patches upstream than me :-)).
>>> My patch (included in 2.5.8) fixed this problem in more generic way - now it is
>>> impossible to allocate those "big amount of memory", as *before* allocation
>>> byte_stream_getstr would check for available buffer size (which was already read
>>> from network and allocated [more than once; pidgin is far from being zero-copy
>>> design], and so cannot be "large").
>>> So no wonder you could not reproduce this issue (it *was* very real [with
>>> security implications] issue before, but already fixed [in different way] long
>>> time ago).
>>
>> Sadly, this is incorrect.  There were at least two people who reported
>> intermittent (unreproducible-ish) crashes in this area in post-2.5.8
>> code (I'm uncertain on versions, but I know at least one of those MUST
>> have been using 2.6.0+).
>
> Then this problem is NOT with length, and this patch hided real issue at best.
>
> After your reply I looked at code once more, and /found/ one more potential
> issue with length: it can be negative (more likely with those misparsed
> ICQWebMessage).
>
> However, 7e159ea patch CANNOT fix this issue any more than my already applied
> patch does.
>
> Quick patch attached (I think better fix should change byte_stream_* prototypes
> to unsigned or size_t len [but this requires more code review]); also, while I'm
> sure this patch cannot make situation /worse/, careful look at all callsites
> required, this is not something "ready for inclusion";
> and I have a feeling there are many other places where byte_stream_getle*
> returning negative result can be problematic.

Hey Yuriy, thanks for emailing about this.  I believe I fixed the
specific problem with channel 2 sms messages by changing the length
variables to be unsigned.  That change went out in Pidgin 2.7.3.

And after the release I changed the ByteStream struct and related
functions to take gsize arguments instead of signed ints.  This should
cause different (and still incorrect) behavior if a negative number is
passed in.

However, I suspect the problem isn't too bad.  I don't believe there
are many places where other clients are able to set this "length"
value.  In fact, I suspect that only happens when our parsing code is
incorrect.  I'm hoping the channel 2 SMS code is the only place where
that is the case.

But if you think there is a problem in a specific bit of code then
please let us know.

Thanks again,
Mark