Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#9483 closed patch (fixed)

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 (4)

01_bstream-bigmalloc-2.patch (609 bytes) - added by darkrain42 7 years 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 (968 bytes) - added by darkrain42 7 years ago.
fixes in incomingim_chan4/case 0x1a: better checks for expected
03_bstream-remove-double-check.patch (1.4 KB) - added by darkrain42 7 years ago.
[optional] remove introduced in (01) double checks and cleanup
04_bstream-cosmetics.patch (1.0 KB) - added by darkrain42 7 years ago.
[optional] remove introduced in (01) double checks and cleanup

Download all attachments as: .zip

Change History (10)

Changed 7 years 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 7 years ago by darkrain42

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

Changed 7 years ago by darkrain42

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

Changed 7 years ago by darkrain42

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

comment:1 Changed 7 years ago by darkrain42

  • Milestone set to 2.6.0

Remote crashes are bad.

comment:2 Changed 7 years ago by rekkanoryo@…

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:3 Changed 7 years ago by rekkanoryo@…

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

comment:4 Changed 7 years ago by rekkanoryo

  • Milestone changed from 2.6.0 to 2.5.8

comment:5 Changed 7 years 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.

comment:6 Changed 7 years 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!