Remotely-triggerable crash in oscar xstatus code
Paul Aurich
paul at darkrain42.org
Wed Jul 14 02:08:58 EDT 2010
On 2010-06-18 18:18, Mark Doliner wrote:
> I believe I found a security problem (frowny face). I'm going out of
> town Sunday through July 4th and will have limited Internet access.
> After someone confirms this problem and confirms that the attached
> patch is a good fix, would anyone be willing to contact the packagers
> list, provide this info, and request a CVE number? I do not believe
> this bug is known in the wild, so maybe we can set an embargo date
> around July 10th? I'm fine with making that soon or later.
Well, I think we should reschedule that a little ;) I do apologize for
leaving this sit for so long (especially with John's gentle nudges every
once-in-a-while).
Except for one issue I had with the patch (and ignoring my issues with
the thought of escaped HTML inside "XML" inside a binary protocol), this
looks good and I think we should consider making a release relatively soon.
> As always, please do not disclose this information to the public
> unless we have released fixed source and binary packages.
>
> Full description:
>
> This patch attempts to fix four bugs in the oscar protocol plugin that
> were introduced with the X-Status code in Pidgin 2.7.0.
>
> Problem #1 (the remotely-triggerable crash):
> The crash happens when a buddy sets an xstatus message containing <desc>
> but no closing </desc>, or <title> but no closing </title>. The fix
> is to check the result of strstr(closing_tag_name) and do nothing if it
> is NULL.
Yep. Jan Kaluza (HanzZ) has a backtrace of this happening in the wild
(dump of the bytestream at the bottom of this email). Actually, I have
a question about the bytestream there:
It looks like there are two xstatus updates back-to-back. I can't
recognize SNAC-framing-in-octal, so I can't tell if that's two SNAC
packets smooshed together or just two xstatus updates, but if it's the
latter, the code doesn't handle it (and if it's the former, I think
that's a bug somewhere else in the prpl)
> Problem #2:
> Fixes potential incorrect parsing of the xstatus string that could result
> in an incorrect message being displayed to the libpurple user. Happens if
> an xstatus message contains </desc> before <desc>, or </title> before
> <title>. The fix is to start looking for the closing tag at the end
> of the beginning tag rather than at the beginning of the xstatus xml.
> Probably not a security problem, but definitely a bug.
Yeah, I noticed that while following the logic looking at HanzZ's crash
(I really should have read this email a month ago! I'm sorry!) and your
patch seems to fix it almost correctly.
I think the two instances of "tmp1 += 12" and "tmp1 += 13" are backwards
("title" is one character more than "desc"), although I'd personally
prefer strlen("stuff") just for readability.
> Problem #3:
> Fixes potential incorrect parsing of the xstatus string that could result
> in the title not being shown to the libpurple user. Happens if the close
> title tag appears after the desc tag in the xstatus xml, because we add a
> null character at the beginning of the close title tag, so strstr() for
> the desc tag would stop searching there. Probably not a security problem,
> but definitely a bug.
>
> Problem #4:
> Fixes potential incorrect display of the xstatus string that could result
> in an incorrect message being displayed to the libpurple user. Happens
> because we reusing the 'xml' string when preparing the string for the user,
> but we copy values from xml to xml. If those values overlap with themselves
> or with each other then an incorrect value could be displayed. Probably not
> a security problem, but definitely a bug.
>
Your patch fixes those AFAICT, too.
> --Mark
>
>
Bytestream from the crash (at the time of the crash, 'xml' points to the
first instance of "<NR><RES>", and it's a NULL dereference on the
'title' tag):
$14 =
"\000\004\000\v\000\000\231s)(2862777\000\000\002\t198794995\000\003\033\000\b",
'\0' <repeats 19 times>, "\003\000\000\000\004��\016\000��", '\0'
<repeats 12 times>,
"\032\000\000\000\000\000\001\000\000O\000;`���*lE��\234Z^g�e\b\000*\000\000\000Script
Plug-in: Remote Notification Arrive\000\000\001", '\0' <repeats 12
times>, "�\004\000\000�\004\000\000<NR><RES><ret
event='OnRemoteNotification'><srv><id>cAwaySrv</id><val
srv_id='cAwaySrv'><Root><CASXtraSetAwayMessage></CASXtraSetAwayMessage><uin>198794995</uin><index>17</index><title>Mindfulness
fosters happiness. Our intentional, effortful
activities*\002\023\t\005\204\000\004\000\v\000\000Q�\000\v2862777\000\000\002\t361595606\000\003\033\000\b",
'\0' <repeats 19 times>, "\003\000\000\000\004��\016\000��", '\0'
<repeats 12 times>,
"\032\000\000\000\000\000\001\000\000O\000;`���*lE��\234Z^g�e\b\000*\000\000\000Script
Plug-in: Remote Notification Arrive\000\000\001", '\0' <repeats 12
times>, "�\004\000\000�\004\000\000<NR><RES><ret
event='OnRemoteNotification'><srv><id>cAwaySrv</id><val
srv_id='cAwaySrv'><Root><CASXtraSetAwayMessage></CASXtraSetAwayMessage><uin>198794995</uin><index>17</index><title>Mindfulness
fosters happiness. Our intentional, effortful activities have a powerful
effect on how happy we are, over and above the effects of our set points
and the circumstances in which we find
ourselves.</title><desc>People high in mindfulness - that
is, those who are prone to be mindfully attentive to the here and now
and keenly aware of their surroundings - are models of flourishing and
positive mental health.\r\n---\r\nWe have control of a big"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 897 bytes
Desc: OpenPGP digital signature
URL: <http://pidgin.im/cgi-bin/mailman/private/security/attachments/20100713/6e474faa/attachment.pgp>
More information about the security
mailing list