Ticket #9483 (closed patch: fixed)

Opened 15 months ago

Last modified 14 months ago

OOM/abort when receiving ICQWebMessage

Reported by: darkrain42 Owned by: MarkDoliner
Milestone: 2.5.8 Component: ICQ
Version: 2.5.7 Keywords:
Cc: yumkam@…

Description

Quoting from http://pidgin.im/pipermail/devel/2009-May/008227.html (with some formatting changes by me. Yuriy is CCed on this ticket.):

Hello! Few month ago ;-) I've got number of OOM/(segv|abort), and found that when pidgin receive chan4/0x1a/ICQWebMessage, it misparses that as ICQSMS, and dies on out-of memory/sigsegv.

  1. fixes in byte_stream_getstr: early check len for validity (this will cause error later anyway), and only then allocate memory.
  2. fixes in incomingim_chan4/case 0x1a: better checks for expected format and errors (and not choke on some unknown gibberish). 3-4. [optional] remove introduced in (01) double checks and cleanup

PS patches checked and works on pidgin-2.5.{1..6}. PPS someone should also think about this:

byte_stream_init(&b, "\7abcde", 6);
len = byte_stream_get8(&b); // ok - len=7
foo = byte_stream_get_str(&b, len); // fails!
num = byte_stream_get32(&b); // reads junk! num = 0x64636261
bar = byte_stream_get_str(&b, num); // DIE! (before my patch) or fail;
// or, in more ``lucky'' case ("\7\1\0\0\0B") may read some unexpected
// gibberish into bar

There are too many places where byte_stream_get* functions used without any check for errors :-\ (from quick glance - no as drastic things[*] as with 4/0x1a, but I don't know code well enough to be sure). Maybe, it make sense to advance buffer to end (b->offset = b->len;) immediately on failed attempt to read something?

[*] that is, with 01_*.patch; without - it would be OOM/die.

Attachments

01_bstream-bigmalloc-2.patch (0.6 kB) - added by darkrain42 15 months ago.
fixes in byte_stream_getstr: early check len for validity (this will cause error later anyway), and only then allocate memory.
02_detect-misparse.patch (0.9 kB) - added by darkrain42 15 months ago.
fixes in incomingim_chan4/case 0x1a: better checks for expected
03_bstream-remove-double-check.patch (1.4 kB) - added by darkrain42 15 months ago.
[optional] remove introduced in (01) double checks and cleanup
04_bstream-cosmetics.patch (1.0 kB) - added by darkrain42 15 months ago.
[optional] remove introduced in (01) double checks and cleanup

Change History

Changed 15 months ago by darkrain42

fixes in byte_stream_getstr: early check len for validity (this will cause error later anyway), and only then allocate memory.

Changed 15 months ago by darkrain42

fixes in incomingim_chan4/case 0x1a: better checks for expected

Changed 15 months ago by darkrain42

[optional] remove introduced in (01) double checks and cleanup

Changed 15 months ago by darkrain42

[optional] remove introduced in (01) double checks and cleanup

Changed 15 months ago by darkrain42

  • milestone set to 2.6.0

Remote crashes are bad.

Changed 14 months ago by rekkanoryo@…

  • status changed from new to closed
  • resolution set to fixed

(In [9bac0a540156fb1848eedd61c8630737dee752c7]):
Fix misparsing an incoming ICQ Web Message as an incomig SMS message. Fixes #9483.

Changed 14 months ago by rekkanoryo@…

(In [634daccc29e66550cbe98be1dd1056d54f080a08]):
Credit where credit is due. Refs #9483.

Changed 14 months ago by rekkanoryo

  • milestone changed from 2.6.0 to 2.5.8

Changed 14 months ago by rekkanoryo@…

(In [84946a05a555bf00b813da3bbf4c1af3856a62e0]):
Apply [9bac0a540156fb1848eedd61c8630737dee752c7] here as well. I should actually have committed this here first, then plucked to 2.5.8, but by the time I realized this it was too late to fix. Refs #9483.

Changed 14 months ago by markdoliner@…

(In [1df9e851fe40753e2fb7bb06f0b1a07c97df33ab]):
Add Yuriy Kaminskiy to the COPYRIGHT file. This is the guy who found and fixed the ICQ SMS bugs. Refs #9483

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!