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

[Yuriy M. Kaminskiy]

[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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bstream-integer-overflow.patch
Type: text/x-diff
Size: 1536 bytes
Desc: not available
URL: <http://pidgin.im/cgi-bin/mailman/private/security/attachments/20100804/e8fcb2a7/attachment.patch>