[patch] libpurple/protocols/oscar: OOM and die on misparsed ICQWebMessage as ICQSMS

[Yuriy Kaminskiy]

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.
01) fixes in byte_stream_getstr: early check len for validity (this will
cause error later anyway), and only then allocate memory.
02) fixes in incomingim_chan4/case 0x1a: better checks for expected
format and errors (and not choke on some unknown gibberish).
03-04) [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.

--- pidgin-2.5.1/libpurple/protocols/oscar/bstream.c.orig 2008-08-31 08:36:26.000000000 +0400
+++ pidgin-2.5.1/libpurple/protocols/oscar/bstream.c 2008-11-09 01:26:22.000000000 +0300
@@ -177,6 +177,9 @@ guint8 *byte_stream_getraw(ByteStream *b
 {
  guint8 *ob;
 
+ if (byte_stream_empty(bs) < len)
+ return NULL;
+
  ob = g_malloc(len);
 
  if (byte_stream_getrawbuf(bs, ob, len) < len) {
@@ -191,6 +195,9 @@ char *byte_stream_getstr(ByteStream *bs,
 {
  char *ob;
 
+ if (byte_stream_empty(bs) < len)
+ return NULL;
+
  ob = g_malloc(len + 1);
 
  if (byte_stream_getrawbuf(bs, (guint8 *)ob, len) < len) {

--- pidgin-2.5.1/libpurple/protocols/oscar/oscar.c.orig 2008-08-31 08:36:26.000000000 +0400
+++ pidgin-2.5.1/libpurple/protocols/oscar/oscar.c 2008-11-09 02:19:24.000000000 +0300
@@ -2740,9 +2740,15 @@ incomingim_chan4(OscarData *od, FlapConn
  /* From libicq2000-0.3.2/src/ICQ.cpp */
  byte_stream_init(&qbs, (guint8 *)args->msg, args->msglen);
  byte_stream_advance(&qbs, 21);
+ /* expected: 01 00 00 20 00 0e 28 f6 00 11 e7 d3 11 bc f3 00 04 ac 96 9d c2 | 00 00 | 06 00 00 00 | 49 43 51 53 43 53 ...*/
+ /* unexpected: 00 00 26 00 81 1a 18 bc 0e 6c 18 47 a5 91 6f 18 dc c7 6f 1a | 00 00 | 0d 00 00 00 | 49 43 51 57 65 62 4d 65 73 73 61 67 65 ... */
  smstype = byte_stream_getle16(&qbs);
+ if (smstype != 0)
+ break;
  taglen = byte_stream_getle32(&qbs);
  tagstr = byte_stream_getstr(&qbs, taglen);
+ if (tagstr == NULL)
+ break;
  byte_stream_advance(&qbs, 3);
  byte_stream_advance(&qbs, 4);
  smslen = byte_stream_getle32(&qbs);

Index: pidgin-2.5.6/libpurple/protocols/oscar/bstream.c
===================================================================
--- pidgin-2.5.6.orig/libpurple/protocols/oscar/bstream.c 2009-05-27 19:52:11.856903422 +0400
+++ pidgin-2.5.6/libpurple/protocols/oscar/bstream.c 2009-05-27 19:52:57.224901046 +0400
@@ -161,16 +161,22 @@ guint32 byte_stream_getle32(ByteStream *
  return aimutil_getle32(bs->data + bs->offset - 4);
 }
 
+static void byte_stream_getrawbuf_nocheck(ByteStream *bs, guint8 *buf, int len);
+
 int byte_stream_getrawbuf(ByteStream *bs, guint8 *buf, int len)
 {
 
  if (byte_stream_empty(bs) < len)
  return 0;
 
+ byte_stream_getrawbuf_nocheck(bs, buf, len);
+ return len;
+}
+
+static void byte_stream_getrawbuf_nocheck(ByteStream *bs, guint8 *buf, int len)
+{
  memcpy(buf, bs->data + bs->offset, len);
  bs->offset += len;
-
- return len;
 }
 
 guint8 *byte_stream_getraw(ByteStream *bs, int len)
@@ -182,10 +188,7 @@ guint8 *byte_stream_getraw(ByteStream *b
 
  ob = g_malloc(len);
 
- if (byte_stream_getrawbuf(bs, ob, len) < len) {
- g_free(ob);
- return NULL;
- }
+ byte_stream_getrawbuf_nocheck(bs, ob, len);
 
  return ob;
 }
@@ -199,10 +204,7 @@ char *byte_stream_getstr(ByteStream *bs,
 
  ob = g_malloc(len + 1);
 
- if (byte_stream_getrawbuf(bs, (guint8 *)ob, len) < len) {
- g_free(ob);
- return NULL;
- }
+ byte_stream_getrawbuf_nocheck(bs, (guint8 *)ob, len);
 
  ob[len] = '\0';
 

Index: pidgin-2.5.6/libpurple/protocols/oscar/bstream.c
===================================================================
--- pidgin-2.5.6.orig/libpurple/protocols/oscar/bstream.c 2009-05-27 19:52:57.224901046 +0400
+++ pidgin-2.5.6/libpurple/protocols/oscar/bstream.c 2009-05-27 19:53:05.092905909 +0400
@@ -161,7 +161,11 @@ guint32 byte_stream_getle32(ByteStream *
  return aimutil_getle32(bs->data + bs->offset - 4);
 }
 
-static void byte_stream_getrawbuf_nocheck(ByteStream *bs, guint8 *buf, int len);
+static void byte_stream_getrawbuf_nocheck(ByteStream *bs, guint8 *buf, int len)
+{
+ memcpy(buf, bs->data + bs->offset, len);
+ bs->offset += len;
+}
 
 int byte_stream_getrawbuf(ByteStream *bs, guint8 *buf, int len)
 {
@@ -173,12 +177,6 @@ int byte_stream_getrawbuf(ByteStream *bs
  return len;
 }
 
-static void byte_stream_getrawbuf_nocheck(ByteStream *bs, guint8 *buf, int len)
-{
- memcpy(buf, bs->data + bs->offset, len);
- bs->offset += len;
-}
-
 guint8 *byte_stream_getraw(ByteStream *bs, int len)
 {
  guint8 *ob;

_______________________________________________
Devel mailing list
Devel@...
http://pidgin.im/cgi-bin/mailman/listinfo/devel

[Yuriy Kaminskiy]

Yuriy Kaminskiy wrote:
> I've got number of OOM/abort, and found that when pidgin receive
> chan4/0x1a/ICQWebMessage, it misparses that as ICQSMS, and dies on
> out-of-memory.
> 01) fixes in byte_stream_getstr: early check len for validity (this will
> cause error later anyway), and only then allocate memory.
> 02) fixes in incomingim_chan4/case 0x1a: better checks for expected
> format and errors (and not choke on some unknown gibberish).
Ping. If no-one noticed, this is security problem (just DoS, not remote
access, but nonetheless). At least some equivalent of patches 1 and 2
MUST be applied.

_______________________________________________
Devel mailing list
Devel@...
http://pidgin.im/cgi-bin/mailman/listinfo/devel

[Mark Doliner]

On Fri, Jun 12, 2009 at 7:59 AM, Yuriy Kaminskiy<yumkam@...> wrote:

> Yuriy Kaminskiy wrote:
>> I've got number of OOM/abort, and found that when pidgin receive
>> chan4/0x1a/ICQWebMessage, it misparses that as ICQSMS, and dies on
>> out-of-memory.
>> 01) fixes in byte_stream_getstr: early check len for validity (this will
>> cause error later anyway), and only then allocate memory.
>> 02) fixes in incomingim_chan4/case 0x1a: better checks for expected
>> format and errors (and not choke on some unknown gibberish).
> Ping. If no-one noticed, this is security problem (just DoS, not remote
> access, but nonetheless). At least some equivalent of patches 1 and 2
> MUST be applied.

Hi Yuriy!  You probably know this already after seeing the traffic
from ticket 8483[1], but we accepted all four of your patches a few
days ago, and they're being released now-ish in Pidgin 2.5.8.  Thank
you for finding and fixing this bug!  And sorry it took us so long to
actually do something :-(

Also, if you (or anyone else following along at home) happen to find
or fix any security problems in the future, we generally prefer that
you email a few developers directly instead of mailing this list.  We
try to fix problems before they're publicly announced, and we try to
set an embargo date and provide a patch to Linux distributions so they
can prepare packages ahead of time and release them as soon as the
security problem is announced.

Thanks!
-Mark

[1] http://developer.pidgin.im/ticket/9483

_______________________________________________
Devel mailing list
Devel@...
http://pidgin.im/cgi-bin/mailman/listinfo/devel